test: Fix `common.PIPE` to be process unique #14168

refack · 2017-07-11T14:05:30Z

Fix common.PIPE to be process unique
tiny bit of refactoring in adjacent lines

Fixes: #14128

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,stream,net

lpinca · 2017-07-11T15:49:54Z

test/common/index.js

-  }
-} else {
-  exports.PIPE = `${exports.tmpDir}/test.sock`;
+{


Is block scoping really needed?

Since the const are only used locally and there's a single exported side-effect (exports.PIPE = ...) IMHO it's cleaner.
Similar to the function setupFoo(); setupFoo(); approach in node_bootstap.js
Maybe even V8 will inline the consts ?

I like it, easier to see that the consts aren't used except in exports.PIPE.

refack · 2017-07-11T16:38:13Z

test/common/index.js

@@ -246,7 +246,7 @@ Object.defineProperty(exports, 'opensslCli', {get: function() {

 Object.defineProperty(exports, 'hasCrypto', {
  get: function() {
-    return process.versions.openssl ? true : false;
+    return Boolean(process.versions.openssl);


Not a huge fan of !! outside of minimal code competitions.
Willing to change if anyone has a strong opinion.

No strong opinion from me. I think it used to be preferred in hot paths for performance. No idea if that's still the case. Regardless, it's totally sensible to value readable code over performant code in test/common/index.js.

refack · 2017-07-11T17:37:45Z

Failures:

not ok 220 parallel/test-cluster-eaccess
  ---
  duration_ms: 0.218
  severity: fail
  stack: |-
    child listening
    child exiting
    PIPE should have been in use.
    master exited
    assert.js:48
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 'EADDRINUSE' === undefined
        at Worker.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-cluster-eaccess.js:48:12)
        at Worker.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:515:15)
        at emitTwo (events.js:130:20)
        at Worker.emit (events.js:213:7)
        at ChildProcess.Worker.process.on (internal/cluster/worker.js:28:12)
        at emitTwo (events.js:125:13)
        at ChildProcess.emit (events.js:213:7)
        at emit (internal/child_process.js:776:12)
        at _combinedTickCallback (internal/process/next_tick.js:141:11)
        at process._tickCallback (internal/process/next_tick.js:180:9)
  ...

not ok 1509 sequential/test-benchmark-http
  ---
  duration_ms: 3.632
  severity: fail
  stack: |-
    
    http/bench-parser.js
    http/bench-parser.js n=1 len=1: 10,362.694300518135
    
    http/check_invalid_header_char.js
    http/check_invalid_header_char.js n=1 key="\"\"": 9,291.521486643438
    
    http/check_is_http_token.js
    http/check_is_http_token.js n=1 key="\"\"": 9,632.797749778445
    
    http/chunked.js
    http/chunked.js c=1 len=1 n=1 benchmarker="test-double": 1
    
    http/client-request-body.js
    http/client-request-body.js method="write" len=1 type="asc" dur=0.1: 3,782.800838240719
    http/client-request-body.js method="end" len=1 type="asc" dur=0.1: 3,753.1900606189497
    http/client-request-body.js method="write" len=1 type="utf" dur=0.1: 4,350.617135040606
    http/client-request-body.js method="end" len=1 type="utf" dur=0.1: 4,771.837295122962
    http/client-request-body.js method="write" len=1 type="buf" dur=0.1: 4,070.584983768017
    http/client-request-body.js method="end" len=1 type="buf" dur=0.1: 4,119.2370015667975
    
    http/cluster.js
    http/cluster.js c=1 len=1 type="bytes" benchmarker="test-double": 1
    http/cluster.js c=1 len=1 type="buffer" benchmarker="test-double": 1
    
    http/create-clientrequest.js
    http/create-clientrequest.js n=1 len=1: 1,233.710396354139
    
    http/end-vs-write-end.js
    http/end-vs-write-end.js method="write" c=1 len=1 type="asc" benchmarker="test-double": 1
    http/end-vs-write-end.js method="end" c=1 len=1 type="asc" benchmarker="test-double": 1
    http/end-vs-write-end.js method="write" c=1 len=1 type="utf" benchmarker="test-double": 1
    http/end-vs-write-end.js method="end" c=1 len=1 type="utf" benchmarker="test-double": 1
    http/end-vs-write-end.js method="write" c=1 len=1 type="buf" benchmarker="test-double": 1
    http/end-vs-write-end.js method="end" c=1 len=1 type="buf" benchmarker="test-double": 1
    
    http/http_server_for_chunky_client.js
    events.js:182
          throw er; // Unhandled 'error' event
          ^
    
    Error: connect ENOENT /data/iojs/node-tmp/tmp.0/node-test.47355.sock
        at Object.exports._errnoException (util.js:1023:11)
        at exports._exceptionWithHostPort (util.js:1046:20)
        at PipeConnectWrap.afterConnect [as oncomplete] (net.js:1150:14)
    assert.js:48
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at ChildProcess.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/benchmark/http/http_server_for_chunky_client.js:43:10)
        at emitTwo (events.js:125:13)
        at ChildProcess.emit (events.js:213:7)
        at maybeClose (internal/child_process.js:929:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:212:5)
    assert.js:48
      throw new errors.AssertionError({
      ^
    
    AssertionError [ERR_ASSERTION]: 1 === 0
        at ChildProcess.child.on (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/sequential/test-benchmark-http.js:34:10)
        at emitTwo (events.js:125:13)
        at ChildProcess.emit (events.js:213:7)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:201:12)
  ...

gibfahn · 2017-07-11T23:25:36Z

For the commit message, maybe:

test: make common.PIPE process unique

Also includes a tiny bit of refactoring in adjacent lines.

Fixes: https://github.com/nodejs/node/issues/14128

refack · 2017-07-12T00:37:18Z

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

refack · 2017-07-12T01:39:28Z

Known flakes:
smartOS - #14177
Windows - #14178

refack · 2017-07-14T21:26:57Z

Pre land CI: https://ci.nodejs.org/job/node-test-pull-request/9121/

* includes a tiny bit of refactoring in adjacent lines. * fixes 1 test and 1 benchmark that depended on PIPE being constant. PR-URL: nodejs#14168 Fixes: nodejs#14128 Reviewed-By: James M Snell <[email protected]>

* includes a tiny bit of refactoring in adjacent lines. * fixes 1 test and 1 benchmark that depended on PIPE being constant. PR-URL: #14168 Fixes: #14128 Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2017-08-16T07:13:19Z

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

refack added wip Issues and PRs that are still a work in progress. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Jul 11, 2017

refack mentioned this pull request Jul 11, 2017

test: flaky - parallel/test-pipe-stream #14128

Closed

lpinca reviewed Jul 11, 2017

View reviewed changes

refack commented Jul 11, 2017

View reviewed changes

refack force-pushed the test-common-rotate-EPIPE branch from cb47882 to 9c9ecac Compare July 12, 2017 00:12

refack changed the title ~~test: tweak common~~ test: Fix common.PIPE to be process unique Jul 12, 2017

refack removed the wip Issues and PRs that are still a work in progress. label Jul 12, 2017

jasnell approved these changes Jul 14, 2017

View reviewed changes

test: make common.PIPE process unique

c34ae48

* includes a tiny bit of refactoring in adjacent lines. * fixes 1 test and 1 benchmark that depended on PIPE being constant. PR-URL: nodejs#14168 Fixes: nodejs#14128 Reviewed-By: James M Snell <[email protected]>

refack force-pushed the test-common-rotate-EPIPE branch from 9c9ecac to c34ae48 Compare July 14, 2017 23:36

refack merged commit c34ae48 into nodejs:master Jul 14, 2017

refack deleted the test-common-rotate-EPIPE branch July 14, 2017 23:38

addaleax mentioned this pull request Jul 18, 2017

v8.2.0 proposal #13744

Merged

refack mentioned this pull request Jul 24, 2017

[v8.x] test: improve the test common documentation #14454

Closed

MylesBorins added the dont-land-on-v6.x label Aug 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Fix `common.PIPE` to be process unique #14168

test: Fix `common.PIPE` to be process unique #14168

refack commented Jul 11, 2017 •

edited

Loading

lpinca Jul 11, 2017

refack Jul 11, 2017

gibfahn Jul 11, 2017

refack Jul 11, 2017

Trott Jul 11, 2017

refack commented Jul 11, 2017

gibfahn commented Jul 11, 2017 •

edited

Loading

refack commented Jul 12, 2017

refack commented Jul 12, 2017 •

edited

Loading

refack commented Jul 14, 2017

MylesBorins commented Aug 16, 2017

test: Fix common.PIPE to be process unique #14168

test: Fix common.PIPE to be process unique #14168

Conversation

refack commented Jul 11, 2017 • edited Loading

Checklist

Affected core subsystem(s)

lpinca Jul 11, 2017

Choose a reason for hiding this comment

refack Jul 11, 2017

Choose a reason for hiding this comment

gibfahn Jul 11, 2017

Choose a reason for hiding this comment

refack Jul 11, 2017

Choose a reason for hiding this comment

Trott Jul 11, 2017

Choose a reason for hiding this comment

refack commented Jul 11, 2017

gibfahn commented Jul 11, 2017 • edited Loading

refack commented Jul 12, 2017

refack commented Jul 12, 2017 • edited Loading

refack commented Jul 14, 2017

MylesBorins commented Aug 16, 2017

test: Fix `common.PIPE` to be process unique #14168

test: Fix `common.PIPE` to be process unique #14168

refack commented Jul 11, 2017 •

edited

Loading

gibfahn commented Jul 11, 2017 •

edited

Loading

refack commented Jul 12, 2017 •

edited

Loading