Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions e2e/plugin-js-packages-e2e/eslint.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import tseslint from 'typescript-eslint';
import baseConfig from '../../eslint.config.js';

export default tseslint.config(...baseConfig, {
files: ['**/*.ts'],
languageOptions: {
parserOptions: {
projectService: true,
tsconfigRootDir: import.meta.dirname,
},
},
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import jsPackagesPlugin from '@code-pushup/js-packages-plugin';
import type { CoreConfig } from '@code-pushup/models';

export default {
persist: {
outputDir: './',
filename: 'report',
format: ['json'],
},
Comment on lines +5 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

plugins: [await jsPackagesPlugin({ packageManager: 'npm' })],
categories: [
{
slug: 'security',
title: 'Security',
refs: [
{ type: 'group', plugin: 'js-packages', slug: 'npm-audit', weight: 1 },
],
},
{
slug: 'updates',
title: 'Updates',
refs: [
{
type: 'group',
plugin: 'js-packages',
slug: 'npm-outdated',
weight: 1,
},
],
},
],
Comment on lines +11 to +31
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} satisfies CoreConfig;
10 changes: 10 additions & 0 deletions e2e/plugin-js-packages-e2e/mocks/fixtures/npm-repo/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"dependencies": {
"express": "3.0.0"
},
"devDependencies": {
"@code-pushup/cli": "latest",
"@code-pushup/js-packages-plugin": "latest",
"@code-pushup/models": "latest"
}
}
23 changes: 23 additions & 0 deletions e2e/plugin-js-packages-e2e/project.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{
"name": "plugin-js-packages-e2e",
"$schema": "../../node_modules/nx/schemas/project-schema.json",
"projectType": "application",
"sourceRoot": "e2e/plugin-js-packages-e2e/src",
"implicitDependencies": ["cli", "plugin-js-packages"],
"targets": {
"lint": {
"executor": "@nx/linter:eslint",
"outputs": ["{options.outputFile}"],
"options": {
"lintFilePatterns": ["e2e/plugin-eslint-e2e/**/*.ts"]
}
},
"e2e": {
"executor": "@nx/vite:test",
"options": {
"configFile": "e2e/plugin-eslint-e2e/vite.config.e2e.ts"
}
}
},
"tags": ["scope:plugin", "type:e2e"]
}
99 changes: 99 additions & 0 deletions e2e/plugin-js-packages-e2e/tests/plugin-js-packages.e2e.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { cp } from 'node:fs/promises';
import path from 'node:path';
import { afterAll, beforeAll, expect, it } from 'vitest';
import { type Report, reportSchema } from '@code-pushup/models';
import { nxTargetProject } from '@code-pushup/test-nx-utils';
import { teardownTestFolder } from '@code-pushup/test-setup';
import {
E2E_ENVIRONMENTS_DIR,
TEST_OUTPUT_DIR,
omitVariableReportData,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused.

} from '@code-pushup/test-utils';
import { executeProcess, readJsonFile } from '@code-pushup/utils';

describe('plugin-js-packages', () => {
const fixturesDir = path.join(
'e2e',
'plugin-js-packages-e2e',
'mocks',
'fixtures',
);
const fixturesNPMDir = path.join(fixturesDir, 'npm-repo');

const envRoot = path.join(
E2E_ENVIRONMENTS_DIR,
nxTargetProject(),
TEST_OUTPUT_DIR,
);
const npmRepoDir = path.join(envRoot, 'npm-repo');

beforeAll(async () => {
await cp(fixturesNPMDir, npmRepoDir, { recursive: true });
});

afterAll(async () => {
await teardownTestFolder(npmRepoDir);
});

it('should run JS packages plugin for NPM and create report.json', async () => {
const { code: installCode } = await executeProcess({
command: 'npm',
args: ['install'],
cwd: npmRepoDir,
});

expect(installCode).toBe(0);
Comment on lines +39 to +45
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const { code, stderr } = await executeProcess({
command: 'npx',
args: ['@code-pushup/cli', 'collect', '--no-progress'],
cwd: npmRepoDir,
});

expect(code).toBe(0);
expect(stderr).toBe('');

const report = await readJsonFile<Report>(
path.join(npmRepoDir, 'report.json'),
);

expect(() => reportSchema.parse(report)).not.toThrow();
expect.objectContaining({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>({
  // ...
}));

categories: expect.arrayContaining([
expect.objectContaining({ slug: 'security' }),
expect.objectContaining({ slug: 'updates' }),
]),
plugins: expect.arrayContaining([
expect.objectContaining({
packageName: '@code-pushup/js-packages-plugin',
audits: expect.arrayContaining([
expect.objectContaining({
slug: 'npm-audit-prod',
displayValue: expect.stringMatching(/\d vulnerabilities/),
value: expect.closeTo(7, 10), // there are 7 vulnerabilities (6 high, 1 low) at the time
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I uderstood the assertion purpose differently, thanks dor explanation

details: expect.objectContaining({
issues: expect.any(Array),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

}),
}),
expect.objectContaining({
slug: 'npm-outdated-prod',
displayValue: '1 major outdated package version',
value: 1,
score: 0,
details: {
issues: expect.arrayContaining([
expect.objectContaining({
message: expect.stringMatching(
/Package \[`express`].*\*\*major\*\* update from \*\*\d+\.\d+\.\d+\*\* to \*\*\d+\.\d+\.\d+\*\*/,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+\*\*\.$/,

),
severity: 'error',
}),
]),
Comment on lines +84 to +91
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
},
],

},
}),
]),
}),
]),
});
});
});
20 changes: 20 additions & 0 deletions e2e/plugin-js-packages-e2e/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"module": "ESNext",
"forceConsistentCasingInFileNames": true,
"strict": true,
"noImplicitOverride": true,
"noPropertyAccessFromIndexSignature": true,
"noImplicitReturns": true,
"noFallthroughCasesInSwitch": true,
"types": ["vitest"]
},
"files": [],
"include": [],
"references": [
{
"path": "./tsconfig.test.json"
}
]
}
15 changes: 15 additions & 0 deletions e2e/plugin-js-packages-e2e/tsconfig.test.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"extends": "./tsconfig.json",
"compilerOptions": {
"outDir": "../../dist/out-tsc",
"types": ["vitest/globals", "vitest/importMeta", "vite/client", "node"],
"target": "ES2020"
},
"exclude": ["__test-env__/**"],
"include": [
"vite.config.e2e.ts",
"tests/**/*.e2e.test.ts",
"tests/**/*.d.ts",
"mocks/**/*.ts"
]
}
21 changes: 21 additions & 0 deletions e2e/plugin-js-packages-e2e/vite.config.e2e.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/// <reference types="vitest" />
import { defineConfig } from 'vite';
import { tsconfigPathAliases } from '../../tools/vitest-tsconfig-path-aliases.js';

export default defineConfig({
cacheDir: '../../node_modules/.vite/plugin-js-packages-e2e',
test: {
reporters: ['basic'],
testTimeout: 120_000,
globals: true,
alias: tsconfigPathAliases(),
pool: 'threads',
poolOptions: { threads: { singleThread: true } },
cache: {
dir: '../../node_modules/.vitest',
},
environment: 'node',
include: ['tests/**/*.e2e.test.{js,mjs,cjs,ts,mts,cts,jsx,tsx}'],
setupFiles: ['../../testing/test-setup/src/lib/reset.mocks.ts'],
},
});
Loading