test: pass env vars through to test-benchmark-http by gibfahn · Pull Request #13390 · nodejs/node

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

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

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 <joyeec9h3@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <joyeec9h3@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

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 <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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

Uh oh!

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

test: pass env vars through to test-benchmark-http#13390
gibfahn merged 1 commit intonodejs:masterfrom
gibfahn:bench-http-tmpdir

gibfahn commented Jun 2, 2017 •

edited

Loading

Uh oh!

gibfahn commented Jun 2, 2017

Uh oh!

refack left a comment

Uh oh!

refack Jun 2, 2017

Uh oh!

gibfahn Jun 2, 2017

Uh oh!

refack Jun 2, 2017

Uh oh!

refack Jun 2, 2017

Uh oh!

gibfahn Jun 2, 2017 •

edited

Loading

Uh oh!

gibfahn commented Jun 2, 2017

Uh oh!

Trott left a comment

Uh oh!

gibfahn commented Jun 3, 2017

Uh oh!

mhdawson left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

gibfahn commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

gibfahn commented Jun 2, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack Jun 2, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn Jun 2, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jun 2, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jun 2, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn Jun 2, 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!

gibfahn commented Jun 2, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

gibfahn commented Jun 3, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

gibfahn commented Jun 2, 2017 •

edited

Loading

gibfahn Jun 2, 2017 •

edited

Loading