From 403df210aca8849779304eb81fa4b156cc17d163 Mon Sep 17 00:00:00 2001 From: Geoffrey Booth Date: Thu, 22 Dec 2022 13:40:32 -0800 Subject: [PATCH] module: move test reporter loading Move the logic for handling --test-reporter out of the general module loader and into the test_runner subsystem. PR-URL: https://github.com/nodejs/node/pull/45923 Backport-PR-URL: https://github.com/nodejs/node/pull/46839 Reviewed-By: Moshe Atlow Reviewed-By: Benjamin Gruenbaum Reviewed-By: Colin Ihrig Reviewed-By: Jacob Smith Reviewed-By: Antoine du Hamel --- doc/api/test.md | 5 +- lib/internal/modules/run_main.js | 25 ++++++++- lib/internal/modules/utils.js | 54 ------------------- lib/internal/test_runner/utils.js | 15 +++++- .../node_modules/reporter-cjs/index.js | 8 +++ .../node_modules/reporter-cjs/package.json | 4 ++ .../node_modules/reporter-esm/index.mjs | 8 +++ .../node_modules/reporter-esm/package.json | 4 ++ test/parallel/test-bootstrap-modules.js | 1 - test/parallel/test-runner-reporters.js | 24 ++++++++- 10 files changed, 87 insertions(+), 61 deletions(-) delete mode 100644 lib/internal/modules/utils.js create mode 100644 test/fixtures/test-runner/node_modules/reporter-cjs/index.js create mode 100644 test/fixtures/test-runner/node_modules/reporter-cjs/package.json create mode 100644 test/fixtures/test-runner/node_modules/reporter-esm/index.mjs create mode 100644 test/fixtures/test-runner/node_modules/reporter-esm/package.json diff --git a/doc/api/test.md b/doc/api/test.md index 2f84d89c5a4043..464c73b292c8ec 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -470,7 +470,7 @@ The following built-reporters are supported: The `spec` reporter outputs the test results in a human-readable format. * `dot` - The `dot` reporter outputs the test results in a comact format, + The `dot` reporter outputs the test results in a compact format, where each passing test is represented by a `.`, and each failing test is represented by a `X`. @@ -591,6 +591,9 @@ module.exports = async function * customReporter(source) { }; ``` +The value provided to `--test-reporter` should be a string like one used in an +`import()` in JavaScript code. + ### Multiple reporters The [`--test-reporter`][] flag can be specified multiple times to report test diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index a9ec80b4c92adc..5a50d5d6afab6e 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -2,12 +2,12 @@ const { ObjectCreate, + StringPrototypeEndsWith, } = primordials; const CJSLoader = require('internal/modules/cjs/loader'); -const { Module, toRealPath } = CJSLoader; +const { Module, toRealPath, readPackageScope } = CJSLoader; const { getOptionValue } = require('internal/options'); const path = require('path'); -const { shouldUseESMLoader } = require('internal/modules/utils'); const { handleProcessExit, } = require('internal/modules/esm/handle_process_exit'); @@ -27,6 +27,27 @@ function resolveMainPath(main) { return mainPath; } +function shouldUseESMLoader(mainPath) { + /** + * @type {string[]} userLoaders A list of custom loaders registered by the user + * (or an empty list when none have been registered). + */ + const userLoaders = getOptionValue('--experimental-loader'); + if (userLoaders.length > 0) + return true; + const esModuleSpecifierResolution = + getOptionValue('--experimental-specifier-resolution'); + if (esModuleSpecifierResolution === 'node') + return true; + // Determine the module format of the main + if (mainPath && StringPrototypeEndsWith(mainPath, '.mjs')) + return true; + if (!mainPath || StringPrototypeEndsWith(mainPath, '.cjs')) + return false; + const pkg = readPackageScope(mainPath); + return pkg && pkg.data.type === 'module'; +} + function runMainESM(mainPath) { const { loadESM } = require('internal/process/esm_loader'); const { pathToFileURL } = require('internal/url'); diff --git a/lib/internal/modules/utils.js b/lib/internal/modules/utils.js deleted file mode 100644 index d2ba0a352f7571..00000000000000 --- a/lib/internal/modules/utils.js +++ /dev/null @@ -1,54 +0,0 @@ -'use strict'; - -const { - ObjectCreate, - StringPrototypeEndsWith, -} = primordials; -const { getOptionValue } = require('internal/options'); - - -function shouldUseESMLoader(mainPath) { - /** - * @type {string[]} userLoaders A list of custom loaders registered by the user - * (or an empty list when none have been registered). - */ - const userLoaders = getOptionValue('--experimental-loader'); - if (userLoaders.length > 0) - return true; - const esModuleSpecifierResolution = - getOptionValue('--experimental-specifier-resolution'); - if (esModuleSpecifierResolution === 'node') - return true; - // Determine the module format of the main - if (mainPath && StringPrototypeEndsWith(mainPath, '.mjs')) - return true; - if (!mainPath || StringPrototypeEndsWith(mainPath, '.cjs')) - return false; - const { readPackageScope } = require('internal/modules/cjs/loader'); - const pkg = readPackageScope(mainPath); - return pkg && pkg.data.type === 'module'; -} - -/** - * @param {string} filePath - * @returns {any} - * requireOrImport imports a module if the file is an ES module, otherwise it requires it. - */ -function requireOrImport(filePath) { - const useESMLoader = shouldUseESMLoader(filePath); - if (useESMLoader) { - const { esmLoader } = require('internal/process/esm_loader'); - const { pathToFileURL } = require('internal/url'); - const { isAbsolute } = require('path'); - const file = isAbsolute(filePath) ? pathToFileURL(filePath).href : filePath; - return esmLoader.import(file, undefined, ObjectCreate(null)); - } - const { Module } = require('internal/modules/cjs/loader'); - - return new Module._load(filePath, null, false); -} - -module.exports = { - shouldUseESMLoader, - requireOrImport, -}; diff --git a/lib/internal/test_runner/utils.js b/lib/internal/test_runner/utils.js index 9dba00de25719e..3fc99ce37cc33a 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -1,6 +1,7 @@ 'use strict'; const { ArrayPrototypePush, + ObjectCreate, ObjectGetOwnPropertyDescriptor, SafePromiseAllReturnArrayLike, RegExp, @@ -9,9 +10,9 @@ const { } = primordials; const { basename } = require('path'); const { createWriteStream } = require('fs'); +const { pathToFileURL } = require('internal/url'); const { createDeferredPromise } = require('internal/util'); const { getOptionValue } = require('internal/options'); -const { requireOrImport } = require('internal/modules/utils'); const { codes: { @@ -103,7 +104,17 @@ const kDefaultDestination = 'stdout'; async function getReportersMap(reporters, destinations) { return SafePromiseAllReturnArrayLike(reporters, async (name, i) => { const destination = kBuiltinDestinations.get(destinations[i]) ?? createWriteStream(destinations[i]); - let reporter = await requireOrImport(kBuiltinReporters.get(name) ?? name); + + // Load the test reporter passed to --test-reporter + const reporterSpecifier = kBuiltinReporters.get(name) ?? name; + let parentURL; + try { + parentURL = pathToFileURL(process.cwd() + '/').href; + } catch { + parentURL = 'file:///'; + } + const { esmLoader } = require('internal/process/esm_loader'); + let reporter = await esmLoader.import(reporterSpecifier, parentURL, ObjectCreate(null)); if (reporter?.default) { reporter = reporter.default; diff --git a/test/fixtures/test-runner/node_modules/reporter-cjs/index.js b/test/fixtures/test-runner/node_modules/reporter-cjs/index.js new file mode 100644 index 00000000000000..d99cd29926e86e --- /dev/null +++ b/test/fixtures/test-runner/node_modules/reporter-cjs/index.js @@ -0,0 +1,8 @@ +module.exports = async function * customReporter(source) { + const counters = {}; + for await (const event of source) { + counters[event.type] = (counters[event.type] ?? 0) + 1; + } + yield "package: reporter-cjs"; + yield JSON.stringify(counters); +}; diff --git a/test/fixtures/test-runner/node_modules/reporter-cjs/package.json b/test/fixtures/test-runner/node_modules/reporter-cjs/package.json new file mode 100644 index 00000000000000..cf7db2b7eca767 --- /dev/null +++ b/test/fixtures/test-runner/node_modules/reporter-cjs/package.json @@ -0,0 +1,4 @@ +{ + "name": "reporter-cjs", + "main": "index.js" +} diff --git a/test/fixtures/test-runner/node_modules/reporter-esm/index.mjs b/test/fixtures/test-runner/node_modules/reporter-esm/index.mjs new file mode 100644 index 00000000000000..0eb82dfe4502d8 --- /dev/null +++ b/test/fixtures/test-runner/node_modules/reporter-esm/index.mjs @@ -0,0 +1,8 @@ +export default async function * customReporter(source) { + const counters = {}; + for await (const event of source) { + counters[event.type] = (counters[event.type] ?? 0) + 1; + } + yield "package: reporter-esm"; + yield JSON.stringify(counters); +}; diff --git a/test/fixtures/test-runner/node_modules/reporter-esm/package.json b/test/fixtures/test-runner/node_modules/reporter-esm/package.json new file mode 100644 index 00000000000000..60d6b3a97fd186 --- /dev/null +++ b/test/fixtures/test-runner/node_modules/reporter-esm/package.json @@ -0,0 +1,4 @@ +{ + "name": "reporter-esm", + "exports": "./index.mjs" +} diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index 73dce999a5c351..73df0ab053b533 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -75,7 +75,6 @@ const expectedModules = new Set([ 'NativeModule internal/mime', 'NativeModule internal/modules/cjs/helpers', 'NativeModule internal/modules/cjs/loader', - 'NativeModule internal/modules/utils', 'NativeModule internal/modules/esm/assert', 'NativeModule internal/modules/esm/create_dynamic_module', 'NativeModule internal/modules/esm/fetch_module', diff --git a/test/parallel/test-runner-reporters.js b/test/parallel/test-runner-reporters.js index 74cae3401e2843..671b6ac4432167 100644 --- a/test/parallel/test-runner-reporters.js +++ b/test/parallel/test-runner-reporters.js @@ -86,10 +86,32 @@ describe('node:test reporters', { concurrency: true }, () => { it(`should support a '${ext}' file as a custom reporter`, async () => { const filename = `custom.${ext}`; const child = spawnSync(process.execPath, - ['--test', '--test-reporter', fixtures.path('test-runner/custom_reporters/', filename), + ['--test', '--test-reporter', fixtures.fileURL('test-runner/custom_reporters/', filename), testFile]); assert.strictEqual(child.stderr.toString(), ''); assert.strictEqual(child.stdout.toString(), `${filename} {"test:start":5,"test:pass":2,"test:fail":3,"test:plan":3,"test:diagnostic":7}`); }); }); + + it('should support a custom reporter from node_modules', async () => { + const child = spawnSync(process.execPath, + ['--test', '--test-reporter', 'reporter-cjs', 'reporters.js'], + { cwd: fixtures.path('test-runner') }); + assert.strictEqual(child.stderr.toString(), ''); + assert.match( + child.stdout.toString(), + /^package: reporter-cjs{"test:start":5,"test:pass":2,"test:fail":3,"test:plan":3,"test:diagnostic":\d+}$/, + ); + }); + + it('should support a custom ESM reporter from node_modules', async () => { + const child = spawnSync(process.execPath, + ['--test', '--test-reporter', 'reporter-esm', 'reporters.js'], + { cwd: fixtures.path('test-runner') }); + assert.strictEqual(child.stderr.toString(), ''); + assert.match( + child.stdout.toString(), + /^package: reporter-esm{"test:start":5,"test:pass":2,"test:fail":3,"test:plan":3,"test:diagnostic":\d+}$/, + ); + }); });