From 1c29e74d304af0827514e8815f04080fc53bd254 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Tue, 20 Aug 2024 19:39:40 +0200 Subject: [PATCH] test_runner: make `mock.module`'s `specifier` consistent with `import()` 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: https://github.com/nodejs/node/pull/54416 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Moshe Atlow --- doc/api/test.md | 2 +- lib/internal/test_runner/mock/mock.js | 9 ++-- test/parallel/test-runner-module-mocking.js | 59 +++++++++++---------- 3 files changed, 39 insertions(+), 31 deletions(-) diff --git a/doc/api/test.md b/doc/api/test.md index 9fb227b1b4e534..cafcd5f9389405 100644 --- a/doc/api/test.md +++ b/doc/api/test.md @@ -2040,7 +2040,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 index 828f3e513f8e5f..a0de3d2dd41909 100644 --- 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 index 7e59c0d4dcde8b..a9a5c33a7c26b4 100644 --- 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,10 +513,11 @@ 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); @@ -522,7 +525,7 @@ test('CJS mocks can be used by both module systems', async (t) => { 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 () => {