test(plugin-js-packages-e2e): create vulnerabilities e2e test

[vmasek]

No description provided.

[github-actions]

Code PushUp

🤨 Code PushUp report has both improvements and regressions – compared target commit b64f036 with source commit 2a05a16.

🕵️ See full comparison in Code PushUp portal 🔍

🏷️ Categories

🏷️ Category	⭐ Previous score	⭐ Current score	🔄 Score change
Performance	🟡 51	🔴 48
Code coverage	🟢 91	🟢 91	–
Security	🟡 81	🟡 81	–
Updates	🟡 80	🟡 80	–
Accessibility	🟢 91	🟢 91	–
Best Practices	🟢 100	🟢 100	–
SEO	🟡 61	🟡 61	–
Bug prevention	🟢 100	🟢 100	–
Code style	🟢 100	🟢 100	–

👎 1 group regressed, 👍 1 audit improved, 👎 4 audits regressed, 11 audits changed without impacting score

🗃️ Groups

🔌 Plugin	🗃️ Group	⭐ Previous score	⭐ Current score	🔄 Score change
Lighthouse	Performance	🟡 51	🔴 48

16 other groups are unchanged.

🛡️ Audits

🔌 Plugin	🛡️ Audit	📏 Previous value	📏 Current value	🔄 Value change
Lighthouse	Largest Contentful Paint	🟨 3.5 s	🟨 3.9 s
Lighthouse	Speed Index	🟨 5.7 s	🟥 6.2 s
Lighthouse	First Contentful Paint	🟥 3.2 s	🟨 2.9 s
Lighthouse	Total Blocking Time	🟥 3,380 ms	🟥 4,540 ms
Lighthouse	Time to Interactive	🟥 14.1 s	🟥 14.5 s
Lighthouse	Avoids enormous network payloads	🟩 Total size was 1,824 KiB	🟩 Total size was 1,820 KiB
Lighthouse	Minimizes main-thread work	🟥 13.7 s	🟥 15.6 s
Lighthouse	JavaScript execution time	🟥 7.0 s	🟥 7.9 s
Lighthouse	Metrics	🟩 100%	🟩 100%
Lighthouse	Max Potential First Input Delay	🟥 2,470 ms	🟥 2,050 ms
Lighthouse	Eliminate render-blocking resources	🟥 Potential savings of 1,000 ms	🟥 Potential savings of 790 ms
Lighthouse	Server Backend Latencies	🟩 310 ms	🟩 100 ms
Lighthouse	Uses efficient cache policy on static assets	🟨 28 resources found	🟨 28 resources found
Lighthouse	Network Round Trip Times	🟩 60 ms	🟩 90 ms
Lighthouse	Initial server response time was short	🟩 Root document took 410 ms	🟩 Root document took 440 ms
Lighthouse	Reduce unused CSS	🟥 Potential savings of 66 KiB	🟥 Potential savings of 66 KiB

572 other audits are unchanged.

[matejchalk]

Why not keep the defaults? The only differences will be that a report.md will also be created and the files will be in .code-pushup, neither of which meaningfully affect the test. Omitting irrelevant options would make the fixture more minimal.

[matejchalk]

This will fail as soon as a new vulnerability is reported. Using expect.closeTo doesn't solve anything here, because the values are integers, so the difference won't be less than 10 decimals.

A more future proof assertion would be something like expect(audit.value).toBeGreaterThanOrEqual(7), but that doesn't have an inline variant, so you'll need to break this up into multiple assertions.

[vmasek]

I uderstood the assertion purpose differently, thanks dor explanation

[matejchalk]

I don't think the categories need to be included here. The test only asserts the slugs are in the resulting report, but that's not the responsibility of this plugin.

[matejchalk]

How about checking for one of the expected vulnerabilities?

Suggested change

	issues: expect.any(Array),
	issues: expect.arrayContaining([
	{
	"message": "`express`'s dependency `connect` has a **high** vulnerability in versions **<=2.30.2**. Fix available: Update `express` to version **4.21.2** (breaking change). More information: [methodOverride Middleware Reflected Cross-Site Scripting in connect](https://github.com/advisories/GHSA-3fw8-66wf-pr7m)",
	"severity": "error"
	},
	]),

Might require a regex for the message though, since the suggested 4.21.2 update is probably just the latest express version 🤔

[matejchalk]

Only the new version will change over time, otherwise you could check the full message 🤔

Suggested change

	/Package \[`express`].*\*\*major\*\* update from \*\*\d+\.\d+\.\d+\*\* to \*\*\d+\.\d+\.\d+\*\*/,
	/^Package \[`express`]\(http:\/\/expressjs\.com\) requires a \*\*major\*\* update from \*\*3.0.0\*\* to \*\*\d+\.\d+\.\d+\*\*\.$/,

[matejchalk]

Since there aren't any other issues or issue fields than the ones you mention, you can omit the expect.arrayContaining and expect.objectContaining wrappers.

Suggested change

	issues: expect.arrayContaining([
	expect.objectContaining({
	message: expect.stringMatching(
	/Package \[`express`].*\*\*major\*\* update from \*\*\d+\.\d+\.\d+\*\* to \*\*\d+\.\d+\.\d+\*\*/,
	),
	severity: 'error',
	}),
	]),
	issues: [
	{
	message: expect.stringMatching(
	/Package \[`express`].*\*\*major\*\* update from \*\*\d+\.\d+\.\d+\*\* to \*\*\d+\.\d+\.\d+\*\*/,
	),
	severity: 'error',
	},
	],

[matejchalk]

Umm ... this doesn't assert anything 😅 Assymetrics matchers like expect.objectContaining need to be used with expect(...).toEqual(...) or expect(...).toHaveBeenCalledWith(...). On their own they don't do anything - how could they, when the value (in this case report) isn't being specified anywhere.

The assertion would need to be:

expect(report).toEqual(expect.objectContaining<Partial<Report>>({
  // ...
}));

(BTW without adding a type parameter, everything in the matcher is typed as any.)

However, you may need to break this into multiple assertions - see my other comment about replacing expect.closeTo(7) with expect(...).toBeGreaterThan(7). So it'll probably be something along these lines in the end:

const plugin = report.plugins[0]!;
expect(plugin.packageName).toBe('@code-pushup/js-packages-plugin');
expect(plugin.audits).toHaveLength(2);
expect(plugin.audits[0]).toEqual<AuditReport>({
  // ...
  value: expect.any(Number),
}));
expect(plugin.audits[0]!.value).toBeGreaterThan(7);
expect(plugin.audits[1]).toEqual<AuditReport>({
  // ...
}));

[matejchalk]

Since neither npm audit or npm outdated rely on packages actually being installed in node_modules, I'm guessing running npm install only serve to create the package-lock.json? It would be more optimal to just commit the package-lock.json in the test fixture, that we way we don't have to run npm install and the test will be faster.

[matejchalk]

This is unused.