test: pass env vars through to test-benchmark-http #13390

gibfahn · 2017-06-02T09:58:37Z

Allows NODE_TEST_DIR to be set (necessary to avoid path length issues
with common.PIPE).

Refs: #12708 (comment)

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

cc/ @nodejs/benchmarking

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

gibfahn · 2017-06-02T10:00:43Z

If there are issues with this then I could just pass this variable through instead, but I don't think that including the whole environment is an issue, it's not as if we actually care about the benchmark's performance here.

diff --git a/test/sequential/test-benchmark-http.js b/test/sequential/test-benchmark-http.js
index cbeafcb8e6..a703d1b364 100644
--- a/test/sequential/test-benchmark-http.js
+++ b/test/sequential/test-benchmark-http.js
@@ -28,7 +28,8 @@ const child = fork(runjs, ['--set', 'benchmarker=test-double',
                            '--set', 'len=1',
                            '--set', 'n=1',
                            'http'],
-                   {env: {NODEJS_BENCHMARK_ZERO_ALLOWED: 1}});
+                   {env: {NODEJS_BENCHMARK_ZERO_ALLOWED: 1,
+                          NODE_TEST_DIR: process.env.NODE_TEST_DIR}});
 child.on('exit', (code, signal) => {
   assert.strictEqual(code, 0);
   assert.strictEqual(signal, null);

refack

1 nit

refack · 2017-06-02T12:18:21Z

test/sequential/test-benchmark-http.js

@@ -20,6 +20,9 @@ const path = require('path');

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

+const env = process.env;


maybe
const env = Object.assign({}, process.env, { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 })
For better hygiene

I find this less readable, so I'd be slightly -1 on this unless there's a benefit. Could you explain what you mean by code hygiene?

at least const env = Object.assign({}, process.env) makes it explicit that you are using a copy, and not mutating the current process's env

refack · 2017-06-02T12:19:31Z

test/sequential/test-benchmark-http.js

@@ -28,7 +31,7 @@ const child = fork(runjs, ['--set', 'benchmarker=test-double',
                           '--set', 'len=1',
                           '--set', 'n=1',
                           'http'],
-                   {env: {NODEJS_BENCHMARK_ZERO_ALLOWED: 1}});
+                   {env: env});


Do we have a thumb-rule/eslint-rule for shorthand properties?

No (at least the linter passes without).

I didn't realise you could use the shorthand props on Node 4, I do think it's nicer, will update.

Done

gibfahn · 2017-06-02T12:36:41Z

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

Trott

LGTM if CI is green. Thanks for improving this.

gibfahn · 2017-06-03T14:27:35Z

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

mhdawson

LGTM

Allows NODE_TEST_DIR to be set (necessary to avoid path length issues with common.PIPE). PR-URL: nodejs#13390 Refs: nodejs#12708 (comment) Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Allows NODE_TEST_DIR to be set (necessary to avoid path length issues with common.PIPE). PR-URL: #13390 Refs: #12708 (comment) Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[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. Refs: nodejs#13390

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]>

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]>

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]>

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]>

gibfahn added the test Issues and PRs related to the tests. label Jun 2, 2017

gibfahn requested review from Trott and joyeecheung June 2, 2017 09:58

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

joyeecheung approved these changes Jun 2, 2017

View reviewed changes

gibfahn force-pushed the bench-http-tmpdir branch from 9345ff9 to e12f8b0 Compare June 2, 2017 12:17

refack approved these changes Jun 2, 2017

View reviewed changes

refack reviewed Jun 2, 2017

View reviewed changes

refack added the benchmark Issues and PRs related to the benchmark subsystem. label Jun 2, 2017

gibfahn force-pushed the bench-http-tmpdir branch from e12f8b0 to f5c5410 Compare June 2, 2017 12:34

gibfahn force-pushed the bench-http-tmpdir branch from f5c5410 to b3b9253 Compare June 2, 2017 13:02

mscdex added the http Issues or PRs related to the http subsystem. label Jun 2, 2017

refack approved these changes Jun 2, 2017

View reviewed changes

cjihrig approved these changes Jun 2, 2017

View reviewed changes

Trott approved these changes Jun 2, 2017

View reviewed changes

jasnell approved these changes Jun 2, 2017

View reviewed changes

mhdawson approved these changes Jun 6, 2017

View reviewed changes

gibfahn force-pushed the bench-http-tmpdir branch from b3b9253 to eef94a8 Compare June 7, 2017 15:41

gibfahn merged commit eef94a8 into nodejs:master Jun 7, 2017

gibfahn deleted the bench-http-tmpdir branch June 7, 2017 16:00

gibfahn mentioned this pull request Jun 8, 2017

benchmark, test: test the HTTP benchmark with a dummy benchmarker #12121

Closed

3 tasks

gibfahn added dont-land-on-v4.x labels Jun 10, 2017

BethGriggs mentioned this pull request Aug 14, 2017

test: preserve env in test cases #14822

Closed

2 tasks

refack mentioned this pull request Aug 14, 2017

FR: child_process.exec(cmd, { additionalEnv }) #14823

Closed

refack mentioned this pull request Aug 17, 2017

test: pass env through to child processes #14845

Closed

3 tasks

BethGriggs mentioned this pull request Feb 20, 2018

[v6.x backport] test: preserve env in test cases #18883

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: pass env vars through to test-benchmark-http #13390

test: pass env vars through to test-benchmark-http #13390

gibfahn commented Jun 2, 2017 •

edited

Loading

gibfahn commented Jun 2, 2017

refack left a comment

refack Jun 2, 2017

gibfahn Jun 2, 2017

refack Jun 2, 2017

refack Jun 2, 2017

gibfahn Jun 2, 2017 •

edited

Loading

gibfahn commented Jun 2, 2017

Trott left a comment

gibfahn commented Jun 3, 2017

mhdawson left a comment

		@@ -20,6 +20,9 @@ const path = require('path');

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

		const env = process.env;

test: pass env vars through to test-benchmark-http #13390

test: pass env vars through to test-benchmark-http #13390

Conversation

gibfahn commented Jun 2, 2017 • edited Loading

Checklist

Affected core subsystem(s)

gibfahn commented Jun 2, 2017

refack left a comment

Choose a reason for hiding this comment

refack Jun 2, 2017

Choose a reason for hiding this comment

gibfahn Jun 2, 2017

Choose a reason for hiding this comment

refack Jun 2, 2017

Choose a reason for hiding this comment

refack Jun 2, 2017

Choose a reason for hiding this comment

gibfahn Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

gibfahn commented Jun 2, 2017

Trott left a comment

Choose a reason for hiding this comment

gibfahn commented Jun 3, 2017

mhdawson left a comment

Choose a reason for hiding this comment

gibfahn commented Jun 2, 2017 •

edited

Loading

gibfahn Jun 2, 2017 •

edited

Loading