test: preserve env in test cases#14822

BethGriggs · 2017-08-14T11:50:33Z

Allows env vars to be passed through to child processes. This is needed for
things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library.

Refs: #13390

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test

/cc @Trott

tniessen · 2017-08-14T11:53:15Z

cc @nodejs/testing

gibfahn · 2017-08-14T11:57:53Z

test/parallel/test-env-var-no-warnings.js

Maybe do this to be more explicit:
functiontest(newEnv){constenv=Object.assign({},process.env,newEnv);

refack · 2017-08-14T13:30:40Z

test/sequential/test-benchmark-child-process.js

Nit: I'd rewrap this as:
constchild=fork(runjs,['--set','dur=0','--set','n=1','--set','len=1','--set','params=1','--set','methodName=execSync','child_process'],{ env });

Not really related to this PR, but if @BethGriggs wants to do that that's fine too.

refack · 2017-08-14T13:34:33Z

7 uses in 7 files in my book calls for a common utility or even a new endpoint in child_process. But that is completely up to you @BethGriggs

refack · 2017-08-14T13:44:02Z

Ref: #14823

gibfahn · 2017-08-14T13:50:27Z

7 uses in 7 files in my book calls for a common utility or even a new endpoint in child_process. But that is completely up to you @BethGriggs

Agreed, but that's definitely not something for this PR (we'd have to decide which one first, and it'd be good to land this to fix these test failures ASAP, they don't fail in CI, but do on machines with long $WORKSPACE paths).

Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. Refs: nodejs#13390

mhdawson

LGTM

refack

💯

refack · 2017-08-14T14:49:43Z

@gibfahn I've added the confirmed-bug label. We can agree that PRs labeled as such should avoid "scope-creep".

addaleax

LGTM, thank you!

I don’t think “confirmed-bug” makes any sense here.

If you don’t want scope creep, not requesting changes that would extend scope sounds sufficient to me?

gibfahn · 2017-08-14T15:28:26Z

I don’t think “confirmed-bug” makes any sense here.

I'd say this is a bug in the tests, not in the source, so assuming that confirmed-bug is for the latter then I agree it doesn't apply.

refack · 2017-08-14T15:36:34Z

I don’t think “confirmed-bug” makes any sense here.
If you don’t want scope creep, not requesting changes that would extend scope sounds sufficient to me?

I made scope-creep suggestions since I was not aware that this is a directed bug-fix. Thus I'm looking for a simple way to flag PRs as such, so future reviewers will be aware.
Since confirmed-bug doesn't have meaning for PRs (yet) it seemed like the easiest choice. Other options could be a new label.

gibfahn · 2017-08-16T13:05:52Z

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

Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. PR-URL: #14822 Refs: #13390 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

tniessen · 2017-08-16T15:19:48Z

Landed in c879c9a, thank you!

Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. PR-URL: nodejs/node#14822 Refs: nodejs/node#13390 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14845Fixes: #14823 Refs: #14822 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: nodejs/node#14845Fixes: nodejs/node#14823 Refs: nodejs/node#14822 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. PR-URL: #14822 Refs: #13390 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14845Fixes: #14823 Refs: #14822 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. PR-URL: #14822 Refs: #13390 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14845Fixes: #14823 Refs: #14822 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. PR-URL: #14822 Refs: #13390 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: #14845Fixes: #14823 Refs: #14822 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: nodejs#14845 Backport-PR-URL: nodejs#15557Fixes: nodejs#14823 Refs: nodejs#14822 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

PR-URL: #14845 Backport-PR-URL: #15557Fixes: #14823 Refs: #14822 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rich Trott <[email protected]>

gibfahn · 2018-02-20T14:58:35Z

@BethGriggs would you mind backporting this to 6.x? Guide is here.

Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. PR-URL: nodejs#14822 Refs: nodejs#13390 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. Backport-PR-URL: #18883 PR-URL: #14822 Refs: #13390 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Aug 14, 2017

gibfahn reviewed Aug 14, 2017
View reviewed changes

BethGriggs force-pushed the preserve-env-vars branch from 001c0b5 to 93c6d1bCompare August 14, 2017 12:19

cjihrig approved these changes Aug 14, 2017
View reviewed changes

refack approved these changes Aug 14, 2017
View reviewed changes

test: preserve env in test cases
21d36ac
Allows env vars to be passed through to child processes. This is needed for things like NODE_TEST_DIR or LD_LIBRARY_PATH if testing the shared library. Refs: nodejs#13390

BethGriggs force-pushed the preserve-env-vars branch from 93c6d1b to 21d36acCompare August 14, 2017 13:51

mhdawson approved these changes Aug 14, 2017
View reviewed changes

lpinca approved these changes Aug 14, 2017
View reviewed changes

refack approved these changes Aug 14, 2017
View reviewed changes

refack added the confirmed-bug Issues with confirmed bugs. label Aug 14, 2017

addaleax approved these changes Aug 14, 2017
View reviewed changes

refack mentioned this pull request Aug 14, 2017
FR: child_process.exec(cmd,{additionalEnv }) #14823
Closed

tniessen approved these changes Aug 14, 2017
View reviewed changes

gibfahn mentioned this pull request Aug 15, 2017
test: pass env through to child processes #14845
Closed
3 tasks

jasnell approved these changes Aug 16, 2017
View reviewed changes

tniessen closed this Aug 16, 2017

MylesBorins mentioned this pull request Sep 10, 2017
v8.5.0 proposal #15308
Merged

MylesBorins added the lts-watch-v6.x label Sep 20, 2017

gibfahn mentioned this pull request Sep 22, 2017
[6.x] test: remove envPlus, use Object.assign everywhere #15557
Closed
2 tasks

MylesBorins added the backport-requested-v6.x label Oct 16, 2017

BethGriggs mentioned this pull request Feb 20, 2018
[v6.x backport] test: preserve env in test cases #18883
Closed
2 tasks

BethGriggs deleted the preserve-env-vars branch February 21, 2018 15:36

This was referenced Feb 27, 2018
v6.13.1 proposal #19025
Closed
v6.13.1 proposal #19027
Merged

Uh oh!

test: preserve env in test cases#14822

test: preserve env in test cases #14822

Uh oh!

Conversation

BethGriggs commented Aug 14, 2017

Checklist

Affected core subsystem(s)

Uh oh!

tniessen commented Aug 14, 2017

Uh oh!

gibfahnAug 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refackAug 14, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahnAug 14, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 14, 2017

Uh oh!

refack commented Aug 14, 2017

Uh oh!

gibfahn commented Aug 14, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 14, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Aug 14, 2017

Uh oh!

refack commented Aug 14, 2017

Uh oh!

gibfahn commented Aug 16, 2017

Uh oh!

tniessen commented Aug 16, 2017

Uh oh!

gibfahn commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

gibfahnAug 14, 2017•
edited
Loading