test_runner: avoid reading process.argv and process.cwd() in run()#61554
test_runner: avoid reading process.argv and process.cwd() in run()#61554batjko wants to merge 2 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
nodejs-github-bot
added
needs-ci
| } | ||
|
|
||
| // Capture process state before passing to run() to avoid reading from process.cwd() inside run() | ||
| options.cwd = process.cwd(); |
There was a problem hiding this comment.
This option is, by default, process.cwd in the run method.
While this clarifies the intent better, I don't think it's strictly necessary
There was a problem hiding this comment.
You're right, it's redundant since run() defaults it anyway. I added it to make the intent explicit (capturing process state at the CLI boundary), but happy to drop it if you think it's just noise.
| } | ||
|
|
||
| async function getReportersMap(reporters, destinations) { | ||
| async function getReportersMap(reporters, destinations, cwd) { |
There was a problem hiding this comment.
Is this change introducing a new behaviour? If so, do we have tests covering it? 👀
There was a problem hiding this comment.
No new behavior - the cwd ?? process.cwd() fallback means it behaves exactly as before when cwd isn't passed. When it IS passed (CLI path or programmatic API with explicit cwd), it uses that instead. Existing tests should cover both paths.
There was a problem hiding this comment.
Then, if there's no change in behaviour, why is this change needed?
Will this introduce a breaking change if reporter and cwd are already declared?
Like in the following case:
const stream = run({
files: [join(testFixtures, 'default-behavior/test/random.cjs')],
reporter: join(testFixtures, 'custom-reporter.js'),
cwd: testFixtures,
});
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61554 +/- ##
==========================================
- Coverage 89.79% 88.90% -0.89%
==========================================
Files 672 672
Lines 203756 203759 +3
Branches 39169 39080 -89
==========================================
- Hits 182956 181154 -1802
- Misses 13135 14873 +1738
- Partials 7665 7732 +67
🚀 New features to boost your workflow:
|
Capture process state in main/test_runner.js and pass it down to run() as options, so the public run() API doesn't depend on process state during execution. Fixes: nodejs#53867
batjko
force-pushed
the
fix/test-runner-no-process-state
branch
from
6867bc5 to
5b2fcfa
Compare
|
Sorry I just fixed a missed lint issue and pulled in latest main. Hope it's still ok? |
3 participants
Captures process state in
main/test_runner.jsand passes it torun()as options, so the public API doesn't read from process during execution.globPatternsinstead ofprocess.argvingetRunArgs()cwdthrough the call chain togetReportersMap()Fixes: #53867