test_runner: unify --require and --import behavior when isolation none #57924
+96 −168
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
| Review requested:
|
nodejs-github-bot added needs-ci 
pmarchini force-pushed the test_runner/fix/57728 branch from 599cf81 to f9472b6 Compare 
lib/internal/test_runner/runner.js Outdated
Comment on lines 771 to 774
| const userModules = [ | ||
| ...getOptionValue('--require'), | ||
| ...getOptionValue('--import'), | ||
| ]; |
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.
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!
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 😁
pmarchini force-pushed the test_runner/fix/57728 branch from 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:
|
pmarchini requested review from JakobJingleheimer, MoLow, atlowChemi, cjihrig and jakecastelli 
cjihrig approved these changes Apr 30, 2025
github-actions bot removed the request-ci 
atlowChemi reviewed May 1, 2025
pmarchini added request-ci atlowChemi approved these changes May 3, 2025
pmarchini added author ready 
jakecastelli added the commit-queue nodejs-github-bot removed the commit-queue | 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>
richardlau added the backport-requested-v22.x 
| This doesn't land cleanly on v22.x-staging so a manual backport will be necessary if this is to be released there. |
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.