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 <marcoippolito54@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

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 (?<count>\d+ )?\*\//;

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,
+  },
+};

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,
 };

test/parallel/test-permission-dc-worker-threads.js

@@ -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',
-  }));
-}

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);
-});