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: pass env through to child processes #14845

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@gibfahn
Member

gibfahn commented Aug 15, 2017

Add a helper function to provide an easy way to modify the
environment passed to child processes.

Fixes: #14823
Refs: #14822

The function itself is trivial, the main benefit to having it is that it gives an easy way to pass modified environments to child processes. There are a bunch of tests that don't pass the environment through to child processes, and the tests that do use a variety of methods. So ideally by having a common function we can standardise on this way of doing things.

An alternative would be to just replace all of these with Object.assign({}, process.env, additionalEnv).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@refack

refack approved these changes Aug 15, 2017

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 15, 2017

Member

Well I guess even that is already very useful.
Next, an ESLint rule ;)

Member

refack commented Aug 15, 2017

Well I guess even that is already very useful.
Next, an ESLint rule ;)

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 15, 2017

Member

I would say that @vsemozhetbyt 's syntax in #14823 (comment) looks really neat, it's a pity we can't start using it for a while.

child_process.exec(cmd, { env: { ...process.env, NEW_VAR: 1 } });
Member

gibfahn commented Aug 15, 2017

I would say that @vsemozhetbyt 's syntax in #14823 (comment) looks really neat, it's a pity we can't start using it for a while.

child_process.exec(cmd, { env: { ...process.env, NEW_VAR: 1 } });
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 15, 2017

Member

As for the debate:
image
If you take out 6 LOCs in the doc and 3 LOCs caused by wrapping, we get a net save of 6 lines, and potentially 25 hard to trace bugs eliminated.

Member

refack commented Aug 15, 2017

As for the debate:
image
If you take out 6 LOCs in the doc and 3 LOCs caused by wrapping, we get a net save of 6 lines, and potentially 25 hard to trace bugs eliminated.

@tniessen

I am not really in favor of the name, but I can't think of anything better.

Show outdated Hide outdated test/common/README.md
Show outdated Hide outdated test/parallel/test-crypto-fips.js
@Trott

This changes a bunch of tests from passing bare environments with just one or two variables set to passing robust environments that inherit tons of environment variables.

This potentially introduces vectors for failing tests, especially in people's local environments. (I suppose it also may fix local tests for some people. But do we have reason to believe this is a significant problem? Honest question, I have no idea.) I don't think those tests should be altered to include user-set environment variables unless there is a compelling reason.

For tests that already have code that duplicates the environment, sure this should not be an issue. But it's all those other cases that are modified here that make me uncomfortable.

@vsemozhetbyt

This comment has been minimized.

Show comment
Hide comment
@vsemozhetbyt

vsemozhetbyt Aug 16, 2017

Member

Refs: recently landed #14822

Member

vsemozhetbyt commented Aug 16, 2017

Refs: recently landed #14822

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 16, 2017

Member

@Trott you're right, but not having the local environment can also cause problems. The two I've noticed recently are NODE_TEST_DIR (for long paths) and LD_LIBRARY_PATH (for shared library builds), but I'm sure there are others.

Basically we either have a blacklist or a whitelist. The nice thing about a common.envPlus() function is that it's easy to do that for all tests.

I'd rather have a blacklist (if necessary) than a whitelist, but open to suggestions.

For tests that already have code that duplicates the environment, sure this should not be an issue. But it's all those other cases that are modified here that make me uncomfortable.

Sure, but the reason those shouldn't be an issue isn't because of something about the tests themselves, it's just because they've been out in the wild for a while, so we'd hope people have run into any issues.

Member

gibfahn commented Aug 16, 2017

@Trott you're right, but not having the local environment can also cause problems. The two I've noticed recently are NODE_TEST_DIR (for long paths) and LD_LIBRARY_PATH (for shared library builds), but I'm sure there are others.

Basically we either have a blacklist or a whitelist. The nice thing about a common.envPlus() function is that it's easy to do that for all tests.

I'd rather have a blacklist (if necessary) than a whitelist, but open to suggestions.

For tests that already have code that duplicates the environment, sure this should not be an issue. But it's all those other cases that are modified here that make me uncomfortable.

Sure, but the reason those shouldn't be an issue isn't because of something about the tests themselves, it's just because they've been out in the wild for a while, so we'd hope people have run into any issues.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 17, 2017

Member

@gibfahn @tniessen do you think there's a place for a common.spawn as a broader solution? Maybe implementing some sort of throttling, or a heuristic that uses vm, or a hook for nyc (the cover tool)...
/cc @nodejs/testing

Member

refack commented Aug 17, 2017

@gibfahn @tniessen do you think there's a place for a common.spawn as a broader solution? Maybe implementing some sort of throttling, or a heuristic that uses vm, or a hook for nyc (the cover tool)...
/cc @nodejs/testing

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 17, 2017

Member

But do we have reason to believe this is a significant problem? Honest question, I have no idea.) I don't think those tests should be altered to include user-set environment variables unless there is a compelling reason.

@Trott see #14822 & #13390

Member

refack commented Aug 17, 2017

But do we have reason to believe this is a significant problem? Honest question, I have no idea.) I don't think those tests should be altered to include user-set environment variables unless there is a compelling reason.

@Trott see #14822 & #13390

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 17, 2017

Member
  • There's an issue with testing node-x32 on Windows-x64 (what's called WOW64) that requires the %SystemRoot% variable in cases that depend on dlls.
  • I'm assuming $LC_ALL might have some effect on results in POSIX systems.
  • %ComSpec% is required by WIN32 in every valid environment.

@Trott question IMHO actually strengthens the need for an abstracted environment-cloning method (over { env: { ...process.env, NEW_VAR: 1 } }) that can implement a white/black list of variables.

Member

refack commented Aug 17, 2017

  • There's an issue with testing node-x32 on Windows-x64 (what's called WOW64) that requires the %SystemRoot% variable in cases that depend on dlls.
  • I'm assuming $LC_ALL might have some effect on results in POSIX systems.
  • %ComSpec% is required by WIN32 in every valid environment.

@Trott question IMHO actually strengthens the need for an abstracted environment-cloning method (over { env: { ...process.env, NEW_VAR: 1 } }) that can implement a white/black list of variables.

test: add common.envPlus()
Add a helper function to provide an easy way to modify the
environment passed to child processes.

Fixes: #14823
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 17, 2017

Member

do you think there's a place for a common.spawn as a broader solution?

I don't see the need right now, but maybe, if there's something that you think should be abstracted. I think the spawns in tests get called with a lot of different configurations, so I'm not sure whether it would be worth it.

Basically (for me) with common functions, there has to be enough of a reason to use them to outweigh having to remember them and look them up. That means it has to be easier to use the function than to reimplement it.

I was on the fence about this function before, the idea of being able to control blacklists or whitelists makes it a no-brainer for me. IDK about common.spawn, maybe modify a couple of files and open a PR.

Member

gibfahn commented Aug 17, 2017

do you think there's a place for a common.spawn as a broader solution?

I don't see the need right now, but maybe, if there's something that you think should be abstracted. I think the spawns in tests get called with a lot of different configurations, so I'm not sure whether it would be worth it.

Basically (for me) with common functions, there has to be enough of a reason to use them to outweigh having to remember them and look them up. That means it has to be easier to use the function than to reimplement it.

I was on the fence about this function before, the idea of being able to control blacklists or whitelists makes it a no-brainer for me. IDK about common.spawn, maybe modify a couple of files and open a PR.

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 17, 2017

Member

I was on the fence about this function before, the idea of being able to control blacklists or whitelists makes it a no-brainer for me. IDK about common.spawn, maybe modify a couple of files and open a PR.

I just have an intuition. I'll do an audit.

Member

refack commented Aug 17, 2017

I was on the fence about this function before, the idea of being able to control blacklists or whitelists makes it a no-brainer for me. IDK about common.spawn, maybe modify a couple of files and open a PR.

I just have an intuition. I'll do an audit.

@Trott Trott dismissed their stale review Aug 17, 2017

I'm almost never a fan of adding another abstraction that only exists in common.js for people to have to learn to use. But people have clearly thought about this, so I'm dismissing my request for changes.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Aug 27, 2017

Member

I think it would be much better to just use Object.assign directly if necessary and not to add another common function. These functions increase the difficulty for everyone new and if they do not bring much benefit I am against adding them. Another negative point is that you can also not just copy a single test case into the repl with common syntax and this is often a fast way to test a special case in a big test file. A plain JS version is therefore always preferred by me and I would rather not land this.

Member

BridgeAR commented Aug 27, 2017

I think it would be much better to just use Object.assign directly if necessary and not to add another common function. These functions increase the difficulty for everyone new and if they do not bring much benefit I am against adding them. Another negative point is that you can also not just copy a single test case into the repl with common syntax and this is often a fast way to test a special case in a big test file. A plain JS version is therefore always preferred by me and I would rather not land this.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 27, 2017

Member

@BridgeAR

I think it would be much better to just use Object.assign directly if necessary and not to add another common function. These functions increase the difficulty for everyone new and if they do not bring much benefit I am against adding them.

As I said earlier, I'm not completely sold on this either. As it stands this just unifies the syntax and stops tests breaking because they don't pass down the existing environment to child processes. However if we need to blacklist/whitelist environment variables that cause problems with people's tests going forward, then having this function will be necessary.

Another negative point is that you can also not just copy a single test case into the repl with common syntax and this is often a fast way to test a special case in a big test file.

Does that work often? You can just paste the middle of a test in without needing the initial setup/require stuff? In any case, pasting the require('../common') line, which is already included in the setup for every test, is all you need, and it's persistent across tests so only needed once. I haven't personally ever found this a problem, so I don't really get why it would be painful.

FWIW, of the 1547 tests in parallel/, 1151 declare a const common (i.e. actually use common), and 396 don't.

Member

gibfahn commented Aug 27, 2017

@BridgeAR

I think it would be much better to just use Object.assign directly if necessary and not to add another common function. These functions increase the difficulty for everyone new and if they do not bring much benefit I am against adding them.

As I said earlier, I'm not completely sold on this either. As it stands this just unifies the syntax and stops tests breaking because they don't pass down the existing environment to child processes. However if we need to blacklist/whitelist environment variables that cause problems with people's tests going forward, then having this function will be necessary.

Another negative point is that you can also not just copy a single test case into the repl with common syntax and this is often a fast way to test a special case in a big test file.

Does that work often? You can just paste the middle of a test in without needing the initial setup/require stuff? In any case, pasting the require('../common') line, which is already included in the setup for every test, is all you need, and it's persistent across tests so only needed once. I haven't personally ever found this a problem, so I don't really get why it would be painful.

FWIW, of the 1547 tests in parallel/, 1151 declare a const common (i.e. actually use common), and 396 don't.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Aug 27, 2017

Member

It depends on the test if it works or not. I just did that a couple of times for small tests. It works fine for those most of the time as they often do not have any further requirements (and common is not always required for a test even if it is required in the test file).

Member

BridgeAR commented Aug 27, 2017

It depends on the test if it works or not. I just did that a couple of times for small tests. It works fine for those most of the time as they often do not have any further requirements (and common is not always required for a test even if it is required in the test file).

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 27, 2017

Member

(and common is not always required for a test even if it is required in the test file).

Sure, that was my point about the 1151 (have requirements) vs 396 (don't).

It's required if it's declared with const common = require('../common');, and not if it's declared with require('../common');. The linter requires that you use the right one (it'd detailed in the test guide).

Member

gibfahn commented Aug 27, 2017

(and common is not always required for a test even if it is required in the test file).

Sure, that was my point about the 1151 (have requirements) vs 396 (don't).

It's required if it's declared with const common = require('../common');, and not if it's declared with require('../common');. The linter requires that you use the right one (it'd detailed in the test guide).

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Aug 28, 2017

Member

@gibfahn I am not sure if we are talking about the same thing but it was not a important point for me anyway. Nevertheless I am still not a fan and I would rather not land this but I will also not stand in the way if other push for it.

Member

BridgeAR commented Aug 28, 2017

@gibfahn I am not sure if we are talking about the same thing but it was not a important point for me anyway. Nevertheless I am still not a fan and I would rather not land this but I will also not stand in the way if other push for it.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 28, 2017

Member

Went back through the tests again, and I tend to agree that the mental complexity of adding another helper method isn't worth it for this, so I've removed it, PTAL.

Member

gibfahn commented Aug 28, 2017

Went back through the tests again, and I tend to agree that the mental complexity of adding another helper method isn't worth it for this, so I've removed it, PTAL.

@gibfahn gibfahn changed the title from test: add common.envPlus() to test: pass env through to child processes Aug 28, 2017

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Aug 30, 2017

Member

I personally would like @Trott to have the last word about this.

Member

BridgeAR commented Aug 30, 2017

I personally would like @Trott to have the last word about this.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 30, 2017

Member

@Trott thoughts?

Member

gibfahn commented Aug 30, 2017

@Trott thoughts?

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Aug 31, 2017

Member

I don't have a strong opinion on the approach itself (not 100% convinced).

Of what? As I've removed the common helper, there isn't really an approach here. Half the tests pass the environment through, half don't. As we need the environment to be passed through, I've made them all do it. Happy to discuss...

Member

gibfahn commented Aug 31, 2017

I don't have a strong opinion on the approach itself (not 100% convinced).

Of what? As I've removed the common helper, there isn't really an approach here. Half the tests pass the environment through, half don't. As we need the environment to be passed through, I've made them all do it. Happy to discuss...

@tniessen

This comment has been minimized.

Show comment
Hide comment
@tniessen

tniessen Aug 31, 2017

Member

I simply don't have a strong opinion on passing the environment around, mainly because I personally did not have any issues not doing that, but I understand the associated problems. I still think @Trott has a point (referring to his first review), but we can think about that once problems arise (if ever). No discussion necessary, as I said, the changes LGTM.

Member

tniessen commented Aug 31, 2017

I simply don't have a strong opinion on passing the environment around, mainly because I personally did not have any issues not doing that, but I understand the associated problems. I still think @Trott has a point (referring to his first review), but we can think about that once problems arise (if ever). No discussion necessary, as I said, the changes LGTM.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2017

Member

I believe @Trott may be taking a bit of personal time this week so might need to give him another day or two to respond.

Member

jasnell commented Aug 31, 2017

I believe @Trott may be taking a bit of personal time this week so might need to give him another day or two to respond.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Aug 31, 2017

Member

Well as we already pass through the env to a lot of tests I guess it does not change a lot to do that with the other ones as well. If someone runs into issues we have to think about a new solution anyway. So I withdraw my last concerns and I am OK with landing this as is.

Member

BridgeAR commented Aug 31, 2017

Well as we already pass through the env to a lot of tests I guess it does not change a lot to do that with the other ones as well. If someone runs into issues we have to think about a new solution anyway. So I withdraw my last concerns and I am OK with landing this as is.

@refack refack referenced this pull request Sep 2, 2017

Closed

test, benchmark: create shared runBenchmark func #15004

4 of 6 tasks complete
@Trott

Trott approved these changes Sep 2, 2017

LGTM

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 3, 2017

Member

Landed in 180f865

Member

BridgeAR commented Sep 3, 2017

Landed in 180f865

@BridgeAR BridgeAR closed this Sep 3, 2017

BridgeAR added a commit that referenced this pull request Sep 3, 2017

test: remove envPlus, use Object.assign everywhere
PR-URL: #14845
Fixes: #14823
Refs: #14822
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@gibfahn gibfahn deleted the gibfahn:envPlus branch Sep 4, 2017

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Sep 4, 2017

Member

@BridgeAR thanks for landing.

For posterity the commit message looks a bit odd, as it says remove envPlus, but that was added in the first commit, so there was nothing to remove in master.

Member

gibfahn commented Sep 4, 2017

@BridgeAR thanks for landing.

For posterity the commit message looks a bit odd, as it says remove envPlus, but that was added in the first commit, so there was nothing to remove in master.

@BridgeAR

This comment has been minimized.

Show comment
Hide comment
@BridgeAR

BridgeAR Sep 4, 2017

Member

@gibfahn uh, you are right. My bad.

Member

BridgeAR commented Sep 4, 2017

@gibfahn uh, you are right. My bad.

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 5, 2017

test: remove envPlus, use Object.assign everywhere
PR-URL: nodejs/node#14845
Fixes: nodejs/node#14823
Refs: nodejs/node#14822
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

test: remove envPlus, use Object.assign everywhere
PR-URL: #14845
Fixes: #14823
Refs: #14822
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 11, 2017

test: remove envPlus, use Object.assign everywhere
PR-URL: #14845
Fixes: #14823
Refs: #14822
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 12, 2017

test: remove envPlus, use Object.assign everywhere
PR-URL: #14845
Fixes: #14823
Refs: #14822
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Sep 20, 2017

Member

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Member

MylesBorins commented Sep 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Sep 22, 2017

Member

Backport in #15557

Member

gibfahn commented Sep 22, 2017

Backport in #15557

gibfahn added a commit to gibfahn/node that referenced this pull request Sep 22, 2017

test: remove envPlus, use Object.assign everywhere
PR-URL: nodejs#14845
Backport-PR-URL: nodejs#15557
Fixes: nodejs#14823
Refs: nodejs#14822
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 22, 2017

test: remove envPlus, use Object.assign everywhere
PR-URL: #14845
Backport-PR-URL: #15557
Fixes: #14823
Refs: #14822
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 22, 2017

Merged

v6.11.4 proposal #15506

MylesBorins added a commit that referenced this pull request Sep 26, 2017

test: remove envPlus, use Object.assign everywhere
PR-URL: #14845
Backport-PR-URL: #15557
Fixes: #14823
Refs: #14822
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment