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

Conversation

ErwanRaulo
Copy link
Contributor

@ErwanRaulo ErwanRaulo commented Dec 31, 2024

TOFIX: could run tests before TS migration, now something goes wrong with childprocess and execution path.

when running nreport init, main process throws the same error :
Error: ENOENT: no such file or directory, open 'C:<yourDevDir>\report\dist\views\template.html'

process try to access views from dist folder but it does not exists, should be added to dist folder as asset in tsconfig file to include a copy when compiling typescript

no breaking changes
use childprocess to collect CLI stdout
missing limit case tests
fix NodeSecure#395
@@ -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.

}

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.

Comment on lines +65 to +76
let actualLines: string[] = [];

try {
actualLines = await filterProcessStdout(options, byMessage);
}
catch (error) {
console.log(error);

assert.fail("Execute command should not throw");
}

assert.deepEqual(actualLines, expectedLines, "we are expecting these lines");
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
let actualLines: string[] = [];
try {
actualLines = await filterProcessStdout(options, byMessage);
}
catch (error) {
console.log(error);
assert.fail("Execute command should not throw");
}
assert.deepEqual(actualLines, expectedLines, "we are expecting these lines");
const actualLines = await filterProcessStdout(options, byMessage);
assert.deepEqual(actualLines, expectedLines, "we are expecting these lines");

Copy link
Member

Choose a reason for hiding this comment

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

Pas forcément besoin de rendre complexe juste pour un message de failure spécifique

Comment on lines +18 to +23
const __dirname = path.dirname(fileURLToPath(import.meta.url));
const processDir = path.join(__dirname, "..", "fixtures");
const configFilePath = path.join(processDir, ".nodesecurerc");

describe("Report init command", async() => {
beforeEach(async() => await fs.unlink(configFilePath));
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants