test_runner: mock_loader resolve the cjs and esm formats respectively. #53846

syi0808 · 2024-07-14T18:37:47Z

This issue was caused by recovering specifier in cjs resolve without format(esm,cjs) check in mock_loader.
So I did resolve logic differently for each format. And also added test.

Fixes: #53807

nodejs-github-bot · 2024-07-14T18:37:53Z

Review requested:

@nodejs/test_runner

RedYetiDev · 2024-07-14T20:18:59Z

Could you amend your commits to be test_runner rather than mock_loader? Thanks!

syi0808 · 2024-07-15T02:38:44Z

Could you amend your commits to be test_runner rather than mock_loader? Thanks!

I will do that. Thank you!
And I think I need to get the format of the parent URL, but now I'm getting the format of the import specifier. I'll change it.
The parentURL has not been handed over to the context parameter, so i should include the modification of the parentURL along with the call of the rescue function.

syi0808 · 2024-07-15T04:46:37Z

I changed to use specifier only when there is no parentURL.

GeoffreyBooth

This just got changed a few days ago in order to unflag detect-module: #53619. Please let that land first, then rebase and ensure that your changes still work when detection is enabled by default.

GeoffreyBooth · 2024-07-15T05:11:41Z

lib/test/mock_loader.js

+const { defaultResolve } = require('internal/modules/esm/resolve');
+const {
+  defaultGetFormatWithoutErrors,
+} = require('internal/modules/esm/get_format');


It feels wrong to me that the mock loader would be using module internals directly rather than the public API. This interdependence will make it harder to refactor modules stuff since non-public changes might break the test runner.

As far as I understand, mock_loader is an internal module resolver when using module mock. So this has to decide the format internally, is there an public API that can solve it? I'm sorry if it's something I haven't found. Thanks!

You should be able to use the next hook in the chain, I'd think.

test/fixtures/module-mocking/wrong-import-after-module-mocking.js

lib/test/mock_loader.js

aduh95 · 2024-07-17T12:34:27Z

lib/test/mock_loader.js

+        const req = createRequire(context.parentURL);
+        specifier = pathToFileURL(req.resolve(specifier)).href;


As long as we're comfortable depending on internal things, we should skip the createRequire call (which is quite heavy) and call directly the CJS resolver.

I wanted to figure out how not to call createRequire, but this would be difficult because the existing cjs filenameResolve relies on a thoroughly generated require.

#53846 (comment)
If we think of a direction that does not depend on the internal module, I think this problem can also be solved by creating a new filename resolve for cjs and esm.

lib/test/mock_loader.js

test/fixtures/module-mocking/wrong-import-after-module-mocking.js

test/parallel/test-runner-mocking.js

GeoffreyBooth · 2024-07-18T14:31:37Z

@syi0808 If you can verify that this won't break with detect-module unflagged, I'll lift my block. I thought that could land this week but it looks like it'll be pushed to this weekend or next week and I don't want to keep you waiting.

@nodejs/test_runner Is there a way to refactor the mock loader to not depend on modules internals? I'd prefer not to keep having issues with modules updates breaking the mock loader or test runner updates necessitating changes in modules code. The current implementation has one small reference to modules internals that I bet could be refactored out, and I feel like this PR could solve the referenced issue without needing internals. If it can't currently be done without internals, I think that's perhaps a sign that we should create new modules public APIs, like import { defaultResolve } from 'node:module'. Then any test runner can benefit from such access, not just Node's internal one, and if our test runner limits itself to public APIs we avoid the refactoring issue.

cjihrig · 2024-07-18T14:37:58Z

If you think it can be refactored to not use the module internals, that would be great. My feeling from working on the mock loader is that we may need some other public API. I know during development I found it frustrating trying to normalize all of the different possible inputs (path, file URL, file URL string, core, CJS, ESM, relative, absolute, etc.) into something consistent.

GeoffreyBooth · 2024-07-18T14:44:32Z

. I know during development I found it frustrating trying to normalize all of the different possible inputs

Can we just have the user resolve before passing it in? Something like mock(new URL(import.meta.resolve('module-to-be-mocked'))).

cjihrig · 2024-07-18T15:05:36Z

That seems like a worse DX IMO since it punts the problem to users. It also seems like functionality that at least some customization hooks authors will run into. IIRC, defaultResolve() was close to enough but broke in the CI on Windows. If someone with a Windows setup can debug that, maybe it could be made to work (but is that a modules internal that you wouldn't want being used?).

GeoffreyBooth · 2024-07-18T15:54:41Z

is that a modules internal that you wouldn't want being used?).

It is currently, but we could expose it. I think it's very low risk because it's essentially already public via import.meta.resolve. That's better than any modules internals potentially being used in the mock loader.

syi0808 · 2024-07-18T17:40:27Z

@syi0808 If you can verify that this won't break with detect-module unflagged, I'll lift my block. I thought that could land this week but it looks like it'll be pushed to this weekend or next week and I don't want to keep you waiting.

@nodejs/test_runner Is there a way to refactor the mock loader to not depend on modules internals? I'd prefer not to keep having issues with modules updates breaking the mock loader or test runner updates necessitating changes in modules code. The current implementation has one small reference to modules internals that I bet could be refactored out, and I feel like this PR could solve the referenced issue without needing internals. If it can't currently be done without internals, I think that's perhaps a sign that we should create new modules public APIs, like import { defaultResolve } from 'node:module'. Then any test runner can benefit from such access, not just Node's internal one, and if our test runner limits itself to public APIs we avoid the refactoring issue.

If there's no disagreement with the new API coming out for resolve, I think I can try to refactor this.

I will test the detect-module unflagged you mentioned after refactoring.

GeoffreyBooth · 2024-07-18T19:12:21Z

If there's no disagreement with the new API coming out for resolve, I think I can try to refactor this.

I think if we make the new API it should be its own PR. But before that, are we sure this PR needs it? You have access to defaultResolve (as next) within the resolve hook; can that do what you need?

syi0808 · 2024-07-18T23:53:21Z

If there's no disagreement with the new API coming out for resolve, I think I can try to refactor this.

I think if we make the new API it should be its own PR. But before that, are we sure this PR needs it? You have access to defaultResolve (as next) within the resolve hook; can that do what you need?

I think this PR is necessary.

Basically, the purpose of fixing is as follows. Suppose you call a mock.module inside an ESM file. This will destroy the existing require behavior, regardless of which file format or package within the mock.module. This is because mock.module adds a hook, and inside the mock_loader, even though parentURL is in ESM format, it resolves like CJS.

The import_loader breaks this despite the incorrect import format because the mock_loader hands over the resolved url to the nextResolve.
However, the existing implementation does not follow the parentURL format, so it is difficult for CJS resolver to determine whether it has been properly resolved. It is also problematic to hand over the received specifier to the nextResolve.

// ESM
mock.module("./test.js")
import("./test")

In the currently implementation, we cannot prevent this behavior.

In order to resolve this, we have to rely on nextResolve, as you think inside the mock_loader, but the mock.module doesn't know what hooks will come before and after depending on the timing of the call.

The purpose of resolve the specifier in mock_loader is to match the URL that was locked in mock.js. If there is no custom hook after mock_loader in the ESM file, you can get the same effect as defaultResolve by calling nextResolve. If there is a custom hook after mock_loader, this may be unintended behavior, there is no guarantee that the saved filename will be the same as the mockSpecifier because the internal implementation of the custom resolve cannot be predicted.
Therefore, I think it is desirable to normalize the mock_loader internally according to the format of parentURL and call nextResolve if the locked module cannot be found based on it.

Additionally, assuming that the user doesn't know that the mock_loader writes registers internally, it may not be understood that the mock_loader can behave differently depending on the timing of the call. I think it would be nice to be able to pull up the hook of the mock_loader to be the first to run.

GeoffreyBooth · 2024-07-19T01:45:02Z

Is the mock loader trying to define a mock for when a specifier is required, imported, or both? Because of exports conditions, the same specifier might resolve as different things when it's imported or when it's required. I think it's hazard prone to try to guess which module system the user wants the mock for; I know it's less convenient, but requiring the user to pass in a URL instance would eliminate the ambiguity.

syi0808 · 2024-07-19T02:23:51Z

I also don't want the user to guess the module system.

I just didn't want the existing import behavior to be broken just by mocking the module.

To be more specific, call CJS filenameResolve to normalize the specifier, find the mocking module, and if can't find it, if you pass the specifier taken over by parameter to nextResolve, the case that corresponds to the issue will be covered.

// ESM
mock.module("./test.js")
import("./test")

But this case is still not covered.

In conclusion, this pr is just exception handling. As you said, it would be nice to hand over this exception to the user, but if they use mock.module, they should pay attention to all import syntax.

…rgument as fallback

GeoffreyBooth · 2024-07-21T16:29:14Z

Can I open a follow up PR about creating a new API and refactoring mock_loader? Or is this the core team's job?

You're volunteering PRs so that makes you part of the core team 😄

If it's possible to achieve the fix you're aiming for in this PR without using modules internals, I would prefer such a solution. If not, it's better to fix a bug than leave it broken, and we can follow up with how to address the issues related to the mock loader.

syi0808 · 2024-07-23T04:16:01Z

If it's possible to achieve the fix you're aiming for in this PR without using modules internals, I would prefer such a solution. If not, it's better to fix a bug than leave it broken, and we can follow up with how to address the issues related to the mock loader.

Sorry, I can't think of a way for the mock_loader to not dependent on the cjs and module format resolve method.

Or shall we wait for this to find a better idea?

syi0808 · 2024-07-27T08:01:18Z

I think the time has come to propose a new API that is merge before this is left unattended.

I'm going to create a resolve function that integrates CJS and ESM. What do you think? @GeoffreyBooth

aduh95

I have another approach that would simplify the loader code greatly IMO, but let's land this first, as it's easier to iterate on a follow-up PR than for me to make a very large suggestion in this PR

test/fixtures/module-mocking/wrong-import-after-module-mocking.js

test/parallel/test-runner-mocking.js

Co-authored-by: Antoine du Hamel <[email protected]>

nodejs-github-bot · 2024-08-03T12:39:26Z

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

syi0808 · 2024-08-04T16:03:41Z

I don't know if the failed CI is related to this PR.

nodejs-github-bot · 2024-08-06T05:57:25Z

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

aduh95 · 2024-08-06T09:24:27Z

Landed in 5bfebfe

syi0808 · 2024-08-06T09:44:01Z

I have another approach that would simplify the loader code greatly IMO, but let's land this first, as it's easier to iterate on a follow-up PR than for me to make a very large suggestion in this PR

Are you going to open a follow-up PR for this?

PR-URL: #53846 Fixes: #53807 Reviewed-By: Antoine du Hamel <[email protected]>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 14, 2024

RedYetiDev added the test_runner Issues and PRs related to the test runner subsystem. label Jul 14, 2024

RedYetiDev added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed test Issues and PRs related to the tests. labels Jul 14, 2024

syi0808 changed the title ~~mock_loader: replace cjs resolve to defaultResolve~~ test_runner: replace cjs resolve to defaultResolve Jul 15, 2024

syi0808 force-pushed the fix/module-mock-throw-error-wrong-import branch from bdfa404 to 94b1014 Compare July 15, 2024 04:13

GeoffreyBooth requested changes Jul 15, 2024

View reviewed changes

GeoffreyBooth previously requested changes Jul 15, 2024

View reviewed changes

RedYetiDev added the blocked PRs that are blocked by other issues or PRs. label Jul 15, 2024

syi0808 force-pushed the fix/module-mock-throw-error-wrong-import branch 2 times, most recently from aa8fe42 to bad16b3 Compare July 15, 2024 13:22

ChALkeR reviewed Jul 17, 2024

View reviewed changes

test/fixtures/module-mocking/wrong-import-after-module-mocking.js Outdated Show resolved Hide resolved

aduh95 reviewed Jul 17, 2024

View reviewed changes

aduh95 removed the blocked PRs that are blocked by other issues or PRs. label Jul 17, 2024

syi0808 changed the title ~~test_runner: replace cjs resolve to defaultResolve~~ test_runner: mock_loader resolve the cjs and esm formats respectively. Jul 17, 2024

syi0808 added 3 commits July 22, 2024 00:08

test_runner: replace cjs resolve to defaultResolve

44aa3e3

test_runner: divert resolve logic by type

729a8ac

test_runner: get format from parentURL as default or pass specifier a…

4a26832

…rgument as fallback

syi0808 force-pushed the fix/module-mock-throw-error-wrong-import branch from e8e47ad to 4c9cc82 Compare July 21, 2024 15:28

syi0808 added 4 commits July 22, 2024 00:29

test_runner: test change to using exist file

45e4ea7

test_runner: use common.spawnPromisified instead of child_process

4d3247e

test_runner: use resolveFilename instead of createRequire

61439ba

test_runner: remove module extension from imported file in fixture

45fb6b1

syi0808 force-pushed the fix/module-mock-throw-error-wrong-import branch from 4c9cc82 to 45fb6b1 Compare July 21, 2024 15:29

syi0808 requested a review from GeoffreyBooth August 1, 2024 16:22

aduh95 approved these changes Aug 2, 2024

View reviewed changes

test/fixtures/module-mocking/wrong-import-after-module-mocking.js Outdated Show resolved Hide resolved

test/parallel/test-runner-mocking.js Outdated Show resolved Hide resolved

aduh95 reviewed Aug 2, 2024

View reviewed changes

test/parallel/test-runner-mocking.js Outdated Show resolved Hide resolved

aduh95 reviewed Aug 2, 2024

View reviewed changes

test/parallel/test-runner-mocking.js Outdated Show resolved Hide resolved

syi0808 and others added 3 commits August 3, 2024 13:19

refactor: if to optional chaining

98870be

Co-authored-by: Antoine du Hamel <[email protected]>

refactor: test move to module-mocking

a6ad04e

formatting

83adb9a

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 3, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 3, 2024

aduh95 merged commit 5bfebfe into nodejs:main Aug 6, 2024
56 checks passed

targos pushed a commit that referenced this pull request Aug 14, 2024

test_runner: make mock_loader not confuse CJS and ESM resolution

dc46573

PR-URL: #53846 Fixes: #53807 Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test_runner: mock_loader resolve the cjs and esm formats respectively. #53846

test_runner: mock_loader resolve the cjs and esm formats respectively. #53846

syi0808 commented Jul 14, 2024

nodejs-github-bot commented Jul 14, 2024

RedYetiDev commented Jul 14, 2024

syi0808 commented Jul 15, 2024

syi0808 commented Jul 15, 2024

GeoffreyBooth left a comment

GeoffreyBooth Jul 15, 2024

syi0808 Jul 15, 2024 •

edited

Loading

GeoffreyBooth Jul 17, 2024

aduh95 Jul 17, 2024

syi0808 Jul 17, 2024 •

edited

Loading

GeoffreyBooth commented Jul 18, 2024

cjihrig commented Jul 18, 2024

GeoffreyBooth commented Jul 18, 2024

cjihrig commented Jul 18, 2024

GeoffreyBooth commented Jul 18, 2024

syi0808 commented Jul 18, 2024

GeoffreyBooth commented Jul 18, 2024

syi0808 commented Jul 18, 2024

GeoffreyBooth commented Jul 19, 2024

syi0808 commented Jul 19, 2024 •

edited

Loading

GeoffreyBooth commented Jul 21, 2024

syi0808 commented Jul 23, 2024 •

edited

Loading

syi0808 commented Jul 27, 2024 •

edited

Loading

aduh95 left a comment

nodejs-github-bot commented Aug 3, 2024

syi0808 commented Aug 4, 2024

nodejs-github-bot commented Aug 6, 2024

aduh95 commented Aug 6, 2024

syi0808 commented Aug 6, 2024

		const req = createRequire(context.parentURL);
		specifier = pathToFileURL(req.resolve(specifier)).href;

test_runner: mock_loader resolve the cjs and esm formats respectively. #53846

test_runner: mock_loader resolve the cjs and esm formats respectively. #53846

Conversation

syi0808 commented Jul 14, 2024

nodejs-github-bot commented Jul 14, 2024

RedYetiDev commented Jul 14, 2024

syi0808 commented Jul 15, 2024

syi0808 commented Jul 15, 2024

GeoffreyBooth left a comment

Choose a reason for hiding this comment

GeoffreyBooth Jul 15, 2024

Choose a reason for hiding this comment

syi0808 Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

GeoffreyBooth Jul 17, 2024

Choose a reason for hiding this comment

aduh95 Jul 17, 2024

Choose a reason for hiding this comment

syi0808 Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

GeoffreyBooth commented Jul 18, 2024

cjihrig commented Jul 18, 2024

GeoffreyBooth commented Jul 18, 2024

cjihrig commented Jul 18, 2024

GeoffreyBooth commented Jul 18, 2024

syi0808 commented Jul 18, 2024

GeoffreyBooth commented Jul 18, 2024

syi0808 commented Jul 18, 2024

GeoffreyBooth commented Jul 19, 2024

syi0808 commented Jul 19, 2024 • edited Loading

GeoffreyBooth commented Jul 21, 2024

syi0808 commented Jul 23, 2024 • edited Loading

syi0808 commented Jul 27, 2024 • edited Loading

aduh95 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Aug 3, 2024

syi0808 commented Aug 4, 2024

nodejs-github-bot commented Aug 6, 2024

aduh95 commented Aug 6, 2024

syi0808 commented Aug 6, 2024

syi0808 Jul 15, 2024 •

edited

Loading

syi0808 Jul 17, 2024 •

edited

Loading

syi0808 commented Jul 19, 2024 •

edited

Loading

syi0808 commented Jul 23, 2024 •

edited

Loading

syi0808 commented Jul 27, 2024 •

edited

Loading