test: move net reconnect error test to sequential #13033

arturgvieira-zz · 2017-05-15T03:47:10Z

The usage of common.PORT could cause undesired port collisions when run
in parallel. The following test was moved to sequential.
test-net-reconnect-error.js

Ref: #12376

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test net

Trott

LGTM

lpinca · 2017-05-15T07:59:26Z

There is a comment on the test

// Hopefully nothing is running on common.PORT

which I think can be removed now. Calls to console.{log,error} can also be removed.
Anyway the cleanup can be done in another PR.

refack · 2017-05-15T13:18:14Z

[non blocking opinion]
I'm of the opinion that if you're touching a test file, you should normalize it (address @lpinca's comments + add common.mustNotCall instead of all the const N = 20; let client_error_count = 0; let disconnect_count = 0; arithmetic.
But I will not block this over that.

arturgvieira-zz · 2017-05-15T22:52:14Z

Hi, I've removed the comment, as @lpinca mentioned.

arturgvieira-zz · 2017-05-16T00:34:20Z

@refack Could you take a look and let me know if this is what you meant? If not could you point me in the right direction? Not sure how to use common.mustNotCall.

refack

👍

refack · 2017-05-16T02:13:10Z

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

arturgvieira-zz · 2017-05-16T02:30:26Z

I checked on the lint errors. Everything should be good to go.

Trott

The changes introduced here greatly change the test. Previously, there were 20 attempted connections, now there is only 1. This is not a refactor; this is a change. Should be in a separate PR if at all.

Trott · 2017-05-16T03:59:53Z

By the way, the change from 20 attempted connections to just 1 seems likely to defeat the purpose of the test, as the test name contains reconnect implying more than 1 connection attempt.

I'm not opposed to refactoring a test that is being moved in general, but I don't know that I would have recommended it here where you already had a bunch of reviews/approvals.

arturgvieira-zz · 2017-05-16T04:01:26Z

Good point, I will revert it back to the original test.

refack · 2017-05-16T04:03:06Z

test/sequential/test-net-reconnect-error.js

+}));
+
+c.on('close', common.mustCall(() => {
+  once(() => c.connect(common.PORT)); // reconnect


Yep @Trott is right, this should run N times

arturgvieira-zz · 2017-05-16T04:09:46Z

I have made the requested changes.

refack · 2017-05-16T04:11:01Z

test/sequential/test-net-reconnect-error.js

@@ -27,19 +27,16 @@ const N = 20;
 let client_error_count = 0;
 let disconnect_count = 0;

-// Hopefully nothing is running on common.PORT
 const c = net.createConnection(common.PORT);

 c.on('connect', common.mustNotCall('client should not have connected'));

 c.on('error', function(e) {


You could try to add `common.mustCall(..., 20) here and on line 39

refack · 2017-05-16T04:12:06Z

Good point, I will revert it back to the original test.

I know we're driving you crazy back and forth, sorry...

arturgvieira-zz · 2017-05-16T04:15:27Z

No worries. I am most interested in getting it right. I don't mind.

addaleax · 2017-05-19T21:44:00Z

@Trott @refack ping

refack · 2017-05-19T21:51:26Z

test/sequential/test-net-reconnect-error.js

  client_error_count++;
  assert.strictEqual('ECONNREFUSED', e.code);
-});
+}, 21));


You could use N + 1

refack · 2017-05-19T21:55:05Z

test/sequential/test-net-reconnect-error.js

  if (disconnect_count++ < N)
    c.connect(common.PORT); // reconnect
-});
+}, 21));

 process.on('exit', function() {


[suggestion]
I see here an interesting point. Think about this handler...
On the one hand you have the mustCall count that verified each operation is called 21 time.
On the other hand you already have these numbers calculated, so why not check them.

From my perspective it's totally up to you to decide, but thinking about it IMHO is the important part.

refack · 2017-05-19T22:03:55Z

Stress on freeBSD (100 x all sequential): https://ci.nodejs.org/job/node-stress-single-test/nodes=freebsd10-64/1230/

The usage of common.PORT could cause undesired port collisions when run in parallel. The following test was moved to sequential. test-net-reconnect-error.js Ref: #12376

arturgvieira-zz · 2017-05-20T00:38:47Z

@refack I made the first changes you requested and used N + 1, but not sure what would be the desired change on the second request entry, the one about the 'exit' handler. If you could clarify. Thanks

refack · 2017-05-20T03:24:33Z

@refack I made the first changes you requested and used N + 1, but not sure what would be the desired change on the second request entry, the one about the 'exit' handler. If you could clarify. Thanks

👍
About the second part, I just said "think about it". You can keep it, or you can remove it, I don't have a strong opinion either way.
IMHO you should decide! And I'd be happy to hear your rationale.

P.S. Stress test seems to pass.

Trott

LGTM if CI is green. Thanks for sticking with it!

arturgvieira-zz · 2017-05-20T07:45:29Z

@refack I think that since those variables are used in the test, that we should track their input and output values. As a way to make certain that the test framing stays together, making sure that their values are known helps to understand the test, and have expectations as to results.

refack · 2017-05-20T12:04:58Z

@refack I think that since those variables are used in the test, that we should track their input and output values. As a way to make certain that the test framing stays together, making sure that their values are known helps to understand the test, and have expectations as to results.

Sounds like a good reason to me. Thank you for taking the time to indulge me 👍

refack · 2017-05-20T12:09:37Z

CI: https://ci.nodejs.org/job/node-test-commit/10039/
If green I'll land.

refack · 2017-05-20T12:50:30Z

landed in c60a7fa

The usage of common.PORT could cause undesired port collisions when run in parallel. The following test was moved to sequential. test-net-reconnect-error.js PR-URL: #13033 Refs: #12376 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

refack · 2017-05-20T12:53:18Z

Post land CI: https://ci.nodejs.org/job/node-test-commit/10041/

The usage of common.PORT could cause undesired port collisions when run in parallel. The following test was moved to sequential. test-net-reconnect-error.js PR-URL: #13033 Refs: #12376 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

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

Trott approved these changes May 15, 2017

View reviewed changes

mscdex added the net Issues and PRs related to the net subsystem. label May 15, 2017

lpinca approved these changes May 15, 2017

View reviewed changes

cjihrig approved these changes May 15, 2017

View reviewed changes

jasnell approved these changes May 15, 2017

View reviewed changes

refack approved these changes May 16, 2017

View reviewed changes

refack self-assigned this May 16, 2017

Trott requested changes May 16, 2017

View reviewed changes

refack suggested changes May 16, 2017

View reviewed changes

refack approved these changes May 19, 2017

View reviewed changes

refack reviewed May 19, 2017

View reviewed changes

refack mentioned this pull request May 19, 2017

test: fix sequential test-net-connect-local-error #13064

Closed

3 tasks

test: move net reconnect error test to sequential

157ddbd

The usage of common.PORT could cause undesired port collisions when run in parallel. The following test was moved to sequential. test-net-reconnect-error.js Ref: #12376

refack approved these changes May 20, 2017

View reviewed changes

Trott approved these changes May 20, 2017

View reviewed changes

refack closed this May 20, 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

MylesBorins added the land-on-v6.x label Jul 17, 2017

MylesBorins mentioned this pull request Jul 18, 2017

v6.11.2 proposal #14356

Merged

refack removed their assignment Oct 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: move net reconnect error test to sequential #13033

test: move net reconnect error test to sequential #13033

arturgvieira-zz commented May 15, 2017 •

edited

Loading

Trott left a comment

lpinca commented May 15, 2017

refack commented May 15, 2017

arturgvieira-zz commented May 15, 2017 •

edited

Loading

arturgvieira-zz commented May 16, 2017 •

edited

Loading

refack left a comment

refack commented May 16, 2017

arturgvieira-zz commented May 16, 2017

Trott left a comment

Trott commented May 16, 2017

arturgvieira-zz commented May 16, 2017

refack May 16, 2017

arturgvieira-zz commented May 16, 2017

refack May 16, 2017

refack commented May 16, 2017

arturgvieira-zz commented May 16, 2017

addaleax commented May 19, 2017

refack May 19, 2017

refack May 19, 2017

refack commented May 19, 2017

arturgvieira-zz commented May 20, 2017

refack commented May 20, 2017 •

edited

Loading

Trott left a comment

arturgvieira-zz commented May 20, 2017

refack commented May 20, 2017

refack commented May 20, 2017

refack commented May 20, 2017

refack commented May 20, 2017

test: move net reconnect error test to sequential #13033

test: move net reconnect error test to sequential #13033

Conversation

arturgvieira-zz commented May 15, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Trott left a comment

Choose a reason for hiding this comment

lpinca commented May 15, 2017

refack commented May 15, 2017

arturgvieira-zz commented May 15, 2017 • edited Loading

arturgvieira-zz commented May 16, 2017 • edited Loading

refack left a comment

Choose a reason for hiding this comment

refack commented May 16, 2017

arturgvieira-zz commented May 16, 2017

Trott left a comment

Choose a reason for hiding this comment

Trott commented May 16, 2017

arturgvieira-zz commented May 16, 2017

refack May 16, 2017

Choose a reason for hiding this comment

arturgvieira-zz commented May 16, 2017

refack May 16, 2017

Choose a reason for hiding this comment

refack commented May 16, 2017

arturgvieira-zz commented May 16, 2017

addaleax commented May 19, 2017

refack May 19, 2017

Choose a reason for hiding this comment

refack May 19, 2017

Choose a reason for hiding this comment

refack commented May 19, 2017

arturgvieira-zz commented May 20, 2017

refack commented May 20, 2017 • edited Loading

Trott left a comment

Choose a reason for hiding this comment

arturgvieira-zz commented May 20, 2017

refack commented May 20, 2017

refack commented May 20, 2017

refack commented May 20, 2017

refack commented May 20, 2017

arturgvieira-zz commented May 15, 2017 •

edited

Loading

arturgvieira-zz commented May 15, 2017 •

edited

Loading

arturgvieira-zz commented May 16, 2017 •

edited

Loading

refack commented May 20, 2017 •

edited

Loading