-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
test_runner: support module detection in module mocks #53642
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
Conversation
This file contains 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
GeoffreyBooth
added
module
Issues and PRs related to the module subsystem.
loaders
Issues and PRs related to ES module loaders
test_runner
Issues and PRs related to the test runner subsystem.
labels
Jun 29, 2024
Review requested:
|
nodejs-github-bot
added
needs-ci
PRs that need a full CI run.
test
Issues and PRs related to the tests.
labels
Jun 29, 2024
cjihrig
approved these changes
Jun 29, 2024
atlowChemi
approved these changes
Jun 30, 2024
benjamingr
approved these changes
Jun 30, 2024
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
jasnell
approved these changes
Jul 4, 2024
GeoffreyBooth
added
author ready
PRs that have at least one approval, no pending requests for changes, and a CI started.
commit-queue
Add this label to land a pull request using GitHub Actions.
commit-queue-squash
Add this label to instruct the Commit Queue to squash all the PR commits into the first one.
labels
Jul 4, 2024
nodejs-github-bot
removed
the
commit-queue
Add this label to land a pull request using GitHub Actions.
label
Jul 4, 2024
Landed in 18a1ac9 |
aduh95
pushed a commit
that referenced
this pull request
Jul 12, 2024
PR-URL: #53642 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
aduh95
pushed a commit
that referenced
this pull request
Jul 16, 2024
PR-URL: #53642 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
targos
pushed a commit
that referenced
this pull request
Sep 21, 2024
PR-URL: #53642 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
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.js
fail 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
format
property returned from theresolve
hook is optional, so this approach is an unsafe one; it breaks not only when detection is enabled, but also if the user registers a customresolve
hook that doesn’t return aformat
property.I’ve updated the logic to instead use the
format
property returned by the nextload
hook in the chain, which is usually Node’s internaldefaultLoad
. Theformat
property is required to be returned fromload
hooks, 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 thatnextLoad
succeeds; 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