test_runner: unify --require and --import behavior when isolation none#57924
Merged
nodejs-github-bot merged 3 commits intonodejs:mainfrom May 6, 2025
Merged
test_runner: unify --require and --import behavior when isolation none#57924nodejs-github-bot merged 3 commits intonodejs:mainfrom
nodejs-github-bot merged 3 commits intonodejs:mainfrom
Conversation
Collaborator
|
Review requested:
|
599cf81 to
f9472b6
Compare
Member
JakobJingleheimer
left a comment
There was a problem hiding this comment.
Thanks for picking this up!
lib/internal/test_runner/runner.js
Outdated
Comment on lines
771
to
774
| const userModules = [ | ||
| ...getOptionValue('--require'), | ||
| ...getOptionValue('--import'), | ||
| ]; |
Member
There was a problem hiding this comment.
I think the approach here is logical in terms of steps. But 2 caveats:
CascadedLoader(akaESMLoaderakaModuleLoader) does not handle CJS modules exactly the same asCJSLoader; if we're in ESM mode, this is "correct" (ESMLoader should handle it); but if not ESM mode, for good or ill, it could lead to unexpected behaviour.- Does this prevent the
--required module(s) executing prior to this? If not, they could be stateful or something else crazy which would get lost when the clone gets created here.
Member
Author
There was a problem hiding this comment.
Thanks for your review, @JakobJingleheimer 🚀
Regarding nr1: ok! I suspected that. I'll take a look to understand if we can just use the default method we're using for --require in the "default" entry point.
nr2: Yes, it prevents the default handling of --require from happening, it's now being executed only at this point!
Member
Author
There was a problem hiding this comment.
I've updated the logic to use loadPreloadModules and applied the other suggestions.
Before marking this PR as "ready to be reviewed", I'd like to ask you to take another look at these changes 😁
f9472b6 to
a2134be
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #57924 +/- ##
==========================================
- Coverage 90.26% 90.19% -0.07%
==========================================
Files 630 630
Lines 185933 186470 +537
Branches 36450 36624 +174
==========================================
+ Hits 167829 168186 +357
- Misses 10972 11061 +89
- Partials 7132 7223 +91
🚀 New features to boost your workflow:
|
cjihrig
approved these changes
Apr 30, 2025
Collaborator
atlowChemi
reviewed
May 1, 2025
Collaborator
Collaborator
Collaborator
Collaborator
Collaborator
Collaborator
Collaborator
Collaborator
atlowChemi
approved these changes
May 3, 2025
Collaborator
Collaborator
Collaborator
Collaborator
|
Landed in 7e24ebc |
targos
pushed a commit
that referenced
this pull request
May 16, 2025
PR-URL: #57924 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Member
|
This doesn't land cleanly on v22.x-staging so a manual backport will be necessary if this is to be released there. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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.
I'm opening this PR as a draft to discuss the implementation.
The goal is to address issue #57728.
This PR unifies the behaviour of
--requireand--importfor the specific case where the test runner is run with isolationnone.