test_runner: make mock.module's specifier consistent with import()

aduh95 · web-flow · commit 68e94c1e75f7 · 2024-08-20T17:39:40.000Z
The previous implementation was trying to follow both `require` and `import` conventions. It is not practical to try to follow both, and aligning with `import()` seems to be what makes the most sense. PR-URL: nodejs#54416 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
diff --git a/doc/api/test.md b/doc/api/test.md
@@ -2050,7 +2050,7 @@ added: v22.3.0
 
 > Stability: 1.0 - Early development
 
-* `specifier` {string} A string identifying the module to mock.
+* `specifier` {string|URL} A string identifying the module to mock.
 * `options` {Object} Optional configuration options for the mock module. The
   following properties are supported:
   * `cache` {boolean} If `false`, each call to `require()` or `import()`
diff --git a/lib/internal/test_runner/mock/mock.js b/lib/internal/test_runner/mock/mock.js
@@ -34,7 +34,7 @@ const {
 } = require('internal/errors');
 const esmLoader = require('internal/modules/esm/loader');
 const { getOptionValue } = require('internal/options');
-const { fileURLToPath, toPathIfFileURL, URL } = require('internal/url');
+const { fileURLToPath, toPathIfFileURL, URL, isURL } = require('internal/url');
 const {
   emitExperimentalWarning,
   getStructuredStack,
@@ -49,7 +49,6 @@ const {
   validateInteger,
   validateObject,
   validateOneOf,
-  validateString,
 } = require('internal/validators');
 const { MockTimers } = require('internal/test_runner/mock/mock_timers');
 const { strictEqual, notStrictEqual } = require('assert');
@@ -488,7 +487,11 @@ class MockTracker {
 
   module(specifier, options = kEmptyObject) {
     emitExperimentalWarning('Module mocking');
-    validateString(specifier, 'specifier');
+    if (typeof specifier !== 'string') {
+      if (!isURL(specifier))
+        throw new ERR_INVALID_ARG_TYPE('specifier', ['string', 'URL'], specifier);
+      specifier = `${specifier}`;
+    }
     validateObject(options, 'options');
     debug('module mock entry, specifier = "%s", options = %o', specifier, options);
 
diff --git a/test/parallel/test-runner-module-mocking.js b/test/parallel/test-runner-module-mocking.js
@@ -10,7 +10,7 @@ const fixtures = require('../common/fixtures');
 const assert = require('node:assert');
 const { relative } = require('node:path');
 const { test } = require('node:test');
-const { fileURLToPath, pathToFileURL } = require('node:url');
+const { pathToFileURL } = require('node:url');
 
 test('input validation', async (t) => {
   await t.test('throws if specifier is not a string', (t) => {
@@ -154,7 +154,7 @@ test('CJS mocking with namedExports option', async (t) => {
     assert.strictEqual(original.string, 'original cjs string');
     assert.strictEqual(original.fn, undefined);
 
-    t.mock.module(fixture, {
+    t.mock.module(pathToFileURL(fixture), {
       namedExports: { fn() { return 42; } },
     });
     const mocked = require(fixture);
@@ -174,7 +174,7 @@ test('CJS mocking with namedExports option', async (t) => {
     assert.strictEqual(original.string, 'original cjs string');
     assert.strictEqual(original.fn, undefined);
 
-    t.mock.module(fixture, {
+    t.mock.module(pathToFileURL(fixture), {
       namedExports: { fn() { return 42; } },
       cache: true,
     });
@@ -195,7 +195,7 @@ test('CJS mocking with namedExports option', async (t) => {
     assert.strictEqual(original.string, 'original cjs string');
     assert.strictEqual(original.fn, undefined);
 
-    t.mock.module(fixture, {
+    t.mock.module(pathToFileURL(fixture), {
       namedExports: { fn() { return 42; } },
       cache: false,
     });
@@ -219,7 +219,7 @@ test('CJS mocking with namedExports option', async (t) => {
 
     const defaultExport = { val1: 5, val2: 3 };
 
-    t.mock.module(fixture, {
+    t.mock.module(pathToFileURL(fixture), {
       defaultExport,
       namedExports: { val1: 'mock value' },
     });
@@ -242,7 +242,7 @@ test('CJS mocking with namedExports option', async (t) => {
 
     const defaultExport = null;
 
-    t.mock.module(fixture, {
+    t.mock.module(pathToFileURL(fixture), {
       defaultExport,
       namedExports: { val1: 'mock value' },
     });
@@ -256,7 +256,7 @@ test('CJS mocking with namedExports option', async (t) => {
 
 test('ESM mocking with namedExports option', async (t) => {
   await t.test('does not cache by default', async (t) => {
-    const fixture = fixtures.path('module-mocking', 'basic-esm.mjs');
+    const fixture = fixtures.fileURL('module-mocking', 'basic-esm.mjs');
     const original = await import(fixture);
 
     assert.strictEqual(original.string, 'original esm string');
@@ -276,7 +276,7 @@ test('ESM mocking with namedExports option', async (t) => {
   });
 
   await t.test('explicitly enables caching', async (t) => {
-    const fixture = fixtures.path('module-mocking', 'basic-esm.mjs');
+    const fixture = fixtures.fileURL('module-mocking', 'basic-esm.mjs');
     const original = await import(fixture);
 
     assert.strictEqual(original.string, 'original esm string');
@@ -297,7 +297,7 @@ test('ESM mocking with namedExports option', async (t) => {
   });
 
   await t.test('explicitly disables caching', async (t) => {
-    const fixture = fixtures.path('module-mocking', 'basic-esm.mjs');
+    const fixture = fixtures.fileURL('module-mocking', 'basic-esm.mjs');
     const original = await import(fixture);
 
     assert.strictEqual(original.string, 'original esm string');
@@ -318,7 +318,8 @@ test('ESM mocking with namedExports option', async (t) => {
   });
 
   await t.test('named exports are not applied to defaultExport', async (t) => {
-    const fixture = fixtures.path('module-mocking', 'basic-esm.mjs');
+    const fixturePath = fixtures.path('module-mocking', 'basic-esm.mjs');
+    const fixture = pathToFileURL(fixturePath);
     const original = await import(fixture);
 
     assert.strictEqual(original.string, 'original esm string');
@@ -338,11 +339,11 @@ test('ESM mocking with namedExports option', async (t) => {
     assert.strictEqual(mocked.default, 'mock default');
     assert.strictEqual(mocked.val1, 'mock value');
     t.mock.reset();
-    common.expectRequiredModule(require(fixture), original);
+    common.expectRequiredModule(require(fixturePath), original);
   });
 
   await t.test('throws if named exports cannot be applied to defaultExport as CJS', async (t) => {
-    const fixture = fixtures.path('module-mocking', 'basic-cjs.js');
+    const fixture = fixtures.fileURL('module-mocking', 'basic-cjs.js');
     const original = await import(fixture);
 
     assert.strictEqual(original.default.string, 'original cjs string');
@@ -366,13 +367,14 @@ test('ESM mocking with namedExports option', async (t) => {
 test('modules cannot be mocked multiple times at once', async (t) => {
   await t.test('CJS', async (t) => {
     const fixture = fixtures.path('module-mocking', 'basic-cjs.js');
+    const fixtureURL = pathToFileURL(fixture).href;
 
-    t.mock.module(fixture, {
+    t.mock.module(fixtureURL, {
       namedExports: { fn() { return 42; } },
     });
 
     assert.throws(() => {
-      t.mock.module(fixture, {
+      t.mock.module(fixtureURL, {
         namedExports: { fn() { return 55; } },
       });
     }, {
@@ -386,7 +388,7 @@ test('modules cannot be mocked multiple times at once', async (t) => {
   });
 
   await t.test('ESM', async (t) => {
-    const fixture = fixtures.path('module-mocking', 'basic-esm.mjs');
+    const fixture = fixtures.fileURL('module-mocking', 'basic-esm.mjs').href;
 
     t.mock.module(fixture, {
       namedExports: { fn() { return 42; } },
@@ -409,10 +411,10 @@ test('modules cannot be mocked multiple times at once', async (t) => {
 
 test('mocks are automatically restored', async (t) => {
   const cjsFixture = fixtures.path('module-mocking', 'basic-cjs.js');
-  const esmFixture = fixtures.path('module-mocking', 'basic-esm.mjs');
+  const esmFixture = fixtures.fileURL('module-mocking', 'basic-esm.mjs');
 
   await t.test('CJS', async (t) => {
-    t.mock.module(cjsFixture, {
+    t.mock.module(pathToFileURL(cjsFixture), {
       namedExports: { fn() { return 42; } },
     });
 
@@ -442,9 +444,9 @@ test('mocks are automatically restored', async (t) => {
 
 test('mocks can be restored independently', async (t) => {
   const cjsFixture = fixtures.path('module-mocking', 'basic-cjs.js');
-  const esmFixture = fixtures.path('module-mocking', 'basic-esm.mjs');
+  const esmFixture = fixtures.fileURL('module-mocking', 'basic-esm.mjs');
 
-  const cjsMock = t.mock.module(cjsFixture, {
+  const cjsMock = t.mock.module(pathToFileURL(cjsFixture), {
     namedExports: { fn() { return 42; } },
   });
 
@@ -511,18 +513,19 @@ test('node:- core module mocks can be used by both module systems', async (t) =>
 
 test('CJS mocks can be used by both module systems', async (t) => {
   const cjsFixture = fixtures.path('module-mocking', 'basic-cjs.js');
-  const cjsMock = t.mock.module(cjsFixture, {
+  const cjsFixtureURL = pathToFileURL(cjsFixture);
+  const cjsMock = t.mock.module(cjsFixtureURL, {
     namedExports: { fn() { return 42; } },
   });
-  let esmImpl = await import(pathToFileURL(cjsFixture));
+  let esmImpl = await import(cjsFixtureURL);
   let cjsImpl = require(cjsFixture);
 
   assert.strictEqual(esmImpl.fn(), 42);
   assert.strictEqual(cjsImpl.fn(), 42);
 
   cjsMock.restore();
 
-  esmImpl = await import(pathToFileURL(cjsFixture));
+  esmImpl = await import(cjsFixtureURL);
   cjsImpl = require(cjsFixture);
 
   assert.strictEqual(esmImpl.default.string, 'original cjs string');
@@ -532,7 +535,7 @@ test('CJS mocks can be used by both module systems', async (t) => {
 test('relative paths can be used by both module systems', async (t) => {
   const fixture = relative(
     __dirname, fixtures.path('module-mocking', 'basic-esm.mjs')
-  );
+  ).replaceAll('\\', '/');
   const mock = t.mock.module(fixture, {
     namedExports: { fn() { return 42; } },
   });
@@ -597,24 +600,26 @@ test('mocked modules do not impact unmocked modules', async (t) => {
 
 test('defaultExports work with CJS mocks in both module systems', async (t) => {
   const fixture = fixtures.path('module-mocking', 'basic-cjs.js');
+  const fixtureURL = pathToFileURL(fixture);
   const original = require(fixture);
   const defaultExport = Symbol('default');
 
   assert.strictEqual(original.string, 'original cjs string');
-  t.mock.module(fixture, { defaultExport });
+  t.mock.module(fixtureURL, { defaultExport });
   assert.strictEqual(require(fixture), defaultExport);
-  assert.strictEqual((await import(pathToFileURL(fixture))).default, defaultExport);
+  assert.strictEqual((await import(fixtureURL)).default, defaultExport);
 });
 
 test('defaultExports work with ESM mocks in both module systems', async (t) => {
-  const fixture = fixtures.fileURL('module-mocking', 'basic-esm.mjs');
+  const fixturePath = fixtures.path('module-mocking', 'basic-esm.mjs');
+  const fixture = pathToFileURL(fixturePath);
   const original = await import(fixture);
   const defaultExport = Symbol('default');
 
   assert.strictEqual(original.string, 'original esm string');
   t.mock.module(`${fixture}`, { defaultExport });
   assert.strictEqual((await import(fixture)).default, defaultExport);
-  assert.strictEqual(require(fileURLToPath(fixture)), defaultExport);
+  assert.strictEqual(require(fixturePath), defaultExport);
 });
 
 test('wrong import syntax should throw error after module mocking.', async () => {
