From 5a3a32f2b52767c618d36ea3a7d81590986304a1 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Tue, 5 Nov 2024 12:16:42 +0100 Subject: [PATCH] refactor: make node-bundle tests executable using ts-jest (#32022) Make `node-bundle` easier to test (in-process instead of using a subcommand that requires `.js` to have been compiled), and fix a bug in the tests that used `--license` instead of `--allowed-license` (configure `yargs` to be `strict`). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- tools/@aws-cdk/node-bundle/package.json | 1 + tools/@aws-cdk/node-bundle/src/api/bundle.ts | 2 +- tools/@aws-cdk/node-bundle/src/cli-main.ts | 115 +++++++++++++++++++ tools/@aws-cdk/node-bundle/src/cli.ts | 105 +---------------- tools/@aws-cdk/node-bundle/test/cli.test.ts | 79 +++++++------ tools/@aws-cdk/node-bundle/tsconfig.json | 1 + yarn.lock | 2 +- 7 files changed, 167 insertions(+), 138 deletions(-) create mode 100644 tools/@aws-cdk/node-bundle/src/cli-main.ts diff --git a/tools/@aws-cdk/node-bundle/package.json b/tools/@aws-cdk/node-bundle/package.json index 3c3f188956568..b157c277819fa 100644 --- a/tools/@aws-cdk/node-bundle/package.json +++ b/tools/@aws-cdk/node-bundle/package.json @@ -17,6 +17,7 @@ "@types/license-checker": "^25.0.6", "@types/madge": "^5.0.3", "@types/node": "^16", + "@types/yargs": "^17", "@typescript-eslint/eslint-plugin": "^6.21.0", "@typescript-eslint/parser": "^6.21.0", "eslint": "^8", diff --git a/tools/@aws-cdk/node-bundle/src/api/bundle.ts b/tools/@aws-cdk/node-bundle/src/api/bundle.ts index 7036bf37c728e..bf8af98023b4f 100644 --- a/tools/@aws-cdk/node-bundle/src/api/bundle.ts +++ b/tools/@aws-cdk/node-bundle/src/api/bundle.ts @@ -327,7 +327,7 @@ export class Bundle { if (this.test) { const command = `${path.join(bundleDir, this.test)}`; - console.log(`Running santiy test: ${command}`); + console.log(`Running sanity test: ${command}`); shell(command, { cwd: bundleDir }); } diff --git a/tools/@aws-cdk/node-bundle/src/cli-main.ts b/tools/@aws-cdk/node-bundle/src/cli-main.ts new file mode 100644 index 0000000000000..754e03a18f99c --- /dev/null +++ b/tools/@aws-cdk/node-bundle/src/cli-main.ts @@ -0,0 +1,115 @@ +import * as path from 'path'; +import * as fs from 'fs-extra'; +import * as yargs from 'yargs'; +import { Bundle, BundleProps, BundleValidateOptions } from './api'; + +function versionNumber(): string { + return fs.readJSONSync(path.join(__dirname, '..', 'package.json')).version; +} + +export async function cliMain(cliArgs: string[]) { + const argv = await yargs + .usage('Usage: node-bundle COMMAND') + .option('entrypoint', { type: 'array', nargs: 1, desc: 'List of entrypoints to bundle' }) + .option('external', { type: 'array', nargs: 1, default: [], desc: 'Packages in this list will be excluded from the bundle and added as dependencies (example: fsevents:optional)' }) + .option('allowed-license', { type: 'array', nargs: 1, default: [], desc: 'List of valid licenses' }) + .option('resource', { type: 'array', nargs: 1, default: [], desc: 'List of resources that need to be explicitly copied to the bundle (example: node_modules/proxy-agent/contextify.js:bin/contextify.js)' }) + .option('dont-attribute', { type: 'string', desc: 'Dependencies matching this regular expressions wont be added to the notice file' }) + .option('test', { type: 'string', desc: 'Validation command to sanity test the bundle after its created' }) + .command('validate', 'Validate the package is ready for bundling', args => args + .option('fix', { type: 'boolean', default: false, alias: 'f', desc: 'Fix any fixable violations' }), + ) + .command('write', 'Write the bundled version of the project to a temp directory') + .command('pack', 'Write the bundle and create the tarball') + .demandCommand() // require a subcommand + .strict() // require a VALID subcommand, and only supported options + .fail((msg, err) => { + // Throw an error in test mode, exit with an error code otherwise + if (err) { throw err; } + if (process.env.NODE_ENV === 'test') { + throw new Error(msg); + } + console.error(msg); + process.exit(1); // exit() not exitCode, we must not return. + }) + .help() + .version(versionNumber()) + .parse(cliArgs); + + const command = argv._[0]; + + function undefinedIfEmpty(arr?: any[]): string[] | undefined { + if (!arr || arr.length === 0) return undefined; + return arr as string[]; + } + + const resources: any = {}; + for (const resource of (argv.resource as string[])) { + const parts = resource.split(':'); + resources[parts[0]] = parts[1]; + } + + const optionalExternals = []; + const runtimeExternals = []; + + for (const external of (argv.external as string[])) { + const parts = external.split(':'); + const name = parts[0]; + const type = parts[1]; + switch (type) { + case 'optional': + optionalExternals.push(name); + break; + case 'runtime': + runtimeExternals.push(name); + break; + default: + throw new Error(`Unsupported dependency type '${type}' for external package '${name}'. Supported types are: ['optional', 'runtime']`); + } + } + + const props: BundleProps = { + packageDir: process.cwd(), + entryPoints: undefinedIfEmpty(argv.entrypoint), + externals: { dependencies: runtimeExternals, optionalDependencies: optionalExternals }, + allowedLicenses: undefinedIfEmpty(argv['allowed-license']), + resources: resources, + dontAttribute: argv['dont-attribute'], + test: argv.test, + }; + + const bundle = new Bundle(props); + + switch (command) { + case 'validate': + // When using `yargs.command(command, builder [, handler])` without the handler + // as we do here, there is no typing for command-specific options. So force a cast. + const fix = argv.fix as boolean | undefined; + validate(bundle, { fix }); + break; + case 'write': + write(bundle); + break; + case 'pack': + pack(bundle); + break; + default: + throw new Error(`Unknown command: ${command}`); + } +} + +function write(bundle: Bundle) { + const bundleDir = bundle.write(); + console.log(bundleDir); +} + +function validate(bundle: Bundle, options: BundleValidateOptions = {}) { + const report = bundle.validate(options); + if (!report.success) { + throw new Error(report.summary); + } +} + +function pack(bundle: Bundle) { + bundle.pack(); +} diff --git a/tools/@aws-cdk/node-bundle/src/cli.ts b/tools/@aws-cdk/node-bundle/src/cli.ts index d286e0a78e024..f6c7ec729405e 100644 --- a/tools/@aws-cdk/node-bundle/src/cli.ts +++ b/tools/@aws-cdk/node-bundle/src/cli.ts @@ -1,107 +1,6 @@ -import * as path from 'path'; -import * as fs from 'fs-extra'; -import * as yargs from 'yargs'; -import { Bundle, BundleProps, BundleValidateOptions } from './api'; +import { cliMain } from './cli-main'; -function versionNumber(): string { - return fs.readJSONSync(path.join(__dirname, '..', 'package.json')).version; -} - -async function buildCommands() { - - const argv = yargs - .usage('Usage: node-bundle COMMAND') - .option('entrypoint', { type: 'array', nargs: 1, desc: 'List of entrypoints to bundle' }) - .option('external', { type: 'array', nargs: 1, default: [], desc: 'Packages in this list will be excluded from the bundle and added as dependencies (example: fsevents:optional)' }) - .option('allowed-license', { type: 'array', nargs: 1, default: [], desc: 'List of valid licenses' }) - .option('resource', { type: 'array', nargs: 1, default: [], desc: 'List of resources that need to be explicitly copied to the bundle (example: node_modules/proxy-agent/contextify.js:bin/contextify.js)' }) - .option('dont-attribute', { type: 'string', desc: 'Dependencies matching this regular expressions wont be added to the notice file' }) - .option('test', { type: 'string', desc: 'Validation command to sanity test the bundle after its created' }) - .command('validate', 'Validate the package is ready for bundling', args => args - .option('fix', { type: 'boolean', default: false, alias: 'f', desc: 'Fix any fixable violations' }), - ) - .command('write', 'Write the bundled version of the project to a temp directory') - .command('pack', 'Write the bundle and create the tarball') - .help() - .version(versionNumber()) - .argv; - - const command = argv._[0]; - - function undefinedIfEmpty(arr?: any[]): string[] | undefined { - if (!arr || arr.length === 0) return undefined; - return arr as string[]; - } - - const resources: any = {}; - for (const resource of (argv.resource as string[])) { - const parts = resource.split(':'); - resources[parts[0]] = parts[1]; - } - - const optionalExternals = []; - const runtimeExternals = []; - - for (const external of (argv.external as string[])) { - const parts = external.split(':'); - const name = parts[0]; - const type = parts[1]; - switch (type) { - case 'optional': - optionalExternals.push(name); - break; - case 'runtime': - runtimeExternals.push(name); - break; - default: - throw new Error(`Unsupported dependency type '${type}' for external package '${name}'. Supported types are: ['optional', 'runtime']`); - } - } - - const props: BundleProps = { - packageDir: process.cwd(), - entryPoints: undefinedIfEmpty(argv.entrypoint), - externals: { dependencies: runtimeExternals, optionalDependencies: optionalExternals }, - allowedLicenses: undefinedIfEmpty(argv['allowed-license']), - resources: resources, - dontAttribute: argv['dont-attribute'], - test: argv.test, - }; - - const bundle = new Bundle(props); - - switch (command) { - case 'validate': - validate(bundle, { fix: argv.fix }); - break; - case 'write': - write(bundle); - break; - case 'pack': - pack(bundle); - break; - default: - throw new Error(`Unknown command: ${command}`); - } -} - -function write(bundle: Bundle) { - const bundleDir = bundle.write(); - console.log(bundleDir); -} - -function validate(bundle: Bundle, options: BundleValidateOptions = {}) { - const report = bundle.validate(options); - if (!report.success) { - throw new Error(report.summary); - } -} - -function pack(bundle: Bundle) { - bundle.pack(); -} - -buildCommands() +cliMain(process.argv.slice(2)) .catch((err: Error) => { console.error(`Error: ${err.message}`); process.exitCode = 1; diff --git a/tools/@aws-cdk/node-bundle/test/cli.test.ts b/tools/@aws-cdk/node-bundle/test/cli.test.ts index 57cdc76a25df4..0e18d2936696e 100644 --- a/tools/@aws-cdk/node-bundle/test/cli.test.ts +++ b/tools/@aws-cdk/node-bundle/test/cli.test.ts @@ -1,9 +1,10 @@ import * as path from 'path'; import * as fs from 'fs-extra'; +import { cliMain } from '../src/cli-main'; import { Package } from './_package'; -import { shell } from '../src/api/_shell'; +import * as util from 'util'; -test('validate', () => { +test('validate', async () => { const pkg = Package.create({ name: 'consumer', licenses: ['Apache-2.0'], circular: true }); const dep1 = pkg.addDependency({ name: 'dep1', licenses: ['INVALID'] }); @@ -14,15 +15,14 @@ test('validate', () => { try { const command = [ - whereami(), '--entrypoint', pkg.entrypoint, '--resource', 'missing:bin/missing', - '--license', 'Apache-2.0', + '--allowed-license', 'Apache-2.0', 'validate', - ].join(' '); - shell(command, { cwd: pkg.dir, quiet: true }); + ]; + await runCliMain(pkg.dir, command); } catch (e: any) { - const violations = new Set(e.stderr.toString().trim().split('\n').filter((l: string) => l.startsWith('-'))); + const violations = new Set(e.message.trim().split('\n').filter((l: string) => l.startsWith('-'))); const expected = new Set([ `- invalid-license: Dependency ${dep1.name}@${dep1.version} has an invalid license: UNKNOWN`, `- multiple-license: Dependency ${dep2.name}@${dep2.version} has multiple licenses: Apache-2.0,MIT`, @@ -35,7 +35,7 @@ test('validate', () => { }); -test('write', () => { +test('write', async () => { const pkg = Package.create({ name: 'consumer', licenses: ['Apache-2.0'] }); pkg.addDependency({ name: 'dep1', licenses: ['MIT'] }); @@ -45,13 +45,12 @@ test('write', () => { pkg.install(); const command = [ - whereami(), '--entrypoint', pkg.entrypoint, - '--license', 'Apache-2.0', - '--license', 'MIT', + '--allowed-license', 'Apache-2.0', + '--allowed-license', 'MIT', 'write', - ].join(' '); - const bundleDir = shell(command, { cwd: pkg.dir, quiet: true }); + ]; + const bundleDir = await runCliMain(pkg.dir, command); expect(fs.existsSync(path.join(bundleDir, pkg.entrypoint))).toBeTruthy(); expect(fs.existsSync(path.join(bundleDir, 'package.json'))).toBeTruthy(); @@ -67,7 +66,7 @@ test('write', () => { }); -test('validate and fix', () => { +test('validate and fix', async () => { const pkg = Package.create({ name: 'consumer', licenses: ['Apache-2.0'] }); pkg.addDependency({ name: 'dep1', licenses: ['MIT'] }); @@ -76,33 +75,32 @@ test('validate and fix', () => { pkg.write(); pkg.install(); - const run = (sub: string) => { + const run = (sub: string[]) => { const command = [ - whereami(), '--entrypoint', pkg.entrypoint, - '--license', 'Apache-2.0', - '--license', 'MIT', - sub, - ].join(' '); - shell(command, { cwd: pkg.dir, quiet: true }); + '--allowed-license', 'Apache-2.0', + '--allowed-license', 'MIT', + ...sub, + ]; + return runCliMain(pkg.dir, command); }; try { - run('pack'); + await run(['pack']); throw new Error('Expected packing to fail before fixing'); } catch { // this should fix the fact we don't generate // the project with the correct attributions - run('validate --fix'); + await run(['validate', '--fix']); } - run('pack'); + await run(['pack']); const tarball = path.join(pkg.dir, `${pkg.name}-${pkg.version}.tgz`); expect(fs.existsSync(tarball)).toBeTruthy(); }); -test('pack', () => { +test('pack', async () => { const pkg = Package.create({ name: 'consumer', licenses: ['Apache-2.0'] }); const dep1 = pkg.addDependency({ name: 'dep1', licenses: ['MIT'] }); @@ -127,19 +125,34 @@ test('pack', () => { pkg.install(); const command = [ - whereami(), '--entrypoint', pkg.entrypoint, - '--license', 'Apache-2.0', - '--license', 'MIT', + '--allowed-license', 'Apache-2.0', + '--allowed-license', 'MIT', 'pack', - ].join(' '); - shell(command, { cwd: pkg.dir, quiet: true }); + ]; + await runCliMain(pkg.dir, command); const tarball = path.join(pkg.dir, `${pkg.name}-${pkg.version}.tgz`); expect(fs.existsSync(tarball)).toBeTruthy(); }); -function whereami() { - return path.join(path.join(__dirname, '..', 'bin', 'node-bundle')); -} +async function runCliMain(cwd: string, command: string[]): Promise { + const log: string[] = [] + const spy = jest + .spyOn(console, 'log') + .mockImplementation((...args) => { + log.push(util.format(...args)); + }); + + const curdir = process.cwd(); + process.chdir(cwd); + try { + await cliMain(command); + + return log.join('\n'); + } finally { + process.chdir(curdir); + spy.mockRestore(); + } +} \ No newline at end of file diff --git a/tools/@aws-cdk/node-bundle/tsconfig.json b/tools/@aws-cdk/node-bundle/tsconfig.json index 96cb12aa0b31d..4d10a59f0d283 100644 --- a/tools/@aws-cdk/node-bundle/tsconfig.json +++ b/tools/@aws-cdk/node-bundle/tsconfig.json @@ -25,6 +25,7 @@ "strictNullChecks": true, "strictPropertyInitialization": true, "stripInternal": true, + "noErrorTruncation": true, "target": "ES2019" }, "include": [ diff --git a/yarn.lock b/yarn.lock index adbadf5371a4b..4f083618498a2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5507,7 +5507,7 @@ dependencies: "@types/yargs-parser" "*" -"@types/yargs@^17.0.8": +"@types/yargs@^17", "@types/yargs@^17.0.8": version "17.0.33" resolved "https://registry.npmjs.org/@types/yargs/-/yargs-17.0.33.tgz#8c32303da83eec050a84b3c7ae7b9f922d13e32d" integrity sha512-WpxBCKWPLr4xSsHgz511rFJAM+wS28w2zEO1QDNY5zM/S8ok70NNfztH0xwhqKyaK0OHCbN98LDAZuy1ctxDkA==