Fix missing original env variable issue on multiple test cases.

[john-yan]

When the parent spawn the child processes, the environment variables passing into the child processes are missing the original env variables passing into the parent. Some missing variables like LD_LIBRARY_PATH cause the child processes unable to run.

PR-URL: #3183

[rvagg]

copied my comments from #3191

lgtm

I imagine the proper way to do this is with Object.assign() but since it's tests I'm not sure it matters, anyone else have an opinion on that?

[mhdawson]

I think @cjihrig was suggesting using this approach

env: util._extend(process.env, {NODE_DEBUG: process.argv[2]})

[john-yan]

@rvagg @cjihrig @mhdawson Hello, I don't mind to change to Object.assign, extend or something else, because it shouldn't make any difference from the testing perspective. But which one do you agree on?

[cjihrig]

I forgot we have Object.assign() now. If you can use that, that's probably the correct approach.

[john-yan]

@rvagg @cjihrig Redo commit with Object.assign()

[cjihrig]

I think you can drop the || {} now.

[john-yan]

Fixed.

[cjihrig]

Is HOME still required? Seems like it should get copied over now.

[john-yan]

Yeah, thanks, you are right. Home should be copied over. Fixed.

[cjihrig]

LGTM

[mhdawson]

lgtm will land tomorrow

[jasnell]

CI: https://ci.nodejs.org/job/node-test-pull-request/449/

[jasnell]

Seeing a CI failure on freebsd (https://ci.nodejs.org/job/node-test-commit-other/854/nodes=freebsd101-64/console) that may be related to this commit.

not ok 886 test-child-process-emfile.js
#
#assert.js:89
#  throw new assert.AssertionError({
#  ^
#AssertionError: 0 undefined null
#    at emitTwo (events.js:87:13)
#    at ChildProcess.emit (events.js:172:7)
#    at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)

@john-yan @mhdawson ... can you please investigate before landing in master. If landed in master in time, I'll pick it back to v4.x but not sure it'll make it for v4.2.0

[evanlucas]

I've been seeing that test fail more often

[jbergstroem]

Pretty sure that test isn't related: #3193

[LinusU]

Object.assign actually modifies the first argument, should we really send process.env there?

Maybe do this instead:

- Object.assign(process.env, { foo: expected })
+ Object.assign({}, process.env, { foo: expected })

[john-yan]

Hello @LinusU , looks like they wanted to extend process.env instead.

[john-yan]

Hello @jasnell , Given that the changes in this commit has no global effect and no change has been made to the failing test cases in the CI, I don't think it's related.

[jasnell]

Ok. Thank you for clarifying. Running one more local test then will land on master.

[jasnell]

Landed in a9d42e0