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

Closed
wants to merge 1 commit into from
Closed

test: dynamic port in test cluster disconnect #12545

wants to merge 1 commit into from

Conversation

sebastianplesciuc
Copy link

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 20, 2017
@vsemozhetbyt vsemozhetbyt added the cluster Issues and PRs related to the cluster subsystem. label Apr 20, 2017
@vsemozhetbyt
Copy link
Contributor

@sebastianplesciuc
Copy link
Author

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

@refack
Copy link
Contributor

refack 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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the second argument of splice be 1?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lpinca
Copy link
Member

lpinca commented Apr 22, 2017

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastianplesciuc
Copy link
Author

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
Copy link
Member

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

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if (cluster.isMaster) {
const servers = 2;
const server_ports = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use camelCase in JavaScript code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjihrig Fixed! Thank you!

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -47,6 +47,7 @@ if (cluster.isWorker) {
// check result
socket.on('end', common.mustCall(() => {
cb(result === 'echo');
serverPorts.splice(serverPorts.indexOf(port), 1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Member

jasnell commented May 4, 2017

@gibfahn
Copy link
Member

gibfahn commented May 11, 2017

Landed in bee250c

@gibfahn gibfahn closed this May 11, 2017
gibfahn pushed a commit that referenced this pull request May 11, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@gibfahn
Copy link
Member

gibfahn 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
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants