test: pass env through to child processes

[gibfahn]

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]

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

[gibfahn]

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]

As for the debate:

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.

[tniessen]

I think the word temporarily is misguiding here. The environment of the child process is modified for its entire lifetime, and the enviroment of the calling process is left unchanged.

[tniessen]

You could as well drop the quotes here and below.

[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]

Refs: recently landed #14822

[gibfahn]

@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]

@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]

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]

• 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.

[gibfahn]

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]

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.

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]

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]

@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]

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]

(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]

@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]

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]

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]

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]

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]

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.

[Trott]

LGTM

[BridgeAR]

Landed in 180f865

[gibfahn]

@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]

@gibfahn uh, you are right. My bad.

[MylesBorins]

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]

Backport in #15557