test: pass process.env to child processes

[rvagg]

Primarily for testing when using LD_LIBRARY_PATH and DYLD_LIBRARY_PATH, these are a few newer instances that have been added since someone last tried to do this. Found while testing OpenSSL 1.1.0 dynamic linking for #16130.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

test

[gibfahn]

these are a few newer instances that have been added since someone last tried to do this.

That was a month ago (#15557), so really looking forward to having this tested in CI.

[rvagg]

https://ci.nodejs.org/job/node-test-commit/13393/

ignore the lint failure there, I messed up some stuff in benchmark/_http-benchmarkers.js that I've force-pushed fixes for since submitting the job.

[rvagg]

@gibfahn I suspect we might find even more instances once we start testing zlib, cares and others as dynamic non-globals.

[cjihrig]

LGTM with a question.

[cjihrig]

Aren't the { env: process.env } changes unnecessary?

[rvagg]

@cjihrig in theory yes, for -h in here you just want them to run, but I figured that we don't control how these applications execute and for all we know they are linked to (or load) crypto/openssl or some other dependency and might fail even for a simple -h. Imagine a custom build of wrk that's dynamically linked to a custom library and needs a path in LD_LIBRARY_PATH.

I could take these out but it seems appropriate to me to just inherit the environment in all of our external tool exec in the test suite.

[cjihrig]

I totally agree that the tools should inherit the environment. What I meant is that the environment is inherited by default, unless you set values for the env option, so { env: process.env } is redundant. I don't mind it either way.

Side note, and possibly a good first contribution: the child_process docs could do a better job pointing out that process.env is the default. Right now, you have to read a lot of text to see that. IMO, it should be part of the option description, like it is for most of the other options.

[rvagg]

@cjihrig gotcha, so I've gone overkill here and it's only the single fork() that needs the adjustment, will remove these, thanks for pointing this out

[rvagg]

Failing across windows:

not ok 486 sequential/test-benchmark-http
  ---
  duration_ms: 1.752
  severity: fail
  stack: |-
    
    http\bench-parser.js
    http\bench-parser.js n=1 len=1: 6,983.240223463687
    
    http\check_invalid_header_char.js
    http\check_invalid_header_char.js n=1 key="\"\"": 5,724.131219984087
    
    http\check_is_http_token.js
    http\check_is_http_token.js n=1 key="\"\"": 5,966.587112171837
    
    http\chunked.js
    _http_client.js:86
          throw new errors.Error('ERR_INVALID_DOMAIN_NAME');
          ^
    
    Error [ERR_INVALID_DOMAIN_NAME]: Unable to determine the domain name
        at new ClientRequest (_http_client.js:86:13)
        at request (http.js:38:10)
        at Object.get (http.js:42:13)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\benchmark\_test-double-benchmarker.js:5:6)
        at Module._compile (module.js:596:30)
        at Object.Module._extensions..js (module.js:607:10)
        at Module.load (module.js:515:32)
        at tryModuleLoad (module.js:478:12)
        at Function.Module._load (module.js:470:3)
        at Function.Module.runMain (module.js:637:10)
    Error: test-double failed with 1.
        at ChildProcess.<anonymous> (c:\workspace\node-test-binary-windows\benchmark\_http-benchmarkers.js:229:16)
        at Object.onceWrapper (events.js:326:30)
        at emitTwo (events.js:135:13)
        at ChildProcess.emit (events.js:224:7)
        at maybeClose (internal/child_process.js:943:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:220:5)
    assert.js:45
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at ChildProcess.child.on (c:\workspace\node-test-binary-windows\test\common\benchmark.js:25:12)
        at emitTwo (events.js:135:13)
        at ChildProcess.emit (events.js:224:7)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:209:12)

This is related cause I changed this file. Something about the environment slipping in to cause DNS failures? I'm not sure about this one.

[rvagg]

fixed windows problem, it was the basic TestDouble benchmark that was using path as an environment variable passed to the child via fork() to use as a URL for an http client call... Getting conflicted with PATH on Windows I believe. I've updated that to use test_url instead and removed a couple of unused env vars while I was there.

PTAL @cjihrig @refack @gireeshpunathil @gibfahn and I'll get this landed

[rvagg]

https://ci.nodejs.org/job/node-test-commit/13892/ FYI, see also the green ticks down below

[gibfahn]

Still LGTM

[cjihrig]

Still LGTM

[gireeshpunathil]

LGTM, thanks!

[rvagg]

landed in 3b3ceaf