From 44aa3e39fa6e0c003b37a1bd76d6feaf5a0ca6cd Mon Sep 17 00:00:00 2001 From: syi0808 Date: Mon, 15 Jul 2024 00:21:32 +0900 Subject: [PATCH 01/10] test_runner: replace cjs resolve to defaultResolve --- lib/test/mock_loader.js | 19 ++++++------------- .../wrong-import-after-module-mocking.js | 12 ++++++++++++ test/parallel/test-runner-mocking.js | 19 +++++++++++++++++++ 3 files changed, 37 insertions(+), 13 deletions(-) create mode 100644 test/fixtures/module-mocking/wrong-import-after-module-mocking.js diff --git a/lib/test/mock_loader.js b/lib/test/mock_loader.js index 307ad980856094..146e83b453d562 100644 --- a/lib/test/mock_loader.js +++ b/lib/test/mock_loader.js @@ -23,6 +23,7 @@ let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => { debug = fn; }); const { createRequire, isBuiltin } = require('module'); +const { defaultResolve } = require('internal/modules/esm/resolve'); // TODO(cjihrig): This file should not be exposed publicly, but register() does // not handle internal loaders. Before marking this API as stable, one of the @@ -95,19 +96,11 @@ async function resolve(specifier, context, nextResolve) { if (isBuiltin(specifier)) { mockSpecifier = ensureNodeScheme(specifier); } else { - // TODO(cjihrig): This try...catch should be replaced by defaultResolve(), - // but there are some edge cases that caused the tests to fail on Windows. - try { - const req = createRequire(context.parentURL); - specifier = pathToFileURL(req.resolve(specifier)).href; - } catch { - const parentURL = normalizeReferrerURL(context.parentURL); - const parsedURL = URL.parse(specifier, parentURL)?.href; - - if (parsedURL) { - specifier = parsedURL; - } - } + specifier = pathToFileURL( + defaultResolve(specifier, { + conditions: context?.conditions, parentURL: normalizeReferrerURL(context.parentURL) + }) + ).href; mockSpecifier = specifier; } diff --git a/test/fixtures/module-mocking/wrong-import-after-module-mocking.js b/test/fixtures/module-mocking/wrong-import-after-module-mocking.js new file mode 100644 index 00000000000000..1aba28a76552e7 --- /dev/null +++ b/test/fixtures/module-mocking/wrong-import-after-module-mocking.js @@ -0,0 +1,12 @@ +import { writeFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { mock } from 'node:test'; + +writeFileSync(resolve(import.meta.dirname, './test-2.js'), 'export default 42'); + +try { + if (mock.module) mock.module('Whatever, this is not significant', { namedExports: {} }); +} catch {} + +const { default: x } = await import('./test-2'); +console.log(`Found x: ${x}`); // prints 42 diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index b5ef955be6926f..3cb0b7d191e3ec 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -1,7 +1,10 @@ 'use strict'; const common = require('../common'); +const child = require('child_process'); const assert = require('node:assert'); const { mock, test } = require('node:test'); +const fixtures = require('../common/fixtures'); +const { unlinkSync } = require('fs'); test('spies on a function', (t) => { const sum = t.mock.fn((arg1, arg2) => { return arg1 + arg2; @@ -1054,3 +1057,19 @@ test('setter() fails if getter options is true', (t) => { t.mock.setter({}, 'method', { getter: true }); }, /The property 'options\.setter' cannot be used with 'options\.getter'/); }); + +test('wrong import syntax should throw error after module mocking.', () => { + const { stdout, stderr } = child.spawnSync( + process.execPath, + [ + '--experimental-test-module-mocks', + '--experimental-default-type=module', + fixtures.path('module-mocking/wrong-import-after-module-mocking.js'), + ] + ); + + unlinkSync(fixtures.path('module-mocking/test-2.js')); + + assert.equal(stdout.toString(), '') + assert.match(stderr.toString(), /Error \[ERR_MODULE_NOT_FOUND\]: Cannot find module/) +}); From 729a8acff2d47d9493c70832446271133d3df389 Mon Sep 17 00:00:00 2001 From: syi0808 Date: Mon, 15 Jul 2024 03:32:32 +0900 Subject: [PATCH 02/10] test_runner: divert resolve logic by type --- lib/test/mock_loader.js | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/lib/test/mock_loader.js b/lib/test/mock_loader.js index 146e83b453d562..78cb0d31b78098 100644 --- a/lib/test/mock_loader.js +++ b/lib/test/mock_loader.js @@ -24,6 +24,7 @@ let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => { }); const { createRequire, isBuiltin } = require('module'); const { defaultResolve } = require('internal/modules/esm/resolve'); +const { defaultGetFormatWithoutErrors } = require('internal/modules/esm/get_format'); // TODO(cjihrig): This file should not be exposed publicly, but register() does // not handle internal loaders. Before marking this API as stable, one of the @@ -96,11 +97,25 @@ async function resolve(specifier, context, nextResolve) { if (isBuiltin(specifier)) { mockSpecifier = ensureNodeScheme(specifier); } else { - specifier = pathToFileURL( - defaultResolve(specifier, { - conditions: context?.conditions, parentURL: normalizeReferrerURL(context.parentURL) - }) - ).href; + const format = defaultGetFormatWithoutErrors(pathToFileURL(specifier)); + + try { + if(format === "module") { + specifier = pathToFileURL( + defaultResolve(specifier, context) + ).href; + } else { + const req = createRequire(context.parentURL); + specifier = pathToFileURL(req.resolve(specifier)).href; + } + } catch { + const parentURL = normalizeReferrerURL(context.parentURL); + const parsedURL = URL.parse(specifier, parentURL)?.href; + + if (parsedURL) { + specifier = parsedURL; + } + } mockSpecifier = specifier; } From 4a2683219c7e94f566e02ba5fd74b3dd9ec09f36 Mon Sep 17 00:00:00 2001 From: syi0808 Date: Mon, 15 Jul 2024 13:45:45 +0900 Subject: [PATCH 03/10] test_runner: get format from parentURL as default or pass specifier argument as fallback --- lib/test/mock_loader.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/test/mock_loader.js b/lib/test/mock_loader.js index 78cb0d31b78098..75756899ff6b28 100644 --- a/lib/test/mock_loader.js +++ b/lib/test/mock_loader.js @@ -23,8 +23,8 @@ let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => { debug = fn; }); const { createRequire, isBuiltin } = require('module'); -const { defaultResolve } = require('internal/modules/esm/resolve'); const { defaultGetFormatWithoutErrors } = require('internal/modules/esm/get_format'); +const { defaultResolve } = require('internal/modules/esm/resolve'); // TODO(cjihrig): This file should not be exposed publicly, but register() does // not handle internal loaders. Before marking this API as stable, one of the @@ -97,13 +97,13 @@ async function resolve(specifier, context, nextResolve) { if (isBuiltin(specifier)) { mockSpecifier = ensureNodeScheme(specifier); } else { - const format = defaultGetFormatWithoutErrors(pathToFileURL(specifier)); + const format = defaultGetFormatWithoutErrors( + pathToFileURL(context.parentURL ?? specifier) + ); try { - if(format === "module") { - specifier = pathToFileURL( - defaultResolve(specifier, context) - ).href; + if (format === 'module') { + specifier = defaultResolve(specifier, context).url; } else { const req = createRequire(context.parentURL); specifier = pathToFileURL(req.resolve(specifier)).href; @@ -240,4 +240,4 @@ function sendAck(buf, status = kMockSuccess) { AtomicsNotify(buf, 0); } -module.exports = { initialize, load, resolve }; +module.exports = { initialize, load, resolve }; \ No newline at end of file From 45e4ea7ca5dcdb835fdb6fd1c4e86db4fa49da0d Mon Sep 17 00:00:00 2001 From: syi0808 Date: Thu, 18 Jul 2024 00:21:25 +0900 Subject: [PATCH 04/10] test_runner: test change to using exist file --- .../module-mocking/wrong-import-after-module-mocking.js | 8 ++------ test/parallel/test-runner-mocking.js | 5 +---- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/test/fixtures/module-mocking/wrong-import-after-module-mocking.js b/test/fixtures/module-mocking/wrong-import-after-module-mocking.js index 1aba28a76552e7..c11975a3efe742 100644 --- a/test/fixtures/module-mocking/wrong-import-after-module-mocking.js +++ b/test/fixtures/module-mocking/wrong-import-after-module-mocking.js @@ -1,12 +1,8 @@ -import { writeFileSync } from 'node:fs'; -import { resolve } from 'node:path'; import { mock } from 'node:test'; -writeFileSync(resolve(import.meta.dirname, './test-2.js'), 'export default 42'); - try { if (mock.module) mock.module('Whatever, this is not significant', { namedExports: {} }); } catch {} -const { default: x } = await import('./test-2'); -console.log(`Found x: ${x}`); // prints 42 +const { string } = await import('./basic-esm'); +console.log(`Found string: ${string}`); // prints 'original esm string' diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index 3cb0b7d191e3ec..28849201acf501 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -4,7 +4,6 @@ const child = require('child_process'); const assert = require('node:assert'); const { mock, test } = require('node:test'); const fixtures = require('../common/fixtures'); -const { unlinkSync } = require('fs'); test('spies on a function', (t) => { const sum = t.mock.fn((arg1, arg2) => { return arg1 + arg2; @@ -1058,7 +1057,7 @@ test('setter() fails if getter options is true', (t) => { }, /The property 'options\.setter' cannot be used with 'options\.getter'/); }); -test('wrong import syntax should throw error after module mocking.', () => { +test.only('wrong import syntax should throw error after module mocking.', () => { const { stdout, stderr } = child.spawnSync( process.execPath, [ @@ -1068,8 +1067,6 @@ test('wrong import syntax should throw error after module mocking.', () => { ] ); - unlinkSync(fixtures.path('module-mocking/test-2.js')); - assert.equal(stdout.toString(), '') assert.match(stderr.toString(), /Error \[ERR_MODULE_NOT_FOUND\]: Cannot find module/) }); From 4d3247e20732f39def42d61ec53983fffb566408 Mon Sep 17 00:00:00 2001 From: syi0808 Date: Thu, 18 Jul 2024 00:22:11 +0900 Subject: [PATCH 05/10] test_runner: use common.spawnPromisified instead of child_process --- test/parallel/test-runner-mocking.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index 28849201acf501..f6a4ae2aa65b10 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -1,6 +1,5 @@ 'use strict'; const common = require('../common'); -const child = require('child_process'); const assert = require('node:assert'); const { mock, test } = require('node:test'); const fixtures = require('../common/fixtures'); @@ -1057,8 +1056,8 @@ test('setter() fails if getter options is true', (t) => { }, /The property 'options\.setter' cannot be used with 'options\.getter'/); }); -test.only('wrong import syntax should throw error after module mocking.', () => { - const { stdout, stderr } = child.spawnSync( +test('wrong import syntax should throw error after module mocking.', async () => { + const { stdout, stderr } = await common.spawnPromisified( process.execPath, [ '--experimental-test-module-mocks', From 61439ba1fec09eecdeaef86004319c81b3111a54 Mon Sep 17 00:00:00 2001 From: syi0808 Date: Thu, 18 Jul 2024 00:59:02 +0900 Subject: [PATCH 06/10] test_runner: use resolveFilename instead of createRequire --- lib/test/mock_loader.js | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/test/mock_loader.js b/lib/test/mock_loader.js index 75756899ff6b28..4ef98e30e08427 100644 --- a/lib/test/mock_loader.js +++ b/lib/test/mock_loader.js @@ -22,7 +22,7 @@ const { normalizeReferrerURL } = require('internal/modules/helpers'); let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => { debug = fn; }); -const { createRequire, isBuiltin } = require('module'); +const { createRequire, isBuiltin, _resolveFilename } = require('module'); const { defaultGetFormatWithoutErrors } = require('internal/modules/esm/get_format'); const { defaultResolve } = require('internal/modules/esm/resolve'); @@ -97,16 +97,19 @@ async function resolve(specifier, context, nextResolve) { if (isBuiltin(specifier)) { mockSpecifier = ensureNodeScheme(specifier); } else { - const format = defaultGetFormatWithoutErrors( - pathToFileURL(context.parentURL ?? specifier) - ); + let format; + + if(context.parentURL) { + format = defaultGetFormatWithoutErrors(pathToFileURL(context.parentURL)); + } try { if (format === 'module') { specifier = defaultResolve(specifier, context).url; } else { - const req = createRequire(context.parentURL); - specifier = pathToFileURL(req.resolve(specifier)).href; + specifier = pathToFileURL( + createRequire(context.parentURL).resolve(specifier) + ).href; } } catch { const parentURL = normalizeReferrerURL(context.parentURL); @@ -240,4 +243,4 @@ function sendAck(buf, status = kMockSuccess) { AtomicsNotify(buf, 0); } -module.exports = { initialize, load, resolve }; \ No newline at end of file +module.exports = { initialize, load, resolve }; From 45fb6b1b233eee7693ef9b4fa51180efd31335a8 Mon Sep 17 00:00:00 2001 From: syi0808 Date: Fri, 19 Jul 2024 00:50:48 +0900 Subject: [PATCH 07/10] test_runner: remove module extension from imported file in fixture --- test/fixtures/module-mocking/basic-esm-without-extension.js | 1 + .../module-mocking/wrong-import-after-module-mocking.js | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/module-mocking/basic-esm-without-extension.js diff --git a/test/fixtures/module-mocking/basic-esm-without-extension.js b/test/fixtures/module-mocking/basic-esm-without-extension.js new file mode 100644 index 00000000000000..f2b47223e34a83 --- /dev/null +++ b/test/fixtures/module-mocking/basic-esm-without-extension.js @@ -0,0 +1 @@ +export let string = 'original esm string'; diff --git a/test/fixtures/module-mocking/wrong-import-after-module-mocking.js b/test/fixtures/module-mocking/wrong-import-after-module-mocking.js index c11975a3efe742..54ffb30057f94a 100644 --- a/test/fixtures/module-mocking/wrong-import-after-module-mocking.js +++ b/test/fixtures/module-mocking/wrong-import-after-module-mocking.js @@ -4,5 +4,5 @@ try { if (mock.module) mock.module('Whatever, this is not significant', { namedExports: {} }); } catch {} -const { string } = await import('./basic-esm'); +const { string } = await import('./basic-esm-without-extension'); console.log(`Found string: ${string}`); // prints 'original esm string' From 98870be5ae56e2133a6556accbecee48437af24f Mon Sep 17 00:00:00 2001 From: Sung Ye In <66503450+syi0808@users.noreply.github.com> Date: Sat, 3 Aug 2024 13:19:32 +0900 Subject: [PATCH 08/10] refactor: if to optional chaining Co-authored-by: Antoine du Hamel --- .../module-mocking/wrong-import-after-module-mocking.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/fixtures/module-mocking/wrong-import-after-module-mocking.js b/test/fixtures/module-mocking/wrong-import-after-module-mocking.js index 54ffb30057f94a..9220e23f5047de 100644 --- a/test/fixtures/module-mocking/wrong-import-after-module-mocking.js +++ b/test/fixtures/module-mocking/wrong-import-after-module-mocking.js @@ -1,7 +1,7 @@ import { mock } from 'node:test'; try { - if (mock.module) mock.module('Whatever, this is not significant', { namedExports: {} }); + mock.module?.('Whatever, this is not significant', { namedExports: {} }); } catch {} const { string } = await import('./basic-esm-without-extension'); From a6ad04ecad1de3fefee03f2b5ab38186e2fe8b33 Mon Sep 17 00:00:00 2001 From: syi0808 Date: Sat, 3 Aug 2024 13:23:29 +0900 Subject: [PATCH 09/10] refactor: test move to module-mocking --- test/parallel/test-runner-mocking.js | 14 -------------- test/parallel/test-runner-module-mocking.js | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index f6a4ae2aa65b10..b59661fba85d6a 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -1055,17 +1055,3 @@ test('setter() fails if getter options is true', (t) => { t.mock.setter({}, 'method', { getter: true }); }, /The property 'options\.setter' cannot be used with 'options\.getter'/); }); - -test('wrong import syntax should throw error after module mocking.', async () => { - const { stdout, stderr } = await common.spawnPromisified( - process.execPath, - [ - '--experimental-test-module-mocks', - '--experimental-default-type=module', - fixtures.path('module-mocking/wrong-import-after-module-mocking.js'), - ] - ); - - assert.equal(stdout.toString(), '') - assert.match(stderr.toString(), /Error \[ERR_MODULE_NOT_FOUND\]: Cannot find module/) -}); diff --git a/test/parallel/test-runner-module-mocking.js b/test/parallel/test-runner-module-mocking.js index 13b614f6e9fe45..f7601730ec7298 100644 --- a/test/parallel/test-runner-module-mocking.js +++ b/test/parallel/test-runner-module-mocking.js @@ -638,3 +638,18 @@ test('defaultExports work with ESM mocks in both module systems', async (t) => { assert.strictEqual((await import(fixture)).default, defaultExport); assert.strictEqual(require(fixture), defaultExport); }); + +test('wrong import syntax should throw error after module mocking.', async () => { + const { stdout, stderr, code } = await common.spawnPromisified( + process.execPath, + [ + '--experimental-test-module-mocks', + '--experimental-default-type=module', + fixtures.path('module-mocking/wrong-import-after-module-mocking.js'), + ] + ); + + assert.strictEqual(stdout, ''); + assert.match(stderr, /Error \[ERR_MODULE_NOT_FOUND\]: Cannot find module/); + assert.strictEqual(code, 1); +}); From 83adb9aac30fb057c12a0b93fbde4dd6bd6e8b3d Mon Sep 17 00:00:00 2001 From: syi0808 Date: Sat, 3 Aug 2024 17:31:36 +0900 Subject: [PATCH 10/10] formatting --- lib/test/mock_loader.js | 8 ++++---- test/parallel/test-runner-mocking.js | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/test/mock_loader.js b/lib/test/mock_loader.js index 4ef98e30e08427..8ec5a5d2a08ea5 100644 --- a/lib/test/mock_loader.js +++ b/lib/test/mock_loader.js @@ -22,7 +22,7 @@ const { normalizeReferrerURL } = require('internal/modules/helpers'); let debug = require('internal/util/debuglog').debuglog('test_runner', (fn) => { debug = fn; }); -const { createRequire, isBuiltin, _resolveFilename } = require('module'); +const { createRequire, isBuiltin } = require('module'); const { defaultGetFormatWithoutErrors } = require('internal/modules/esm/get_format'); const { defaultResolve } = require('internal/modules/esm/resolve'); @@ -98,8 +98,8 @@ async function resolve(specifier, context, nextResolve) { mockSpecifier = ensureNodeScheme(specifier); } else { let format; - - if(context.parentURL) { + + if (context.parentURL) { format = defaultGetFormatWithoutErrors(pathToFileURL(context.parentURL)); } @@ -108,7 +108,7 @@ async function resolve(specifier, context, nextResolve) { specifier = defaultResolve(specifier, context).url; } else { specifier = pathToFileURL( - createRequire(context.parentURL).resolve(specifier) + createRequire(context.parentURL).resolve(specifier), ).href; } } catch { diff --git a/test/parallel/test-runner-mocking.js b/test/parallel/test-runner-mocking.js index b59661fba85d6a..b5ef955be6926f 100644 --- a/test/parallel/test-runner-mocking.js +++ b/test/parallel/test-runner-mocking.js @@ -2,7 +2,6 @@ const common = require('../common'); const assert = require('node:assert'); const { mock, test } = require('node:test'); -const fixtures = require('../common/fixtures'); test('spies on a function', (t) => { const sum = t.mock.fn((arg1, arg2) => { return arg1 + arg2;