From 1d686b73bda19dfdbc16ad23dcab30352ddfe666 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 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 | 6 +- lib/internal/modules/run_main.js | 26 ++++++++- lib/internal/modules/utils.js | 55 ------------------- 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, 90 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 97e160ae09eebd..345c2fb6fd0153 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, or a value provided for [`--import`][]. + ### Multiple reporters The [`--test-reporter`][] flag can be specified multiple times to report test @@ -1581,6 +1584,7 @@ added: aborted. [TAP]: https://testanything.org/ +[`--import`]: cli.md#--importmodule [`--test-name-pattern`]: cli.md#--test-name-pattern [`--test-only`]: cli.md#--test-only [`--test-reporter-destination`]: cli.md#--test-reporter-destination diff --git a/lib/internal/modules/run_main.js b/lib/internal/modules/run_main.js index c5c5331055a8dd..c948eaf4ae4437 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -2,10 +2,11 @@ const { ObjectCreate, + StringPrototypeEndsWith, } = primordials; + const { getOptionValue } = require('internal/options'); const path = require('path'); -const { shouldUseESMLoader } = require('internal/modules/utils'); function resolveMainPath(main) { // Note extension resolution for the main entry point can be deprecated in a @@ -23,6 +24,29 @@ 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'); + /** + * @type {string[]} userImports A list of preloaded modules registered by the user + * (or an empty list when none have been registered). + */ + const userImports = getOptionValue('--import'); + if (userLoaders.length > 0 || userImports.length > 0) + return true; + const { readPackageScope } = require('internal/modules/cjs/loader'); + // 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 7c943289118119..00000000000000 --- a/lib/internal/modules/utils.js +++ /dev/null @@ -1,55 +0,0 @@ -'use strict'; - -const { - ObjectCreate, - StringPrototypeEndsWith, -} = primordials; -const { getOptionValue } = require('internal/options'); - - -function shouldUseESMLoader(filePath) { - /** - * @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'); - /** - * @type {string[]} userImports A list of preloaded modules registered by the user - * (or an empty list when none have been registered). - */ - const userImports = getOptionValue('--import'); - if (userLoaders.length > 0 || userImports.length > 0) - return true; - // Determine the module format of the main - if (filePath && StringPrototypeEndsWith(filePath, '.mjs')) - return true; - if (!filePath || StringPrototypeEndsWith(filePath, '.cjs')) - return false; - const { readPackageScope } = require('internal/modules/cjs/loader'); - const pkg = readPackageScope(filePath); - return 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 44f850915a2b9d..693fa9efb4111b 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -53,7 +53,6 @@ const expectedModules = new Set([ 'NativeModule internal/idna', 'NativeModule internal/linkedlist', 'NativeModule internal/modules/cjs/loader', - 'NativeModule internal/modules/utils', 'NativeModule internal/modules/esm/utils', 'NativeModule internal/modules/helpers', 'NativeModule internal/modules/package_json_reader', diff --git a/test/parallel/test-runner-reporters.js b/test/parallel/test-runner-reporters.js index b649fe529629f2..c8440b9d60d649 100644 --- a/test/parallel/test-runner-reporters.js +++ b/test/parallel/test-runner-reporters.js @@ -86,7 +86,7 @@ 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(), ''); const stdout = child.stdout.toString(); @@ -94,4 +94,26 @@ describe('node:test reporters', { concurrency: true }, () => { assert.strictEqual(stdout.slice(0, filename.length + 2), `${filename} {`); }); }); + + 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+}$/, + ); + }); });