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: nodejs#45923 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
MoLow · Jan 26, 2023 · 5a2ea89 · 5a2ea89
1 parent a27c469
commit 5a2ea89
Show file tree

Hide file tree

Showing 10 changed files with 88 additions and 61 deletions.
diff --git 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
@@ -1537,6 +1540,7 @@ added: v18.7.0
   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
@@ -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
diff --git 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
diff --git a/test/fixtures/test-runner/node_modules/reporter-cjs/package.json b/test/fixtures/test-runner/node_modules/reporter-cjs/package.json
diff --git a/test/fixtures/test-runner/node_modules/reporter-esm/index.mjs b/test/fixtures/test-runner/node_modules/reporter-esm/index.mjs
diff --git a/test/fixtures/test-runner/node_modules/reporter-esm/package.json b/test/fixtures/test-runner/node_modules/reporter-esm/package.json
diff --git 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
@@ -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+}$/,
+    );
+  });
 });