From 7e24ebc78079bc7551b6dd96e1f0d249f4599605 Mon Sep 17 00:00:00 2001 From: Pietro Marchini Date: Tue, 6 May 2025 08:28:29 +0200 Subject: [PATCH] test_runner: unify --require and --import behavior when isolation none PR-URL: https://github.com/nodejs/node/pull/57924 Reviewed-By: Colin Ihrig Reviewed-By: Chemi Atlow --- lib/internal/main/test_runner.js | 10 +- lib/internal/process/pre_execution.js | 14 +- lib/internal/test_runner/runner.js | 12 ++ .../test-runner-no-isolation-hooks.mjs | 68 --------- .../test-runner-global-setup-teardown.mjs | 136 +++++++----------- .../test-runner-no-isolation-hooks.mjs | 12 ++ 6 files changed, 90 insertions(+), 162 deletions(-) delete mode 100644 test/known_issues/test-runner-no-isolation-hooks.mjs diff --git a/lib/internal/main/test_runner.js b/lib/internal/main/test_runner.js index 87c7dca048b..fda47897da9 100644 --- a/lib/internal/main/test_runner.js +++ b/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. ' + diff --git a/lib/internal/process/pre_execution.js b/lib/internal/process/pre_execution.js index 65c161584f3..8877184fcc5 100644 --- a/lib/internal/process/pre_execution.js +++ b/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, diff --git a/lib/internal/test_runner/runner.js b/lib/internal/test_runner/runner.js index bbc2515473c..0f40fa1016b 100644 --- a/lib/internal/test_runner/runner.js +++ b/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); diff --git a/test/known_issues/test-runner-no-isolation-hooks.mjs b/test/known_issues/test-runner-no-isolation-hooks.mjs deleted file mode 100644 index 7ef087ce015..00000000000 --- a/test/known_issues/test-runner-no-isolation-hooks.mjs +++ /dev/null @@ -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: ', - 'suite one', - - 'before two: ', - '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: ', - 'after two: ', -].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); -}); diff --git a/test/parallel/test-runner-global-setup-teardown.mjs b/test/parallel/test-runner-global-setup-teardown.mjs index eceba774d16..9498efbc73f 100644 --- a/test/parallel/test-runner-global-setup-teardown.mjs +++ b/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,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'); + 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'); - // Create a setup file for test-file.js to find - fs.writeFileSync(setupFlagPath, 'non-empty'); + 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'); + 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 }); - } 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'); + 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 { 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 - }); + const importedExecutedPosition = stdout.indexOf('Imported module executed'); + const globalSetupExecutedPosition = stdout.indexOf('Global setup executed'); - 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/); + assert.ok( + shouldImportAfterSetup ? + importedExecutedPosition > globalSetupExecutedPosition : + importedExecutedPosition < globalSetupExecutedPosition, + `Imported module should have been executed ${shouldImportAfterSetup ? 'after' : 'before'} global setup` + ); - // 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'); - }); - } + 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'); diff --git a/test/parallel/test-runner-no-isolation-hooks.mjs b/test/parallel/test-runner-no-isolation-hooks.mjs index 7896ae2fec8..d67195bae83 100644 --- a/test/parallel/test-runner-no-isolation-hooks.mjs +++ b/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); +});