Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: dynamic port in test cluster disconnect #12545

Conversation

@sebastianplesciuc
Copy link
Contributor

commented Apr 20, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

@sebastianplesciuc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

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

@refack

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

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.

test/parallel/test-cluster-disconnect.js Outdated
@@ -47,6 +47,7 @@ if (cluster.isWorker) {
// check result
socket.on('end', common.mustCall(() => {
cb(result === 'echo');
server_ports.splice(server_ports.indexOf(port));

This comment has been minimized.

Copy link
@lpinca

lpinca Apr 22, 2017

Member

Shouldn't the second argument of splice be 1?

This comment has been minimized.

Copy link
@sebastianplesciuc

sebastianplesciuc Apr 22, 2017

Author Contributor

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

@sebastianplesciuc sebastianplesciuc force-pushed the sebastianplesciuc:common-port-cluster-disconnect branch Apr 22, 2017

@lpinca
lpinca approved these changes Apr 22, 2017
@lpinca

This comment has been minimized.

Copy link
Member

commented Apr 22, 2017

test/parallel/test-cluster-disconnect.js Outdated
@@ -72,7 +73,11 @@ if (cluster.isWorker) {
let online = 0;

for (let i = 0, l = workers; i < l; i++) {
cluster.fork().on('listening', common.mustCall(() => {
cluster.fork().on('listening', common.mustCall((address) => {
if (server_ports.indexOf(address.port) === -1) {

This comment has been minimized.

Copy link
@lpinca

lpinca Apr 22, 2017

Member

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

This comment has been minimized.

Copy link
@sebastianplesciuc

sebastianplesciuc Apr 22, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@refack

refack Apr 22, 2017

Member

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)?

This comment has been minimized.

Copy link
@lpinca

lpinca Apr 22, 2017

Member

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

This comment has been minimized.

Copy link
@refack
@sebastianplesciuc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2017

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

This comment has been minimized.

Copy link
Member

commented Apr 22, 2017

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

@refack
refack approved these changes Apr 22, 2017
test/parallel/test-cluster-disconnect.js Outdated
@@ -72,7 +73,11 @@ if (cluster.isWorker) {
let online = 0;

for (let i = 0, l = workers; i < l; i++) {
cluster.fork().on('listening', common.mustCall(() => {
cluster.fork().on('listening', common.mustCall((address) => {
if (server_ports.indexOf(address.port) === -1) {

This comment has been minimized.

Copy link
@refack

refack Apr 22, 2017

Member

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)?

test/parallel/test-cluster-disconnect.js Outdated
} else if (cluster.isMaster) {
const servers = 2;
const server_ports = [];

This comment has been minimized.

Copy link
@cjihrig

cjihrig Apr 22, 2017

Contributor

Please use camelCase in JavaScript code.

This comment has been minimized.

Copy link
@sebastianplesciuc

sebastianplesciuc Apr 22, 2017

Author Contributor

@cjihrig Fixed! Thank you!

@sebastianplesciuc sebastianplesciuc force-pushed the sebastianplesciuc:common-port-cluster-disconnect branch Apr 22, 2017

test/parallel/test-cluster-disconnect.js Outdated
@@ -72,7 +73,11 @@ if (cluster.isWorker) {
let online = 0;

for (let i = 0, l = workers; i < l; i++) {
cluster.fork().on('listening', common.mustCall(() => {
cluster.fork().on('listening', common.mustCall((address) => {
if (serverPorts.indexOf(address.port) === -1) {

This comment has been minimized.

Copy link
@aqrln

aqrln Apr 29, 2017

Member

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

This comment has been minimized.

Copy link
@gibfahn

gibfahn Apr 30, 2017

Member

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

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

This comment has been minimized.

Copy link
@sebastianplesciuc

sebastianplesciuc Apr 30, 2017

Author Contributor

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

@sebastianplesciuc sebastianplesciuc force-pushed the sebastianplesciuc:common-port-cluster-disconnect branch 2 times, most recently Apr 30, 2017

@aqrln
aqrln approved these changes May 2, 2017
test/parallel/test-cluster-disconnect.js Outdated
@@ -47,6 +47,7 @@ if (cluster.isWorker) {
// check result
socket.on('end', common.mustCall(() => {
cb(result === 'echo');
serverPorts.splice(serverPorts.indexOf(port), 1);

This comment has been minimized.

Copy link
@aqrln

aqrln May 2, 2017

Member

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

This comment has been minimized.

Copy link
@sebastianplesciuc

sebastianplesciuc May 4, 2017

Author Contributor

@aqrln Changed to use Set. Thank you!

@sebastianplesciuc sebastianplesciuc force-pushed the sebastianplesciuc:common-port-cluster-disconnect branch to af0ed07 May 4, 2017

Sebastian Plesciuc
test: dynamic port in cluster disconnect
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
jasnell approved these changes May 4, 2017
@cjihrig
cjihrig approved these changes May 4, 2017
@jasnell

This comment has been minimized.

Copy link
Member

commented May 4, 2017

@gibfahn

This comment has been minimized.

Copy link
Member

commented May 11, 2017

Landed in bee250c

@gibfahn gibfahn closed this May 11, 2017

gibfahn added a commit that referenced this pull request May 11, 2017
test: dynamic port in cluster disconnect
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

This comment has been minimized.

Copy link
Member

commented May 11, 2017

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

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Sebastian Plesciuc Olivier Martin
test: dynamic port in cluster disconnect
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>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
gibfahn added a commit that referenced this pull request Jun 20, 2017
test: dynamic port in cluster disconnect
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 gibfahn added land-on-v6.x and removed lts-watch-v6.x labels Jun 20, 2017

MylesBorins added a commit that referenced this pull request Jul 11, 2017
test: dynamic port in cluster disconnect
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>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.