FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: pass process.env to child processes by rvagg · Pull Request #16405 · nodejs/node · GitHub

/ node Public

test: pass process.env to child processes - #16405

Closed
rvagg wants to merge 2 commits into
nodejs:masterfrom
rvagg:rvagg/test-pass-env
ConversationCommits (2)ChecksFiles changed
Closed

test: pass process.env to child processes#16405
rvagg wants to merge 2 commits into
nodejs:masterfrom
rvagg:rvagg/test-pass-env

Conversation

rvagg commented Oct 23, 2017

Copy link
Copy Markdown
Member

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 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
Closed
3 tasks
rvagg force-pushed the rvagg/test-pass-env branch from aa8ebe9 to e49fd4a Compare October 23, 2017 11:33

gibfahn commented Oct 23, 2017

Copy link
Copy Markdown
Member

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 force-pushed the rvagg/test-pass-env branch from e49fd4a to 6078c6c Compare October 23, 2017 11:46

rvagg commented Oct 23, 2017

Copy link
Copy Markdown
Member Author

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 commented Oct 23, 2017

Copy link
Copy Markdown
Member Author

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

cjihrig approved these changes Oct 23, 2017
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with a question.

Comment thread benchmark/_http-benchmarkers.js Outdated

cjihrig Oct 23, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

rvagg Oct 23, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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 Oct 23, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 Nov 9, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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 commented Oct 23, 2017

Copy link
Copy Markdown
Member Author

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 added 2 commits November 9, 2017 23:56
test: pass process.env to child processes
0905341 
PR-URL: nodejs#16405
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
!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

rvagg commented Nov 9, 2017

Copy link
Copy Markdown
Member Author

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 commented Nov 9, 2017

Copy link
Copy Markdown
Member Author

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

gibfahn approved these changes Nov 9, 2017
View reviewed changes

gibfahn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Still LGTM

cjihrig commented Nov 9, 2017

Copy link
Copy Markdown
Contributor

Still LGTM

gireeshpunathil commented Nov 9, 2017

Copy link
Copy Markdown
Member

LGTM, thanks!

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

rvagg commented Nov 9, 2017

Copy link
Copy Markdown
Member Author

landed in 3b3ceaf

rvagg added a commit that referenced this pull request Nov 9, 2017
test: pass process.env to child processes
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 <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
test: pass process.env to child processes
1ee6df9 
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 <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
evanlucas mentioned this pull request Nov 13, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
test: pass process.env to child processes
1522562 
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 <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
gibfahn mentioned this pull request Nov 21, 2017
Merged
richardlau mentioned this pull request Oct 9, 2018
Closed
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
gireeshpunathil gireeshpunathil approved these changes
+2 more reviewers
refack refack approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL