-
-
Notifications
You must be signed in to change notification settings - Fork 34.6k
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
test_runner: add env option to run function #61367
Conversation
|
Review requested:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61367 +/- ##
==========================================
+ Coverage 88.51% 89.87% +1.36%
==========================================
Files 704 671 -33
Lines 208739 203319 -5420
Branches 40274 39095 -1179
==========================================
- Hits 184770 182742 -2028
+ Misses 15968 12926 -3042
+ Partials 8001 7651 -350
🚀 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.
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.
@Ethan-Arrowood The docs here are missing the default value.
I created a doc PR 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.
Good catch!
| 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. |
Support an `env` option that is passed to the underlying child_process. Fixes: nodejs#60709
2ce1c30 to
0eb68d1
Compare
|
Silly new line lol |
mcollina
left a comment
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.
lgtm
|
Landed in ec15c3b |
Support an `env` option that is passed to the underlying child_process. Fixes: #60709 PR-URL: #61367 Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Notable changes: async_hooks: * (SEMVER-MINOR) add trackPromises option to createHook() (Joyee Cheung) #61415 src: * improve textEncoder encode performance with simdutf (Mert Can Altin) #61496 stream: * (SEMVER-MINOR) add bytes() method to stream/consumers (wantaek) #60426 test_runner: * (SEMVER-MINOR) add env option to run function (Ethan Arrowood) #61367 url: * update ada to v3.4.2 and support unicode 17 (Yagiz Nizipli) #61593 PR-URL: #61635
Support an `env` option that is passed to the underlying child_process. Fixes: #60709 PR-URL: #61367 Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Chemi Atlow <chemi@atlow.co.il> Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Notable changes: async_hooks: * (SEMVER-MINOR) add `trackPromises` option to `createHook()` (Joyee Cheung) #61415 net: * (SEMVER-MINOR) add `setTOS` and `getTOS` to `Socket` (Amol Yadav) #61503 src: * (SEMVER-MINOR) add initial support for ESM in embedder API (Joyee Cheung) #61548 * improve `TextEncoder` encode performance with `simdutf` (Mert Can Altin) #61496 stream: * (SEMVER-MINOR) add `bytes()` method to `node:stream/consumers` (wantaek) #60426 test_runner: * (SEMVER-MINOR) add `env` option to `run` function (Ethan Arrowood) #61367 url: * update Ada to v3.4.2 and support Unicode 17 (Yagiz Nizipli) #61593 PR-URL: #61635
Notable changes: async_hooks: * (SEMVER-MINOR) add `trackPromises` option to `createHook()` (Joyee Cheung) #61415 net: * (SEMVER-MINOR) add `setTOS` and `getTOS` to `Socket` (Amol Yadav) #61503 src: * (SEMVER-MINOR) add initial support for ESM in embedder API (Joyee Cheung) #61548 * improve `TextEncoder` encode performance with `simdutf` (Mert Can Altin) #61496 stream: * (SEMVER-MINOR) add `bytes()` method to `node:stream/consumers` (wantaek) #60426 test_runner: * (SEMVER-MINOR) add `env` option to `run` function (Ethan Arrowood) #61367 url: * update Ada to v3.4.2 and support Unicode 17 (Yagiz Nizipli) #61593 PR-URL: #61635
Notable changes: async_hooks: * (SEMVER-MINOR) add `trackPromises` option to `createHook()` (Joyee Cheung) #61415 net: * (SEMVER-MINOR) add `setTOS` and `getTOS` to `Socket` (Amol Yadav) #61503 src: * (SEMVER-MINOR) add initial support for ESM in embedder API (Joyee Cheung) #61548 * improve `TextEncoder` encode performance with `simdutf` (Mert Can Altin) #61496 stream: * (SEMVER-MINOR) add `bytes()` method to `node:stream/consumers` (wantaek) #60426 test_runner: * (SEMVER-MINOR) add `env` option to `run` function (Ethan Arrowood) #61367 url: * update Ada to v3.4.2 and support Unicode 17 (Yagiz Nizipli) #61593 PR-URL: #61635
Support an
envoption that is passed to theunderlying child_process.
Fixes: #60709