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: replace port in cluster dgram reuse test #12901

Closed
wants to merge 1 commit into from

Conversation

@arturgvieira
Copy link
Contributor

commented May 8, 2017

Replaced common.PORT with zero in the following test.
test-cluster-dgram-reuse.js

Refs: #12376

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@watilde

This comment has been minimized.

test/parallel/test-net-socket-destroy-twice.js Outdated
@@ -23,7 +23,7 @@
const common = require('../common');
const net = require('net');

const conn = net.createConnection(common.PORT);
const conn = net.createConnection(0);

This comment has been minimized.

Copy link
@arturgvieira

arturgvieira May 8, 2017

Author Contributor

Would net.connect() work better here?

This comment has been minimized.

Copy link
@Trott

Trott May 8, 2017

Member

Port 0 with connect() or createConnection() makes no sense, except perhaps for connections that will fail for other reasons already (in which case it should probably have a comment explaining that).

This comment has been minimized.

Copy link
@Trott

Trott May 8, 2017

Member

(Which it looks like this one is expected to fail because there's nothing listening on the port. But then the issue is: Does this test still test the thing it was written to test? I don't know the answer to that.)

test/parallel/test-tls-client-default-ciphers.js Outdated
@@ -40,7 +40,7 @@ function test1() {
};

try {
tls.connect(common.PORT);
tls.connect(0);

This comment has been minimized.

Copy link
@Trott

Trott May 8, 2017

Member

Same: port 0 on connect() makes no sense. Comment needed if this change is correct.

test/parallel/test-tls-connect.js Outdated
@@ -37,7 +37,7 @@ const path = require('path');
const cert = fs.readFileSync(path.join(common.fixturesDir, 'test_cert.pem'));
const key = fs.readFileSync(path.join(common.fixturesDir, 'test_key.pem'));

const options = {cert: cert, key: key, port: common.PORT};
const options = {cert: cert, key: key, port: 0};

This comment has been minimized.

Copy link
@Trott

Trott May 8, 2017

Member

Same as other comments regarding connect() and port 0.

test/parallel/test-tls-connect.js Outdated
@@ -51,7 +51,7 @@ const path = require('path');
const conn = tls.connect({
cert: cert,
key: key,
port: common.PORT,
port: 0,

This comment has been minimized.

Copy link
@Trott

Trott May 8, 2017

Member

Same as other comments regarding connect() and port 0.

@Trott
Copy link
Member

left a comment

I'm skeptical that the port 0 connect() calls are still testing what they are supposed to test. At a minimum, they need a comment.

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

The comment, I'm trying to understand how to phrase it, please let me know if this is correct.

If you use common.PORT, then common/index.js assigns process.env.NODE_COMMON_PORT | 12376 so that would be the same thing as well, right.

@Trott

This comment has been minimized.

Copy link
Member

commented May 8, 2017

Let's say that common.PORT is 12346 (which it often is).

net.connect(common.PORT) will be "connect to port 12346". Often port 12346 isn't listening. In that case, I get ECONNREFUSED.

net.connect(0) is invalid. In that case, I get EADDRNOTAVAIL.

So changing net.connect(common.PORT) to net.connect(0) fundamentally changes the way the network connection fails. It is therefore dangerous (or at least unwise) to make this change without fully understanding how the test works and what is being tested. You may be causing the test to succeed for the wrong reasons.

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

Understood. What do you think would be a better solution. If you could help me I would really appreciate it. I'm on #node-dev.

@Trott

This comment has been minimized.

Copy link
Member

commented May 8, 2017

Understood. What do you think would be a better solution.

It requires a case-by-case analysis. In some cases, using port 0 will be fine. In others, moving to sequential will be the better solution.

@Trott

This comment has been minimized.

Copy link
Member

commented May 8, 2017

By the way, moving to sequential for these is always fine. It's might not always the best solution. But it is an acceptable solution.
¯\(ツ)

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

@Trott Thanks for the direction, I will try to look a little closer at each test.

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2017

I have shrunk this PR to just the first file. Might be better to go over those other tests separately.

@arturgvieira arturgvieira changed the title test: remove common.PORT or move test to sequential test: replace common.PORT with zero May 9, 2017

@Trott Trott dismissed their stale review May 9, 2017

scope of PR has been reduced, eliminating my concerns

@lpinca
lpinca approved these changes May 9, 2017
@lpinca

This comment has been minimized.

Copy link
Member

commented May 9, 2017

@arturgvieira arturgvieira changed the title test: replace common.PORT with zero test: replace port in cluster dgram reuse test May 10, 2017

test/parallel/test-cluster-dgram-reuse.js Outdated
@@ -37,4 +37,4 @@ function close() {
}

for (let i = 0; i < 2; i++)
dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(common.PORT, next);
dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(0, next);

This comment has been minimized.

Copy link
@lpinca

lpinca May 12, 2017

Member

At a second glance, it seems that this will make the two sockets use two different ports which is not what we want here. Something like this should work.

diff --git a/test/parallel/test-cluster-dgram-reuse.js b/test/parallel/test-cluster-dgram-reuse.js
index aed565a380..3e7868bc30 100644
--- a/test/parallel/test-cluster-dgram-reuse.js
+++ b/test/parallel/test-cluster-dgram-reuse.js
@@ -17,24 +17,22 @@ if (cluster.isMaster) {
   return;
 }
 
-const sockets = [];
-function next() {
-  sockets.push(this);
-  if (sockets.length !== 2)
-    return;
-
-  // Work around health check issue
-  process.nextTick(() => {
-    for (let i = 0; i < sockets.length; i++)
-      sockets[i].close(close);
-  });
-}
-
 let waiting = 2;
 function close() {
   if (--waiting === 0)
     cluster.worker.disconnect();
 }
 
-for (let i = 0; i < 2; i++)
-  dgram.createSocket({ type: 'udp4', reuseAddr: true }).bind(common.PORT, next);
+const options = { type: 'udp4', reuseAddr: true };
+const socket1 = dgram.createSocket(options);
+const socket2 = dgram.createSocket(options);
+
+socket1.bind(0, () => {
+  socket2.bind(socket1.address().port, () => {
+    // Work around health check issue
+    process.nextTick(() => {
+      socket1.close(close);
+      socket2.close(close);
+    });
+  })
+});

This comment has been minimized.

Copy link
@arturgvieira

arturgvieira May 12, 2017

Author Contributor

I will make the changes, thanks.

test: replace port in cluster dgram reuse test
Replaced common.PORT with zero in the following test.
test-cluster-dgram-reuse.js

Refs: #12376
@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2017

@lpinca All done, I made the edits.

@lpinca
lpinca approved these changes May 12, 2017
@lpinca

This comment has been minimized.

Copy link
Member

commented May 12, 2017

@lpinca

This comment has been minimized.

Copy link
Member

commented May 19, 2017

@nodejs/collaborators anyone else want to sign-off on this?

@mcollina
Copy link
Member

left a comment

LGTM

@refack
refack approved these changes May 19, 2017
@refack

This comment has been minimized.

Copy link
Member

commented May 19, 2017

Just for cross reference #13100

@lpinca

This comment has been minimized.

Copy link
Member

commented May 19, 2017

Landed in 6b1819c.

@lpinca lpinca closed this May 19, 2017

lpinca added a commit that referenced this pull request May 19, 2017
test: use dynamic port in test-cluster-dgram-reuse
Remove common.PORT from test-cluster-dgram-reuse to eliminate
possibility that a dynamic port used in another test will collide with
common.PORT.

PR-URL: #12901
Ref: #12376
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@lpinca lpinca referenced this pull request May 19, 2017
2 of 2 tasks complete
@Trott Trott referenced this pull request May 19, 2017
55 of 64 tasks complete
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: use dynamic port in test-cluster-dgram-reuse
Remove common.PORT from test-cluster-dgram-reuse to eliminate
possibility that a dynamic port used in another test will collide with
common.PORT.

PR-URL: nodejs#12901
Ref: nodejs#12376
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@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
MylesBorins added a commit that referenced this pull request Jul 17, 2017
test: use dynamic port in test-cluster-dgram-reuse
Remove common.PORT from test-cluster-dgram-reuse to eliminate
possibility that a dynamic port used in another test will collide with
common.PORT.

PR-URL: #12901
Ref: #12376
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@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.