-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
test_runner: support module detection in module mocks #53642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
nodejs-github-bot
merged 1 commit into
nodejs:main
from
GeoffreyBooth:fix-mocking-with-detection
Jul 4, 2024
Merged
test_runner: support module detection in module mocks #53642
nodejs-github-bot
merged 1 commit into
nodejs:main
from
GeoffreyBooth:fix-mocking-with-detection
Jul 4, 2024
+17
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
|
Review requested:
|
cjihrig
approved these changes
Jun 29, 2024
atlowChemi
approved these changes
Jun 30, 2024
benjamingr
approved these changes
Jun 30, 2024
Member
benjamingr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thanks
Collaborator
jasnell
approved these changes
Jul 4, 2024
Collaborator
|
Landed in 18a1ac9 |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
author ready
PRs that have at least one approval, no pending requests for changes, and a CI started.
commit-queue-squash
Add this label to instruct the Commit Queue to squash all the PR commits into the first one.
loaders
Issues and PRs related to ES module loaders
module
Issues and PRs related to the module subsystem.
needs-ci
PRs that need a full CI run.
test_runner
Issues and PRs related to the test runner subsystem.
test
Issues and PRs related to the tests.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The tests in
test-runner-module-mocking.jsfail when module detection is enabled; you can see by running the following on currentmain:(The first two flags are present already at the top of
test-runner-module-mocking.js.)I think that this means that the test runner module mocking feature is already broken for anyone trying to use it along with
--experimental-detect-module; and of course this becomes all the more urgent to fix as we hopefully unflag--experimental-detect-module(#53619).The issue stems from the module mocking using resolution to determine the format to use for what kind of source to generate for the mock, CommonJS or ESM. The
formatproperty returned from theresolvehook is optional, so this approach is an unsafe one; it breaks not only when detection is enabled, but also if the user registers a customresolvehook that doesn’t return aformatproperty.I’ve updated the logic to instead use the
formatproperty returned by the nextloadhook in the chain, which is usually Node’s internaldefaultLoad. Theformatproperty is required to be returned fromloadhooks, so this should be a safe approach. The only potential complication is that this requires the module to be mocked to actually exist on disk, so thatnextLoadsucceeds; it’s unclear to me whether this was an expected requirement previously. (There were no tests for mocking nonexistent modules.) I think it should be acceptable, because generally users would be mocking existing things? If mocking nonexistent modules is a requirement, we will need to take another approach or add a fallback approach for that condition.cc @cjihrig @nodejs/test_runner