test_runner: refactor mock_loader

[aduh95]

It seems the mock_loader can be greatly simplified if it doesn't try to support things Node.js doesn't support (e.g. importing a path, we only support importing URLs), and do not need to rely too much on internals.

[nodejs-github-bot]

Review requested:

• @nodejs/test_runner

[codecov]

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.

Additional details and impacted files

@@           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     

Files with missing lines	Coverage Δ
lib/internal/test_runner/mock/loader.js	95.52% <100.00%> (-0.62%)	⬇️
lib/internal/test_runner/mock/mock.js	99.25% <100.00%> (+<0.01%)	⬆️

... and 24 files with indirect coverage changes

[cjihrig]

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

[aduh95]

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

[cjihrig]

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?

[aduh95]

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)

[cjihrig]

LGTM if the CI passes.

[nodejs-github-bot]

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

[aduh95]

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

[cjihrig]

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.

[aduh95]

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.

[cjihrig]

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.

[aduh95]

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

[aduh95]

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.

[aduh95]

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.

[cjihrig]

SGTM. Thanks!

[nodejs-github-bot]

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

[cjihrig]

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

[aduh95]

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.

[nodejs-github-bot]

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

[cjihrig]

Still LGTM since I guess the commit queue needs a reapproval.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54223
✔  Done loading data for nodejs/node/pull/54223
----------------------------------- PR info ------------------------------------
Title      test_runner: refactor `mock_loader` (#54223)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     aduh95:refactor-mock_loader -> nodejs:main
Labels     test, author ready, needs-ci, test_runner
Commits    2
 - test_runner: refactor `mock_loader`
 - squash! add Windows-only test
Committers 1
 - Antoine du Hamel <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54223
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54223
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: James M Snell <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 06 Aug 2024 09:37:08 GMT
   ✔  Approvals: 5
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54223#pullrequestreview-2221307097
   ✔  - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/54223#pullrequestreview-2250956747
   ✔  - Chemi Atlow (@atlowChemi): https://github.com/nodejs/node/pull/54223#pullrequestreview-2222805845
   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/54223#pullrequestreview-2230052660
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54223#pullrequestreview-2231864401
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-08-20T17:54:28Z: https://ci.nodejs.org/job/node-test-pull-request/61294/
- Querying data for job/node-test-pull-request/61294/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 54223
From https://github.com/nodejs/node
 * branch                  refs/pull/54223/merge -> FETCH_HEAD
✔  Fetched commits as cc26951180e6..ac0933b2ab3b
--------------------------------------------------------------------------------
[main f1e95b31b5] test_runner: refactor `mock_loader`
 Author: Antoine du Hamel <[email protected]>
 Date: Sat Aug 3 14:30:47 2024 +0200
 2 files changed, 16 insertions(+), 42 deletions(-)
[main d4e46cdcd7] squash! add Windows-only test
 Author: Antoine du Hamel <[email protected]>
 Date: Fri Aug 16 23:57:03 2024 +0200
 1 file changed, 6 insertions(+)
   ✔  Patches applied
Please run the following commands to complete landing
$ git rebase origin/main --no-keep-empty -i -x "git node land --amend" --autosquash

$ git node land --continue

https://github.com/nodejs/node/actions/runs/10492400336

[nodejs-github-bot]

Landed in b4344cf