From f46d501b7f4c6a8b838f472693596044d14a566f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 22 Oct 2025 13:05:39 +0200 Subject: [PATCH] test_runner: use module.registerHooks in module mocks Migrate away from module.register(). This no longer needs to deal with the worker synchronization. PR-URL: https://github.com/nodejs/node/pull/60326 Reviewed-By: Marco Ippolito Reviewed-By: Chemi Atlow Reviewed-By: Yagiz Nizipli Reviewed-By: Geoffrey Booth --- lib/internal/test_runner/coverage.js | 2 +- lib/internal/test_runner/mock/loader.js | 90 ++++-------------- lib/internal/test_runner/mock/mock.js | 92 +++++++------------ .../test-permission-dc-worker-threads.js | 19 ---- test/parallel/test-runner-module-mocking.js | 41 --------- 5 files changed, 54 insertions(+), 190 deletions(-) delete mode 100644 test/parallel/test-permission-dc-worker-threads.js diff --git a/lib/internal/test_runner/coverage.js b/lib/internal/test_runner/coverage.js index 80811dffb1..8fa9c87256 100644 --- a/lib/internal/test_runner/coverage.js +++ b/lib/internal/test_runner/coverage.js @@ -37,7 +37,7 @@ const { }, } = require('internal/errors'); const { matchGlobPattern } = require('internal/fs/glob'); -const { kMockSearchParam } = require('internal/test_runner/mock/mock'); +const { constants: { kMockSearchParam } } = require('internal/test_runner/mock/loader'); const kCoverageFileRegex = /^coverage-(\d+)-(\d{13})-(\d+)\.json$/; const kIgnoreRegex = /\/\* node:coverage ignore next (?\d+ )?\*\//; diff --git a/lib/internal/test_runner/mock/loader.js b/lib/internal/test_runner/mock/loader.js index ddde5599df..a7a22539be 100644 --- a/lib/internal/test_runner/mock/loader.js +++ b/lib/internal/test_runner/mock/loader.js @@ -1,77 +1,24 @@ 'use strict'; const { - AtomicsNotify, - AtomicsStore, JSONStringify, SafeMap, } = primordials; -const { - kBadExportsMessage, - kMockSearchParam, - kMockSuccess, - kMockExists, - kMockUnknownMessage, -} = require('internal/test_runner/mock/mock'); + +const kMockSearchParam = 'node-test-mock'; +const kBadExportsMessage = 'Cannot create mock because named exports ' + + 'cannot be applied to the provided default export.'; + const { URL, URLParse } = require('internal/url'); let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => { debug = fn; }); -// TODO(cjihrig): The mocks need to be thread aware because the exports are -// evaluated on the thread that creates the mock. Before marking this API as -// stable, one of the following issues needs to be implemented: -// https://github.com/nodejs/node/issues/49472 -// or https://github.com/nodejs/node/issues/52219 - const mocks = new SafeMap(); -async function initialize(data) { - data?.port.on('message', ({ type, payload }) => { - debug('mock loader received message type "%s" with payload %o', type, payload); - - if (type === 'node:test:register') { - const { baseURL } = payload; - const mock = mocks.get(baseURL); - - if (mock?.active) { - debug('already mocking "%s"', baseURL); - sendAck(payload.ack, kMockExists); - return; - } - - const localVersion = mock?.localVersion ?? 0; - - debug('new mock version %d for "%s"', localVersion, baseURL); - mocks.set(baseURL, { - __proto__: null, - active: true, - cache: payload.cache, - exportNames: payload.exportNames, - format: payload.format, - hasDefaultExport: payload.hasDefaultExport, - localVersion, - url: baseURL, - }); - sendAck(payload.ack); - } else if (type === 'node:test:unregister') { - const mock = mocks.get(payload.baseURL); - - if (mock !== undefined) { - mock.active = false; - mock.localVersion++; - } - - sendAck(payload.ack); - } else { - sendAck(payload.ack, kMockUnknownMessage); - } - }); -} - -async function resolve(specifier, context, nextResolve) { +function resolve(specifier, context, nextResolve) { debug('resolve hook entry, specifier = "%s", context = %o', specifier, context); - const nextResolveResult = await nextResolve(specifier, context); + const nextResolveResult = nextResolve(specifier, context); const mockSpecifier = nextResolveResult.url; const mock = mocks.get(mockSpecifier); @@ -95,7 +42,7 @@ async function resolve(specifier, context, nextResolve) { return { __proto__: null, url: href, format: nextResolveResult.format }; } -async function load(url, context, nextLoad) { +function load(url, context, nextLoad) { debug('load hook entry, url = "%s", context = %o', url, context); const parsedURL = URLParse(url); if (parsedURL) { @@ -105,7 +52,7 @@ async function load(url, context, nextLoad) { const baseURL = parsedURL ? parsedURL.href : url; const mock = mocks.get(baseURL); - const original = await nextLoad(url, context); + const original = nextLoad(url, context); debug('load hook, mock = %o', mock); if (mock?.active !== true) { return original; @@ -130,14 +77,14 @@ async function load(url, context, nextLoad) { __proto__: null, format, shortCircuit: true, - source: await createSourceFromMock(mock, format), + source: createSourceFromMock(mock, format), }; debug('load hook finished, result = %o', result); return result; } -async function createSourceFromMock(mock, format) { +function createSourceFromMock(mock, format) { // Create mock implementation from provided exports. const { exportNames, hasDefaultExport, url } = mock; const useESM = format === 'module' || format === 'module-typescript'; @@ -196,9 +143,12 @@ if (module.exports === null || typeof module.exports !== 'object') { return source; } -function sendAck(buf, status = kMockSuccess) { - AtomicsStore(buf, 0, status); - AtomicsNotify(buf, 0); -} - -module.exports = { initialize, load, resolve }; +module.exports = { + hooks: { __proto__: null, load, resolve }, + mocks, + constants: { + __proto__: null, + kBadExportsMessage, + kMockSearchParam, + }, +}; diff --git a/lib/internal/test_runner/mock/mock.js b/lib/internal/test_runner/mock/mock.js index 8a63c7757d..fac97e51b6 100644 --- a/lib/internal/test_runner/mock/mock.js +++ b/lib/internal/test_runner/mock/mock.js @@ -2,12 +2,9 @@ const { ArrayPrototypePush, ArrayPrototypeSlice, - AtomicsStore, - AtomicsWait, Error, FunctionPrototypeBind, FunctionPrototypeCall, - Int32Array, ObjectDefineProperty, ObjectGetOwnPropertyDescriptor, ObjectGetPrototypeOf, @@ -19,9 +16,6 @@ const { SafeMap, StringPrototypeSlice, StringPrototypeStartsWith, - globalThis: { - SharedArrayBuffer, - }, } = primordials; const { codes: { @@ -54,19 +48,10 @@ const { validateOneOf, } = require('internal/validators'); const { MockTimers } = require('internal/test_runner/mock/mock_timers'); -const { strictEqual, notStrictEqual } = require('assert'); const { Module } = require('internal/modules/cjs/loader'); -const { MessageChannel } = require('worker_threads'); const { _load, _nodeModulePaths, _resolveFilename, isBuiltin } = Module; function kDefaultFunction() {} const enableModuleMocking = getOptionValue('--experimental-test-module-mocks'); -const kMockSearchParam = 'node-test-mock'; -const kMockSuccess = 1; -const kMockExists = 2; -const kMockUnknownMessage = 3; -const kWaitTimeout = 5_000; -const kBadExportsMessage = 'Cannot create mock because named exports ' + - 'cannot be applied to the provided default export.'; const kSupportedFormats = [ 'builtin', 'commonjs-typescript', @@ -76,6 +61,11 @@ const kSupportedFormats = [ 'module', ]; let sharedModuleState; +const { + hooks: mockHooks, + mocks, + constants: { kBadExportsMessage, kMockSearchParam }, +} = require('internal/test_runner/mock/loader'); class MockFunctionContext { #calls; @@ -201,8 +191,8 @@ class MockModuleContext { hasDefaultExport, namedExports, sharedState, + specifier, }) { - const ack = new Int32Array(new SharedArrayBuffer(4)); const config = { __proto__: null, cache, @@ -218,7 +208,6 @@ class MockModuleContext { this.#sharedState = sharedState; this.#restore = { __proto__: null, - ack, baseURL, cached: fullPath in Module._cache, format, @@ -226,20 +215,29 @@ class MockModuleContext { value: Module._cache[fullPath], }; - sharedState.loaderPort.postMessage({ - __proto__: null, - type: 'node:test:register', - payload: { + const mock = mocks.get(baseURL); + + if (mock?.active) { + debug('already mocking "%s"', baseURL); + throw new ERR_INVALID_STATE( + `Cannot mock '${specifier}'. The module is already mocked.`, + ); + } else { + const localVersion = mock?.localVersion ?? 0; + + debug('new mock version %d for "%s"', localVersion, baseURL); + mocks.set(baseURL, { __proto__: null, - ack, - baseURL, + url: baseURL, cache, exportNames: ObjectKeys(namedExports), hasDefaultExport, format, - }, - }); - waitForAck(ack); + localVersion, + active: true, + }); + } + delete Module._cache[fullPath]; sharedState.mockExports.set(baseURL, { __proto__: null, @@ -261,17 +259,12 @@ class MockModuleContext { Module._cache[this.#restore.fullPath] = this.#restore.value; } - AtomicsStore(this.#restore.ack, 0, 0); - this.#sharedState.loaderPort.postMessage({ - __proto__: null, - type: 'node:test:unregister', - payload: { - __proto__: null, - ack: this.#restore.ack, - baseURL: this.#restore.baseURL, - }, - }); - waitForAck(this.#restore.ack); + const mock = mocks.get(this.#restore.baseURL); + + if (mock !== undefined) { + mock.active = false; + mock.localVersion++; + } this.#sharedState.mockMap.delete(this.#restore.baseURL); this.#sharedState.mockMap.delete(this.#restore.fullPath); @@ -654,7 +647,7 @@ class MockTracker { const hasFileProtocol = StringPrototypeStartsWith(filename, 'file://'); const caller = hasFileProtocol ? filename : pathToFileURL(filename).href; const { format, url } = sharedState.moduleLoader.resolveSync( - mockSpecifier, caller, null, + mockSpecifier, caller, kEmptyObject, ); debug('module mock, url = "%s", format = "%s", caller = "%s"', url, format, caller); if (format) { // Format is not yet known for ambiguous files when detection is enabled. @@ -828,20 +821,13 @@ function setupSharedModuleState() { if (sharedModuleState === undefined) { const { mock } = require('test'); const mockExports = new SafeMap(); - const { port1, port2 } = new MessageChannel(); + const { registerHooks } = require('internal/modules/customization_hooks'); const moduleLoader = esmLoader.getOrInitializeCascadedLoader(); - moduleLoader.register( - 'internal/test_runner/mock/loader', - 'node:', - { __proto__: null, port: port2 }, - [port2], - true, - ); + registerHooks(mockHooks); sharedModuleState = { __proto__: null, - loaderPort: port1, mockExports, mockMap: new SafeMap(), moduleLoader, @@ -941,13 +927,6 @@ function findMethodOnPrototypeChain(instance, methodName) { return descriptor; } -function waitForAck(buf) { - const result = AtomicsWait(buf, 0, 0, kWaitTimeout); - - notStrictEqual(result, 'timed-out', 'test mocking synchronization failed'); - strictEqual(buf[0], kMockSuccess); -} - function ensureNodeScheme(specifier) { if (!StringPrototypeStartsWith(specifier, 'node:')) { return `node:${specifier}`; @@ -962,10 +941,5 @@ if (!enableModuleMocking) { module.exports = { ensureNodeScheme, - kBadExportsMessage, - kMockSearchParam, - kMockSuccess, - kMockExists, - kMockUnknownMessage, MockTracker, }; diff --git a/test/parallel/test-permission-dc-worker-threads.js b/test/parallel/test-permission-dc-worker-threads.js deleted file mode 100644 index 4fdb566f9e..0000000000 --- a/test/parallel/test-permission-dc-worker-threads.js +++ /dev/null @@ -1,19 +0,0 @@ -// Flags: --permission --allow-fs-read=* --experimental-test-module-mocks -'use strict'; - -const common = require('../common'); -const assert = require('node:assert'); - -{ - const diagnostics_channel = require('node:diagnostics_channel'); - diagnostics_channel.subscribe('worker_threads', common.mustNotCall()); - const { mock } = require('node:test'); - - // Module mocking should throw instead of posting to worker_threads dc - assert.throws(() => { - mock.module('node:path'); - }, common.expectsError({ - code: 'ERR_ACCESS_DENIED', - permission: 'WorkerThreads', - })); -} diff --git a/test/parallel/test-runner-module-mocking.js b/test/parallel/test-runner-module-mocking.js index 89e08a9e22..dcb6f84597 100644 --- a/test/parallel/test-runner-module-mocking.js +++ b/test/parallel/test-runner-module-mocking.js @@ -679,44 +679,3 @@ test('wrong import syntax should throw error after module mocking', async () => assert.match(stderr, /Error \[ERR_MODULE_NOT_FOUND\]: Cannot find module/); assert.strictEqual(code, 1); }); - -test('should throw ERR_ACCESS_DENIED when permission model is enabled', async (t) => { - const cwd = fixtures.path('test-runner'); - const fixture = fixtures.path('test-runner', 'mock-nm.js'); - const args = [ - '--permission', - '--allow-fs-read=*', - '--experimental-test-module-mocks', - fixture, - ]; - const { - code, - stdout, - } = await common.spawnPromisified(process.execPath, args, { cwd }); - - assert.strictEqual(code, 1); - assert.match(stdout, /Error: Access to this API has been restricted/); - assert.match(stdout, /permission: 'WorkerThreads'/); -}); - -test('should work when --allow-worker is passed and permission model is enabled', async (t) => { - const cwd = fixtures.path('test-runner'); - const fixture = fixtures.path('test-runner', 'mock-nm.js'); - const args = [ - '--permission', - '--allow-fs-read=*', - '--allow-worker', - '--experimental-test-module-mocks', - fixture, - ]; - const { - code, - stdout, - stderr, - signal, - } = await common.spawnPromisified(process.execPath, args, { cwd }); - - assert.strictEqual(code, 0, stderr); - assert.strictEqual(signal, null); - assert.match(stdout, /pass 1/, stderr); -});