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

PR-URL: #57924
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>

Author: pmarchini

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

@@ -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,108 +420,63 @@ 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
-      // 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({
-            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 after the global setup
-          const globalSetupExecutedPosition = stdout.indexOf('Global setup executed');
-          const importedExecutedPosition = stdout.indexOf('Imported module executed');
-          assert.ok(globalSetupExecutedPosition < importedExecutedPosition,
-                    'Imported module should be executed after 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 run imported module ${shouldImportAfterSetup ? 'after' : 'before'} globalSetup`, async () => {
+        const setupFlagPath = tmpdir.resolve('setup-for-imported.tmp');
+        const teardownFlagPath = tmpdir.resolve('teardown-for-imported.tmp');
+
+        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/);
+
+        const importedExecutedPosition = stdout.indexOf('Imported module executed');
+        const globalSetupExecutedPosition = stdout.indexOf('Global setup executed');
+
+        assert.ok(
+          shouldImportAfterSetup ?
+            importedExecutedPosition > globalSetupExecutedPosition :
+            importedExecutedPosition < globalSetupExecutedPosition,
+          `Imported module should have been executed ${shouldImportAfterSetup ? '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');
+        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-global-setup-teardown.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);
+});