test: dynamic port in test cluster disconnect by sebastianplesciuc · Pull Request #12545 · nodejs/node

sebastianplesciuc · 2017-04-20T17:11:02Z

Removed common.PORT from test-cluster-disconnect to eliminate the
possibility that a dynamic port used in another test will collide
with common.PORT.

Refs: #12376

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

vsemozhetbyt · 2017-04-20T17:20:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7556/

sebastianplesciuc · 2017-04-20T18:06:04Z

Uhm. I'm not really sure why the test/windows-fanned build failed. Help?

refack · 2017-04-20T18:11:09Z

Uhm. I'm not really sure why the test/windows-fanned build failed. Help?

Not your fault, timeout issue. CI can be considered Green on Windows.

lpinca · 2017-04-22T07:04:28Z

test/parallel/test-cluster-disconnect.js

Shouldn't the second argument of splice be 1?

You're absolutely right. Thanks for pointing that out. I missed it somehow. I've pushed the changes.

lpinca · 2017-04-22T09:59:27Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7585/

lpinca · 2017-04-22T10:03:54Z

test/parallel/test-cluster-disconnect.js

Nit: if this change will not be backported to v4 we can use Array.prototype.includes() instead of Array.prototype.indexOf().

Please let me know if I should change it. It will also require a new CI build.

IMHO can land as is (found 433 hits to indexOf in /test/*.js)
@lpinca since v4 entered maintenance, do you think we can eliminate all 433 now (in a different PR)?

@refack yes, not sure if it's worth the effort though.

sebastianplesciuc · 2017-04-22T11:00:17Z

I'm looking at https://ci.nodejs.org/job/node-test-commit-linux/nodes=fedora22/9316/console and I can't tell why it failed. Help?

bnoordhuis · 2017-04-22T12:10:06Z

@sebastianplesciuc Infrastructure failure, don't worry about it. That buildbot is getting the ax soon anyway (ref).

refack · 2017-04-22T13:18:35Z

test/parallel/test-cluster-disconnect.js

IMHO can land as is (found 433 hits to indexOf in /test/*.js)
@lpinca since v4 entered maintenance, do you think we can eliminate all 433 now (in a different PR)?

cjihrig · 2017-04-22T14:17:18Z

test/parallel/test-cluster-disconnect.js

Please use camelCase in JavaScript code.

@cjihrig Fixed! Thank you!

aqrln · 2017-04-29T23:12:55Z

test/parallel/test-cluster-disconnect.js

Will we be backporting PRs that fix potentially flaky tests to v4.x? If not (or these common.PORT tests will just be moved to sequential in v4.x), using Array.prototype.includes() might be more readable here.

/cc @nodejs/lts

Will we be backporting PRs that fix potentially flaky tests to v4.x?

Almost certainly not. ! serverPorts.includes(address.port) should be fine here.

Changed it to use includes and rebased this branch to the latest master. Thanks!

aqrln · 2017-05-02T01:51:09Z

test/parallel/test-cluster-disconnect.js

You might want to use a Set, not an array.

@aqrln Changed to use Set. Thank you!

Removed common.PORT from test-cluster-disconnect to eliminate the possibility that a port used in another test will collide with common.PORT. Refs: #12376

jasnell · 2017-05-04T13:53:39Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/7855/

gibfahn · 2017-05-11T09:58:56Z

Landed in bee250c

Removed common.PORT from test-cluster-disconnect to eliminate the possibility that a port used in another test will collide with common.PORT. PR-URL: #12545 Refs: #12376 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

gibfahn · 2017-05-11T10:00:31Z

Adding the lts-watch label as we've seen this fail in CI.

Removed common.PORT from test-cluster-disconnect to eliminate the possibility that a port used in another test will collide with common.PORT. PR-URL: nodejs#12545 Refs: nodejs#12376 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Removed common.PORT from test-cluster-disconnect to eliminate the possibility that a port used in another test will collide with common.PORT. PR-URL: #12545 Refs: #12376 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

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

vsemozhetbyt added the cluster Issues and PRs related to the cluster subsystem. label Apr 20, 2017

lpinca reviewed Apr 22, 2017

View reviewed changes

lpinca approved these changes Apr 22, 2017

View reviewed changes

lpinca reviewed Apr 22, 2017

View reviewed changes

refack approved these changes Apr 22, 2017

View reviewed changes

cjihrig reviewed Apr 22, 2017

View reviewed changes

aqrln reviewed Apr 29, 2017

View reviewed changes

aqrln approved these changes May 2, 2017

View reviewed changes

test: dynamic port in cluster disconnect

af0ed07

Removed common.PORT from test-cluster-disconnect to eliminate the possibility that a port used in another test will collide with common.PORT. Refs: #12376

jasnell approved these changes May 4, 2017

View reviewed changes

cjihrig approved these changes May 4, 2017

View reviewed changes

gibfahn approved these changes May 11, 2017

View reviewed changes

gibfahn closed this May 11, 2017

gibfahn added the lts-watch-v6.x label May 11, 2017

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added land-on-v6.x and removed lts-watch-v6.x labels Jun 20, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

Uh oh!

Conversation

sebastianplesciuc commented Apr 20, 2017

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt commented Apr 20, 2017

Uh oh!

sebastianplesciuc commented Apr 20, 2017

Uh oh!

refack commented Apr 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Apr 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastianplesciuc commented Apr 22, 2017

Uh oh!

bnoordhuis commented Apr 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 4, 2017

Uh oh!

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

Uh oh!

Uh oh!

gibfahn commented May 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

gibfahn commented May 11, 2017 •

edited

Loading