GeezerVison

Review requested:

• @nodejs/test_runner

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.31%. Comparing base (68e94c1) to head (ac0933b).
Report is 336 commits behind head on main.

@@           Coverage Diff           @@
##             main   #54223   +/-   ##
=======================================
  Coverage   87.31%   87.31%           
=======================================
  Files         648      648           
  Lines      182362   182336   -26     
  Branches    34986    34980    -6     
=======================================
- Hits       159229   159212   -17     
+ Misses      16398    16392    -6     
+ Partials     6735     6732    -3     

importing a path, we only support importing URLs

Can you clarify this? From what I can tell, dynamic import does work with paths.

> await import('./foo.mjs')
[Module: null prototype] { foo: { foo: 1 } }

Can you clarify this? From what I can tell, dynamic import does work with paths.

In the example you give here (import("./foo.mjs")), you're passing a relative URL, not a path. As long as you are not on Windows and the path you're using does not contain any special char (such as %, ?, #, etc.), a relative path can serves as a relative URL, hence the confusion I suppose – also, an absolute Unix path can serve as a origin-relative URL as long as no special char are in use.

/// Assuming path of the current module is /tmp/entry.mjs

await import('./foo?test.js'); // Would attempt to load /tmp/foo with import.meta.url being file:///tmp/foo?test.js
await import('./foo%3Ftest.js'); // Would attempt to load /tmp/foo?test.js with import.meta.url being file:///tmp/foo%3Ftest.js

await import('/path/to/file#1.js'); // Would try to load /path/to/file with import.meta.url being file:///path/to/file#1.js
await import('/path/to/file%231.js'); // Would try to load /path/to/file#1.js with import.meta.url being file:///path/to/file%231.js

Another question: if this is only an internal refactor/simplification, I would expect the existing tests to continue working. Why are all of the changes to the test file required?

Another question: if this is only an internal refactor/simplification, I would expect the existing tests to continue working. Why are all of the changes to the test file required?

That's fair, let's move the test refactor to a dedicated PR (#54233)

CI: https://ci.nodejs.org/job/node-test-pull-request/60915/

So it breaks on Windows – but IMO that's because the tests are wrong, importing a Windows path is not supposed to work.

importing a Windows path is not supposed to work

I ran into a similar issue when originally developing the feature. The tests only broke on Windows when I tried to implement things more cleanly. I assumed that importing a Windows path was supposed to work because in the tests, code like this works without any mocks configured:

const fixture = fixtures.path('module-mocking', 'basic-esm.mjs');
const original = await import(fixture);

Maybe it's possible that the code only loads because the mock loader is enabled, even with no mocks configured. If the test is wrong and the code doesn't load without the mock loader, then we should drop the invalid test(s) or even add a test to make sure it never works with the mock loader.

I assumed that importing a Windows path was supposed to work because in the tests, code like this works without any mocks configured:

const fixture = fixtures.path('module-mocking', 'basic-esm.mjs');
const original = await import(fixture);

Are you sure? It seems like somthing that shouldn't work. IIRC you should get Unknown protocol c: or something like that.

I edited my comment, but what might be happening is that the mock loader is enabled and even though no modules are mocked, the resolve() function is allowing those Windows paths to be loaded. I don't have access to Windows to verify, but if you're confident that it shouldn't work, I'm OK with updating those tests.

Let me validate that once I get a hand on a Windows machine.

I tried reproducing on 22.6.0 on Windows. Both node --experimental-test-module-mocks -e "import(path.resolve('file.mjs'))" and node -e "import(path.resolve('file.mjs'))" report the error I was mentioning earlier (Only URLs with a scheme file, data, and node are supported […]. Received protocol 'c:'). I tried adding the test runner to the mix, but that didn't change anything. Mocking the path would then make the import succeeds, but that's because the mock loader interferes with the usual resolve algorithm.

I suggest we land #54233, then I can add a Windows-only test case in this PR that validates passing a path to import results in a rejected promise.

SGTM. Thanks!

CI: https://ci.nodejs.org/job/node-test-pull-request/60985/

@aduh95 I think this is ready for the change you proposed in #54223 (comment).

It looks like it's not possible to support t.mock.module(somePath) with my changes (at least on Windows). IMO we should make it support only URLs, that way it's consistent on all OSes, and consistent with import() – and let's be real, for most cases, URLs and paths are indistinguishable from each other. I'll open a separate PR for that.

CI: https://ci.nodejs.org/job/node-test-pull-request/61294/

Landed in b4344cf