Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: pass process.env to child processes #16405

Closed
wants to merge 2 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Oct 23, 2017

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

@nodejs-github-bot 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 rvagg mentioned this pull request Oct 23, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Oct 23, 2017

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
Copy link
Member Author

rvagg commented Oct 23, 2017

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
Copy link
Member Author

rvagg commented Oct 23, 2017

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

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a question.

@@ -15,7 +15,8 @@ class AutocannonBenchmarker {
this.name = 'autocannon';
this.executable =
process.platform === 'win32' ? 'autocannon.cmd' : 'autocannon';
const result = child_process.spawnSync(this.executable, ['-h']);
const result =
child_process.spawnSync(this.executable, ['-h'], { env: process.env });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member Author

rvagg commented Oct 23, 2017

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
Copy link
Member Author

rvagg commented Nov 9, 2017

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
Copy link
Member Author

rvagg commented Nov 9, 2017

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

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Nov 9, 2017

Still LGTM

@gireeshpunathil
Copy link
Member

LGTM, thanks!

@rvagg rvagg closed this Nov 9, 2017
@rvagg rvagg deleted the rvagg/test-pass-env branch November 9, 2017 22:25
@rvagg
Copy link
Member Author

rvagg commented Nov 9, 2017

landed in 3b3ceaf

rvagg added a commit that referenced this pull request Nov 9, 2017
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]>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
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]>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants