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?

I think @cjihrig was suggesting using this approach

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

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

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

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

LGTM

lgtm will land tomorrow

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

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

I've been seeing that test fail more often

Pretty sure that test isn't related: #3193

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 })

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

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.

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

Landed in a9d42e0