test: pass process.env to child processes #16405

rvagg · 2017-10-23T09:11:30Z

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 · 2017-10-23T11:41:47Z

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 · 2017-10-23T11:47:31Z

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 · 2017-10-23T11:48:36Z

@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 · 2017-10-23T12:38:22Z

benchmark/_http-benchmarkers.js

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

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

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.

@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 · 2017-10-23T21:53:35Z

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.

PR-URL: nodejs#16405 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

rvagg · 2017-11-09T14:24:04Z

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 · 2017-11-09T14:24:31Z

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

gibfahn

Still LGTM

cjihrig · 2017-11-09T14:38:28Z

Still LGTM

gireeshpunathil · 2017-11-09T15:35:33Z

LGTM, thanks!

rvagg · 2017-11-09T22:25:49Z

landed in 3b3ceaf

For variables such as LD_LIBRARY_PATH and DYLD_LIBRARY_PATH that are needed for dynamically linked binaries PR-URL: #16405 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Oct 23, 2017

rvagg mentioned this pull request Oct 23, 2017

Support both OpenSSL 1.1.0 and 1.0.2 #16130

Closed

3 tasks

refack approved these changes Oct 23, 2017

View reviewed changes

rvagg force-pushed the rvagg/test-pass-env branch from aa8ebe9 to e49fd4a Compare October 23, 2017 11:33

gibfahn approved these changes Oct 23, 2017

View reviewed changes

gireeshpunathil approved these changes Oct 23, 2017

View reviewed changes

rvagg force-pushed the rvagg/test-pass-env branch from e49fd4a to 6078c6c Compare October 23, 2017 11:46

cjihrig approved these changes Oct 23, 2017

View reviewed changes

rvagg added 2 commits November 9, 2017 23:56

test: pass process.env to child processes

0905341

PR-URL: nodejs#16405 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

!squash don't use 'path' env var!

edffa1d

rvagg force-pushed the rvagg/test-pass-env branch from 6078c6c to edffa1d Compare November 9, 2017 13:39

gibfahn approved these changes Nov 9, 2017

View reviewed changes

rvagg closed this Nov 9, 2017

rvagg deleted the rvagg/test-pass-env branch November 9, 2017 22:25

evanlucas mentioned this pull request Nov 13, 2017

v9.2.0 proposal #16992

Merged

MylesBorins added the dont-land-on-v6.x label Nov 17, 2017

MylesBorins added the land-on-v8.x label Nov 17, 2017

gibfahn mentioned this pull request Nov 21, 2017

v8.9.2 proposal #17204

Merged

richardlau mentioned this pull request Oct 9, 2018

child_process: handling fork( path, undefined / null, obj ) #22416

Closed

4 tasks

Uh oh!

test: pass process.env to child processes #16405

test: pass process.env to child processes #16405

Uh oh!

Conversation

rvagg commented Oct 23, 2017

Checklist

Affected core subsystem(s)

Uh oh!

gibfahn commented Oct 23, 2017

Uh oh!

rvagg commented Oct 23, 2017

Uh oh!

rvagg commented Oct 23, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

rvagg Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

cjihrig Oct 23, 2017

Choose a reason for hiding this comment

Uh oh!

rvagg Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

rvagg commented Oct 23, 2017

Uh oh!

rvagg commented Nov 9, 2017

Uh oh!

rvagg commented Nov 9, 2017

Uh oh!

gibfahn left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Nov 9, 2017

Uh oh!

gireeshpunathil commented Nov 9, 2017

Uh oh!

rvagg commented Nov 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants