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

@@ -7,6 +7,7 @@ if (process.argv[2] === 'child') {
  process.emitWarning('foo');
 } else {
  function test(env) {
+    env = Object.assign({}, process.env, env);


Maybe do this to be more explicit:

function test(newEnv) { const env = Object.assign({}, process.env, newEnv);

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

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

@@ -8,13 +8,16 @@ const path = require('path');

 const runjs = path.join(__dirname, '..', '..', 'benchmark', 'run.js');

+const env = Object.assign({}, process.env,
+                          { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 });
+
 const child = fork(runjs, ['--set', 'dur=0',


Nit: I'd rewrap this as:

const child = 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: #14845 Fixes: #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#14845 Fixes: 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: #14845 Fixes: #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: #14845 Fixes: #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: #14845 Fixes: #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#15557 Fixes: 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: #15557 Fixes: #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 93c6d1b Compare 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 21d36ac Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: preserve env in test cases #14822

test: preserve env in test cases #14822

BethGriggs commented Aug 14, 2017

tniessen commented Aug 14, 2017

gibfahn Aug 14, 2017 •

edited

Loading

refack Aug 14, 2017

gibfahn Aug 14, 2017

refack commented Aug 14, 2017

refack commented Aug 14, 2017

gibfahn commented Aug 14, 2017

mhdawson left a comment

refack left a comment

refack commented Aug 14, 2017

addaleax left a comment

gibfahn commented Aug 14, 2017

refack commented Aug 14, 2017

gibfahn commented Aug 16, 2017

tniessen commented Aug 16, 2017

gibfahn commented Feb 20, 2018

test: preserve env in test cases #14822

test: preserve env in test cases #14822

Conversation

BethGriggs commented Aug 14, 2017

Checklist

Affected core subsystem(s)

tniessen commented Aug 14, 2017

gibfahn Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

refack Aug 14, 2017

Choose a reason for hiding this comment

gibfahn Aug 14, 2017

Choose a reason for hiding this comment

refack commented Aug 14, 2017

refack commented Aug 14, 2017

gibfahn commented Aug 14, 2017

mhdawson left a comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

refack commented Aug 14, 2017

addaleax left a comment

Choose a reason for hiding this comment

gibfahn commented Aug 14, 2017

refack commented Aug 14, 2017

gibfahn commented Aug 16, 2017

tniessen commented Aug 16, 2017

gibfahn commented Feb 20, 2018

gibfahn Aug 14, 2017 •

edited

Loading