-
-
Notifications
You must be signed in to change notification settings - Fork 34.5k
test_runner: add env option to run function #61367
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
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61367 +/- ##
==========================================
+ Coverage 88.51% 89.85% +1.33%
==========================================
Files 704 671 -33
Lines 208739 203176 -5563
Branches 40274 39052 -1222
==========================================
- Hits 184770 182560 -2210
+ Misses 15968 12971 -2997
+ Partials 8001 7645 -356
🚀 New features to boost your workflow:
|
724b84e to
cd4d84e
Compare
lib/internal/test_runner/runner.js
Outdated
| const args = getRunArgs(path, opts); | ||
| const stdio = ['pipe', 'pipe', 'pipe']; | ||
| const env = { __proto__: null, ...process.env, NODE_TEST_CONTEXT: 'child-v8' }; | ||
| const env = opts.env || { __proto__: null, ...process.env, NODE_TEST_CONTEXT: 'child-v8' }; |
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.
NODE_TEST_CONTEXT is used by the test runner to identify the runner subprocesses, and removing it may cause issues with the reporters.
It must be preserved even if the user passes opts.env.
I would suggest, in any case, adding a snapshot test when using this option to cover the E2E once the option has been provided!
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.
Okay, I'm going to switch this line to this: const env = { __proto__: null, NODE_TEST_CONTEXT: 'child-v8', ...(opts.env || process.env) }; so that its configured by default, but is overridable if someone wanted to. I believe currently there is a subtle bug here where even if you specified the NODE_TEST_CONTEXT variable in the main process, it'd be overridden to child-v8 no matter what.
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.
I don't fully understand the ask here:
I would suggest, in any case, adding a snapshot test when using this option to cover the E2E once the option has been provided!
Snapshot of what? The environment variables? Do you mean for me to just assert that something such as NODE_TEST_CONTEXT is retained when env option is specified, or something more in depth?
doc/api/test.md
Outdated
| * `functionCoverage` {number} Require a minimum percent of covered functions. If code | ||
| coverage does not reach the threshold specified, the process will exit with code `1`. | ||
| **Default:** `0`. | ||
| * `env` {Object} Specify environment variables to be passed along to the test process. This options is not compatible with `isolation='none'`. |
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.
I think it might be worth specifying here that these variables will override those from the main process
cd4d84e to
315e0bb
Compare
| * `functionCoverage` {number} Require a minimum percent of covered functions. If code | ||
| coverage does not reach the threshold specified, the process will exit with code `1`. | ||
| **Default:** `0`. | ||
| * `env` {Object} Specify environment variables to be passed along to the test process. |
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.
env vars are supposed to be inherited, what is the usecase here?
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.
They still are by default; this feature is essentially just bubbling up the underlying child_process.spawn() env option.
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.
If you are using the run() method multiple times to spawn multiple test processes. And want to be able to specify unique environment variables for each; there currently is no way to do so.
| const args = getRunArgs(path, opts); | ||
| const stdio = ['pipe', 'pipe', 'pipe']; | ||
| const env = { __proto__: null, ...process.env, NODE_TEST_CONTEXT: 'child-v8' }; | ||
| const env = { __proto__: null, NODE_TEST_CONTEXT: 'child-v8', ...(opts.env || process.env) }; |
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.
| const env = { __proto__: null, NODE_TEST_CONTEXT: 'child-v8', ...(opts.env || process.env) }; | |
| const env = { __proto__: null, NODE_TEST_CONTEXT: 'child-v8', ...(opts.env ?? process.env) }; |
nitpic
|
Hey @Ethan-Arrowood, while checking the CI, I noticed failures related to these changes. |
|
The only one that isn't a flake or build issue is Linux Containered: https://ci.nodejs.org/job/node-test-commit-linux-containered/54320/ I'm not familiar with this environment, but I will try to take a look. |
|
I am having a hard time understanding why this test is failing on these environments. I'm inspecting the stack trace in Jenkins and its not sharing anything useful: All this stack is telling me is that the Locally, if I add a line like: Does anyone have a better idea here? |
|
Is the issue that we are not inheriting anything from So following this idea... could the |
315e0bb to
c7e2bd6
Compare
|
I prompted AI with that idea and it came up with something by looking at other tests and things. Here is a summary from what it figured out. The Fix: Added Why this approach:
This reasoning makes sense to me, but please correct if it was wrong. |
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - test_runner: add env option to run function ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/21179244874 |
|
Those debugger tests on the macOS action are not failing for me locally 🤔 That has to be flakiness right? |
yes that seems just a flakiness issue! |
Support an `env` option that is passed to the underlying child_process. Fixes: nodejs#60709
c7e2bd6 to
2ce1c30
Compare
|
Only flake in the CI 🎉 This is ready to merge I think 🚀 |
|
I think we could backport this feature to all active release lines, right? Nothing at all breaking here. |
Support an
envoption that is passed to theunderlying child_process.
Fixes: #60709