From 3a3a6d87f1d241084344ca5886c9144cc3d0f7f0 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. Backport-PR-URL: https://github.com/nodejs/node/pull/46361 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 | 28 +++++++++- lib/internal/modules/utils.js | 54 ------------------- lib/internal/test_runner/utils.js | 14 ++++- .../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, 91 insertions(+), 60 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 e5dc867882dbde..0bfe7b11241416 100644 --- a/lib/internal/modules/run_main.js +++ b/lib/internal/modules/run_main.js @@ -1,8 +1,11 @@ 'use strict'; +const { + 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 @@ -20,6 +23,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 1e2d0b3edcf499..00000000000000 --- a/lib/internal/modules/utils.js +++ /dev/null @@ -1,54 +0,0 @@ -'use strict'; - -const { - 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, { __proto__: 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..b9c9b4b677fef4 100644 --- a/lib/internal/test_runner/utils.js +++ b/lib/internal/test_runner/utils.js @@ -9,9 +9,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 +103,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, { __proto__: 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 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+}$/, + ); + }); });