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(e2e): add nominal tests for exec & init CLI commands #434

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
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
6 changes: 5 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -65,5 +65,9 @@ json/
reports/
preview/
dist/
.nodesecurerc
/.nodesecurerc
.DS_Store

# IDE
.vscode
jsconfig.json
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"lint": "eslint src test bin scripts",
"test-only": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.spec.ts\"",
"test": "c8 --all --src ./src -r html npm run test-only",
"test:e2e": "glob -c \"tsx --test-reporter=spec --test\" \"./test/commands/*.spec.ts\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think functional tests are not executed during development as we expect quick feedback from small unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to have a separate script for e2e tests, however, the glob from existing test-only script still match e2e tests files.
I think it's simpler to handle this via file names, for instance:

{
  "test-only": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.spec.ts\"",
  "test:e2e": "glob -c \"tsx --test-reporter=spec --test\" \"./test/**/*.e2e-spec.ts\""
}

"preview:light": "tsx --no-warnings ./scripts/preview.js --theme light",
"preview:dark": "tsx --no-warnings ./scripts/preview.js --theme dark",
"prepublishOnly": "npm run build"
Expand Down
68 changes: 68 additions & 0 deletions test/commands/execute.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import dotenv from "dotenv";
dotenv.config();
Comment on lines +1 to +2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import dotenv from "dotenv";
dotenv.config();

Require dotenv in the script instead ? This way we don't need to import it in multiple files
"test:e2e": "glob -c \"tsx -r dotenv/config --test-reporter=spec --test\" \"./test/commands/*.spec.ts\""


// Import Node.js Dependencies
import { fileURLToPath } from "node:url";
import path from "node:path";
import fs from "node:fs/promises";
import { afterEach, describe, it } from "node:test";
import assert from "node:assert";

// Import Third-party Dependencies
import stripAnsi from "strip-ansi";

// Import Internal Dependencies
import { filterProcessStdout } from "../helpers/reportCommandRunner.js";
import * as CONSTANTS from "../../src/constants.js";

// CONSTANTS
const __dirname = path.dirname(fileURLToPath(import.meta.url));
const processDir = path.join(__dirname, "../..");
PierreDemailly marked this conversation as resolved.
Show resolved Hide resolved

describe("Report execute command", async() => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe("Report execute command", async() => {
describe("Report execute command", () => {

afterEach(async() => await fs.rm(CONSTANTS.DIRS.CLONES, {
recursive: true, force: true
}));

it("should execute command on fixture '.nodesecurerc'", async() => {
const options = {
cmd: "node",
args: ["dist/bin/index.js", "execute"],
cwd: processDir
};

function byMessage(buffer) {
const message = `.*`;
const afterNonAlphaNum = String.raw`?<=[^a-zA-Z\d\s:]\s`;
const beforeTime = String.raw`?=\s\d{1,5}.\d{1,4}ms`;
const withoutDuplicates = String.raw`(?![\s\S]*\1)`;

const matchMessage = `(${afterNonAlphaNum})(${message})(${beforeTime})|(${afterNonAlphaNum})(${message})`;
const reg = new RegExp(`(${matchMessage})${withoutDuplicates}`, "g");

const matchedMessages = stripAnsi(buffer.toString()).match(reg);

return matchedMessages ?? [""];
}

const expectedLines = [
"Executing nreport at: C:\\PERSO\\dev\\report",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Executing nreport at: C:\\PERSO\\dev\\report",
`Executing nreport at: ${kProcessDir}`,

"title: Default report title",
"reporters: html,pdf",
"[Fetcher: NPM] - Fetching NPM packages metadata on the NPM Registry",
"",
"[Fetcher: NPM] - successfully executed in",
"[Fetcher: GIT] - Cloning GIT repositories",
"[Fetcher: GIT] - Fetching repositories metadata on the NPM Registry",
"[Fetcher: GIT] - successfully executed in",
"[Reporter: HTML] - Building template and assets",
"[Reporter: HTML] - successfully executed in",
"[Reporter: PDF] - Using puppeteer to convert HTML content to PDF",
"[Reporter: PDF] - successfully executed in",
"Security report successfully generated! Enjoy 🚀."
];

const actualLines = await filterProcessStdout(options, byMessage);
assert.deepEqual(actualLines, expectedLines, "we are expecting these lines");
});
});
48 changes: 48 additions & 0 deletions test/commands/initialize.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import dotenv from "dotenv";
dotenv.config();

Comment on lines +1 to +3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import dotenv from "dotenv";
dotenv.config();

Same as above

// Import Node.js Dependencies
import { fileURLToPath } from "node:url";
import fs from "node:fs/promises";
import path from "node:path";
import { beforeEach, describe, it } from "node:test";
import assert from "node:assert";

// Import Third-party Dependencies
import stripAnsi from "strip-ansi";

// Import Internal Dependencies
import { runProcess } from "../helpers/reportCommandRunner.js";

// CONSTANTS
const __dirname = path.dirname(fileURLToPath(import.meta.url));
const processDir = path.join(__dirname, "..", "fixtures");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const processDir = path.join(__dirname, "..", "fixtures");
const kProcessDir = path.join(__dirname, "..", "fixtures");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder why it's prefixed by k ?

Copy link
Member

Choose a reason for hiding this comment

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

This is a convention for all the constants in all our projects: file-scoped constants should be prefixed with k, exported constants should be named using CONSTANT_CASE/UPPER_CASE, and other constants should be named using camelCase :)

AFAIK this convention comes from the Google C++ style guide used in Node.js (it's more used for Symbols tho)
Examples:
https://github.com/nodejs/node/blob/main/lib/events.js#L62
https://github.com/nodejs/node/blob/main/lib/events.js#L87
Another example at NodeSecure:
https://github.com/NodeSecure/cli/blob/master/src/http-server/cache.js#L13

const configFilePath = path.join(processDir, ".nodesecurerc");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const configFilePath = path.join(processDir, ".nodesecurerc");
const kConfigFilePath = path.join(processDir, ".nodesecurerc");


describe("Report init command", async() => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
describe("Report init command", async() => {
describe("Report init command", () => {

beforeEach(async() => await fs.unlink(configFilePath));
Comment on lines +18 to +23
Copy link
Member

Choose a reason for hiding this comment

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

Il faudrait plutôt créer la configuration dans le path temp pour ne pas avoir de problèmes avec une modification non-prévu du fichier dans /fixtures

it("should create config if not exists", async() => {
const lines = [
/.*/,
/ > Executing nreport at: .*$/,
/.*/,
/Successfully generated NodeSecure runtime configuration at current location/,
/.*/
];

const processOptions = {
cmd: "node",
args: ["dist/bin/index.js", "initialize"],
cwd: processDir
};

for await (const line of runProcess(processOptions)) {
const regexp = lines.shift();
assert.ok(regexp, "we are expecting this line");
assert.ok(regexp.test(stripAnsi(line)), `line (${line}) matches ${regexp}`);
}

// to avoid false positive if no lines have been emitted from process
assert.equal(lines.length, 0);
});
});
42 changes: 42 additions & 0 deletions test/fixtures/.nodesecurerc
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
{
"version": "1.0.0",
"i18n": "english",
"strategy": "github-advisory",
"registry": "https://registry.npmjs.org",
"report": {
"theme": "light",
"includeTransitiveInternal": false,
"reporters": [
"html",
"pdf"
],
"charts": [
{
"name": "Extensions",
"display": true,
"interpolation": "d3.interpolateRainbow",
"type": "bar"
},
{
"name": "Licenses",
"display": true,
"interpolation": "d3.interpolateCool",
"type": "bar"
},
{
"name": "Warnings",
"display": true,
"type": "horizontalBar",
"interpolation": "d3.interpolateInferno"
},
{
"name": "Flags",
"display": true,
"type": "horizontalBar",
"interpolation": "d3.interpolateSinebow"
}
],
"title": "Default report title",
"showFlags": true
}
}
54 changes: 54 additions & 0 deletions test/helpers/reportCommandRunner.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Import Node.js Dependencies
import { ChildProcess, spawn } from "node:child_process";
import { createInterface } from "node:readline";

// Import Third-party Dependencies
import stripAnsi from "strip-ansi";

export async function* runProcess(options) {
const childProcess = spawnedProcess(options);
try {
if (!childProcess.stdout) {
return;
}

const rStream = createInterface(childProcess.stdout);

for await (const line of rStream) {
yield stripAnsi(line);
}
}
finally {
childProcess.kill();
}
}

export function filterProcessStdout(options, filter): Promise<string[]> {
return new Promise((resolve, reject) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

childprocess was turned into promise in order to collect filtered stdout outputs and emit them all at the end.

const childProcess = spawnedProcess(options);
const output = new Set<string>();

childProcess.stdout?.on("data", (buffer) => {
filter(buffer).forEach((filteredData) => {
output.add(filteredData);
});
});

childProcess.on("close", (code) => {

Check failure on line 37 in test/helpers/reportCommandRunner.ts

View workflow job for this annotation

GitHub Actions / test (20.x, ubuntu-latest)

'code' is defined but never used. Allowed unused args must match /^_/u

Check failure on line 37 in test/helpers/reportCommandRunner.ts

View workflow job for this annotation

GitHub Actions / test (22.x, ubuntu-latest)

'code' is defined but never used. Allowed unused args must match /^_/u
resolve(Array.from(output));
});

childProcess.on("error", (err) => {
reject(err);
});
});
}

function spawnedProcess(options): ChildProcess {
const { cmd, args = [], cwd = process.cwd() } = options;

return spawn(cmd, args, {
stdio: ["ignore", "pipe", "pipe", "ipc"],
cwd
});
}
Loading