test_runner: unify --require and --import behavior when isolation none

PR-URL: https://github.com/nodejs/node/pull/57924
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>

lib/internal/main/test_runner.js

@@ -5,8 +5,8 @@ const {
 } = primordials;
 
 const {
-  prepareMainThreadExecution,
   markBootstrapComplete,
+  prepareTestRunnerMainExecution,
 } = require('internal/process/pre_execution');
 const { isUsingInspector } = require('internal/util/inspector');
 const { run } = require('internal/test_runner/runner');
@@ -16,10 +16,12 @@ let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => {
   debug = fn;
 });
 
-prepareMainThreadExecution(false);
-markBootstrapComplete();
-
 const options = parseCommandLine();
+const isTestIsolationDisabled = options.isolation === 'none';
+// We set initializeModules to false as we want to load user modules in the test runner run function
+// if we are running with --test-isolation=none
+prepareTestRunnerMainExecution(!isTestIsolationDisabled);
+markBootstrapComplete();
 
 if (isUsingInspector() && options.isolation === 'process') {
   process.emitWarning('Using the inspector with --test forces running at a concurrency of 1. ' +

lib/internal/process/pre_execution.js

@@ -50,6 +50,15 @@ function prepareMainThreadExecution(expandArgv1 = false, initializeModules = tru
   });
 }
 
+function prepareTestRunnerMainExecution(loadUserModules = true) {
+  return prepareExecution({
+    expandArgv1: false,
+    initializeModules: true,
+    isMainThread: true,
+    forceDefaultLoader: !loadUserModules,
+  });
+}
+
 function prepareWorkerThreadExecution() {
   prepareExecution({
     expandArgv1: false,
@@ -87,7 +96,7 @@ function prepareShadowRealmExecution() {
 }
 
 function prepareExecution(options) {
-  const { expandArgv1, initializeModules, isMainThread } = options;
+  const { expandArgv1, initializeModules, isMainThread, forceDefaultLoader } = options;
 
   refreshRuntimeOptions();
 
@@ -147,7 +156,7 @@ function prepareExecution(options) {
   }
 
   if (initializeModules) {
-    setupUserModules();
+    setupUserModules(forceDefaultLoader);
   }
 
   return mainEntry;
@@ -712,6 +721,7 @@ module.exports = {
   prepareMainThreadExecution,
   prepareWorkerThreadExecution,
   prepareShadowRealmExecution,
+  prepareTestRunnerMainExecution,
   markBootstrapComplete,
   loadPreloadModules,
   initializeFrozenIntrinsics,

lib/internal/test_runner/runner.js

@@ -88,6 +88,7 @@ const {
 const { Glob } = require('internal/fs/glob');
 const { once } = require('events');
 const { validatePath } = require('internal/fs/utils');
+const { loadPreloadModules } = require('internal/process/pre_execution');
 const {
   triggerUncaughtException,
   exitCodes: { kGenericUserError },
@@ -692,6 +693,7 @@ function run(options = kEmptyObject) {
   };
   const root = createTestTree(rootTestOptions, globalOptions);
   let testFiles = files ?? createTestFileList(globPatterns, cwd);
+  const { isTestRunner } = globalOptions;
 
   if (shard) {
     testFiles = ArrayPrototypeFilter(testFiles, (_, index) => index % shard.total === shard.index - 1);
@@ -765,6 +767,16 @@ function run(options = kEmptyObject) {
             SafePromiseAllReturnVoid([root.harness.bootstrapPromise, promise]) :
             promise;
 
+          // We need to setup the user modules in the test runner if we are running with
+          // --test-isolation=none and --test in order to avoid loading the user modules
+          // BEFORE the creation of the root test (that would cause them to get lost).
+          if (isTestRunner) {
+            // If we are not coming from the test runner entry point, the user-required and imported
+            // modules have already been loaded.
+            // Since it's possible to delete modules from require.cache, a CommonJS module
+            // could otherwise be executed twice.
+            loadPreloadModules();
+          }
           const userImports = getOptionValue('--import');
           for (let i = 0; i < userImports.length; i++) {
             await cascadedLoader.import(userImports[i], parentURL, kEmptyObject);

test/known_issues/test-runner-no-isolation-hooks.mjs

@@ -1,68 +0,0 @@
-import * as common from '../common/index.mjs';
-import * as fixtures from '../common/fixtures.mjs';
-import { test } from 'node:test';
-
-const testArguments = [
-  '--test',
-  '--test-isolation=none',
-];
-
-const testFiles = [
-  fixtures.path('test-runner', 'no-isolation', 'one.test.js'),
-  fixtures.path('test-runner', 'no-isolation', 'two.test.js'),
-];
-
-const order = [
-  'before(): global',
-
-  'before one: <root>',
-  'suite one',
-
-  'before two: <root>',
-  'suite two',
-
-  'beforeEach(): global',
-  'beforeEach one: suite one - test',
-  'beforeEach two: suite one - test',
-
-  'suite one - test',
-  'afterEach(): global',
-  'afterEach one: suite one - test',
-  'afterEach two: suite one - test',
-
-  'before suite two: suite two',
-  'beforeEach(): global',
-  'beforeEach one: suite two - test',
-  'beforeEach two: suite two - test',
-
-  'suite two - test',
-  'afterEach(): global',
-  'afterEach one: suite two - test',
-  'afterEach two: suite two - test',
-
-  'after(): global',
-  'after one: <root>',
-  'after two: <root>',
-].join('\n');
-
-/**
- * TODO: The `--require` flag is processed in `loadPreloadModules` (process/pre_execution.js) BEFORE
- * the root test is created by the test runner. This causes a global `before` hook to register (and
- * run) but then the root test-case is created, causing the "subsequent" hooks to get lost. This
- * behaviour (CJS route only) is different from the ESM route, where test runner explicitly handles
- * `--import` in `root.runInAsyncScope` (test_runner/runner.js).
- * @see https://github.com/nodejs/node/pull/57595#issuecomment-2770724492
- * @see https://github.com/nodejs/node/issues/57728
- * Moved from test/parallel/test-runner-no-isolation-hooks.mjs
- */
-test('use --require to define global hooks', async (t) => {
-  const { stdout } = await common.spawnPromisified(process.execPath, [
-    ...testArguments,
-    '--require', fixtures.path('test-runner', 'no-isolation', 'global-hooks.cjs'),
-    ...testFiles,
-  ]);
-
-  const testHookOutput = stdout.split('\n▶')[0];
-
-  t.assert.equal(testHookOutput, order);
-});

test/parallel/test-runner-global-setup-teardown.mjs

@@ -388,11 +388,16 @@ async function runTest(
       const cjsPath = join(testFixtures, 'global-setup-teardown', 'required-module.cjs');
       const esmpFile = fixtures.fileURL('test-runner', 'global-setup-teardown', 'imported-module.mjs');
 
-      it('should run required module before globalSetup', async () => {
+      // The difference in behavior is due to how --require and --import are handled by
+      // the main entry point versus the test runner entry point.
+      // When isolation is 'none', both --require and --import are handled by the test runner.
+      const shouldRequireAfterSetup = runnerEnabled && isolation === 'none';
+      const shouldImportAfterSetup = runnerEnabled;
+
+      it(`should run required module ${shouldRequireAfterSetup ? 'after' : 'before'} globalSetup`, async () => {
         const setupFlagPath = tmpdir.resolve('setup-for-required.tmp');
         const teardownFlagPath = tmpdir.resolve('teardown-for-required.tmp');
 
-        // Create a setup file for test-file.js to find
         fs.writeFileSync(setupFlagPath, '');
 
         const { stdout } = await runTest({
@@ -415,29 +420,26 @@ async function runTest(
         assert.match(stdout, /Global setup executed/);
         assert.match(stdout, /Global teardown executed/);
 
-        // Verify that the required module was executed before the global setup
         const requiredExecutedPosition = stdout.indexOf('Required module executed');
         const globalSetupExecutedPosition = stdout.indexOf('Global setup executed');
-        assert.ok(requiredExecutedPosition < globalSetupExecutedPosition,
-                  'Required module should have been executed before global setup');
 
-        // After all tests complete, the teardown should have run
+        assert.ok(
+          shouldRequireAfterSetup ?
+            requiredExecutedPosition > globalSetupExecutedPosition :
+            requiredExecutedPosition < globalSetupExecutedPosition,
+          `Required module should have been executed ${shouldRequireAfterSetup ? 'after' : 'before'} global setup`
+        );
+
         assert.ok(fs.existsSync(teardownFlagPath), 'Teardown flag file should exist');
         const content = fs.readFileSync(teardownFlagPath, 'utf8');
         assert.strictEqual(content, 'Teardown was executed');
-
-        // Setup flag should have been removed by teardown
         assert.ok(!fs.existsSync(setupFlagPath), 'Setup flag file should have been removed');
       });
 
-      // This difference in behavior is due to the way --import is being handled by
+      it(`should run imported module ${shouldImportAfterSetup ? 'after' : 'before'} globalSetup`, async () => {
-      // run_main entry point or test_runner entry point
-      if (runnerEnabled) {
-        it('should run imported module after globalSetup', async () => {
         const setupFlagPath = tmpdir.resolve('setup-for-imported.tmp');
         const teardownFlagPath = tmpdir.resolve('teardown-for-imported.tmp');
 
-          // Create a setup file for test-file.js to find
         fs.writeFileSync(setupFlagPath, 'non-empty');
 
         const { stdout } = await runTest({
@@ -460,63 +462,21 @@ async function runTest(
         assert.match(stdout, /Global setup executed/);
         assert.match(stdout, /Global teardown executed/);
 
-          // Verify that the imported module was executed after the global setup
-          const globalSetupExecutedPosition = stdout.indexOf('Global setup executed');
         const importedExecutedPosition = stdout.indexOf('Imported module executed');
-          assert.ok(globalSetupExecutedPosition < importedExecutedPosition,
+        const globalSetupExecutedPosition = stdout.indexOf('Global setup executed');
-                    'Imported module should be executed after global setup');
+
+        assert.ok(
+          shouldImportAfterSetup ?
+            importedExecutedPosition > globalSetupExecutedPosition :
+            importedExecutedPosition < globalSetupExecutedPosition,
+          `Imported module should have been executed ${shouldImportAfterSetup ? 'after' : 'before'} global setup`
+        );
 
-          // After all tests complete, the teardown should have run
         assert.ok(fs.existsSync(teardownFlagPath), 'Teardown flag file should exist');
         const content = fs.readFileSync(teardownFlagPath, 'utf8');
         assert.strictEqual(content, 'Teardown was executed');
-
-          // Setup flag should have been removed by teardown
         assert.ok(!fs.existsSync(setupFlagPath), 'Setup flag file should have been removed');
       });
-      } else {
-        it('should run imported module before globalSetup', async () => {
-          const setupFlagPath = tmpdir.resolve('setup-for-imported.tmp');
-          const teardownFlagPath = tmpdir.resolve('teardown-for-imported.tmp');
-
-          // Create a setup file for test-file.js to find
-          fs.writeFileSync(setupFlagPath, 'non-empty');
-
-          const { stdout } = await runTest({
-            isolation,
-            globalSetupFile: 'basic-setup-teardown.mjs',
-            importPath: './imported-module.js',
-            env: {
-              SETUP_FLAG_PATH: setupFlagPath,
-              TEARDOWN_FLAG_PATH: teardownFlagPath
-            },
-            additionalFlags: [
-              `--import=${esmpFile}`,
-            ],
-            runnerEnabled
-          });
-
-          assert.match(stdout, /pass 2/);
-          assert.match(stdout, /fail 0/);
-          assert.match(stdout, /Imported module executed/);
-          assert.match(stdout, /Global setup executed/);
-          assert.match(stdout, /Global teardown executed/);
-
-          // Verify that the imported module was executed before the global setup
-          const importedExecutedPosition = stdout.indexOf('Imported module executed');
-          const globalSetupExecutedPosition = stdout.indexOf('Global setup executed');
-          assert.ok(importedExecutedPosition < globalSetupExecutedPosition,
-                    'Imported module should be executed before global setup');
-
-          // After all tests complete, the teardown should have run
-          assert.ok(fs.existsSync(teardownFlagPath), 'Teardown flag file should exist');
-          const content = fs.readFileSync(teardownFlagPath, 'utf8');
-          assert.strictEqual(content, 'Teardown was executed');
-
-          // Setup flag should have been removed by teardown
-          assert.ok(!fs.existsSync(setupFlagPath), 'Setup flag file should have been removed');
-        });
-      }
 
       it('should execute globalSetup and globalTeardown correctly with imported module containing tests', async () => {
         const setupFlagPath = tmpdir.resolve('setup-executed.tmp');

test/parallel/test-runner-no-isolation-hooks.mjs

@@ -68,3 +68,15 @@ test('use --import (ESM) to define global hooks', async (t) => {
 
   t.assert.equal(testHookOutput, order);
 });
+
+test('use --require to define global hooks', async (t) => {
+  const { stdout } = await common.spawnPromisified(process.execPath, [
+    ...testArguments,
+    '--require', fixtures.path('test-runner', 'no-isolation', 'global-hooks.cjs'),
+    ...testFiles,
+  ]);
+
+  const testHookOutput = stdout.split('\n▶')[0];
+
+  t.assert.equal(testHookOutput, order);
+});