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: move net reconnect error test to sequential #13033

Closed
wants to merge 1 commit into from

Conversation

@arturgvieira
Copy link
Contributor

commented May 15, 2017

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
Trott approved these changes May 15, 2017
Copy link
Member

left a comment

LGTM

@mscdex mscdex added the net label May 15, 2017

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

This comment has been minimized.

Copy link
Member

commented May 15, 2017

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

This comment has been minimized.

Copy link
Member

commented May 15, 2017

[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

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2017

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

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

@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 approved these changes May 16, 2017
Copy link
Member

left a comment

👍

@refack

This comment has been minimized.

Copy link
Member

commented May 16, 2017

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

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

@refack refack self-assigned this May 16, 2017

@Trott
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member

commented May 16, 2017

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

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

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

test/sequential/test-net-reconnect-error.js Outdated
}));

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

This comment has been minimized.

Copy link
@refack

refack May 16, 2017

Member

Yep @Trott is right, this should run N times

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

I have made the requested changes.

test/sequential/test-net-reconnect-error.js Outdated
@@ -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) {

This comment has been minimized.

Copy link
@refack

refack May 16, 2017

Member

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

@refack

This comment has been minimized.

Copy link
Member

commented May 16, 2017

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

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

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2017

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

@addaleax

This comment has been minimized.

Copy link
Member

commented May 19, 2017

@Trott @refack ping

@refack
refack approved these changes May 19, 2017
test/sequential/test-net-reconnect-error.js Outdated
client_error_count++;
assert.strictEqual('ECONNREFUSED', e.code);
});
}, 21));

This comment has been minimized.

Copy link
@refack

refack May 19, 2017

Member

You could use N + 1

test/sequential/test-net-reconnect-error.js Outdated
if (disconnect_count++ < N)
c.connect(common.PORT); // reconnect
});
}, 21));

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

This comment has been minimized.

Copy link
@refack

refack May 19, 2017

Member

[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 refack referenced this pull request May 19, 2017
3 of 3 tasks complete
@refack

This comment has been minimized.

Copy link
Member

commented May 19, 2017

test: move net reconnect error test to sequential
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

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2017

@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
refack approved these changes May 20, 2017
@refack

This comment has been minimized.

Copy link
Member

commented May 20, 2017

@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
Trott approved these changes May 20, 2017
Copy link
Member

left a comment

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

@arturgvieira

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2017

@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

This comment has been minimized.

Copy link
Member

commented May 20, 2017

@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

This comment has been minimized.

Copy link
Member

commented May 20, 2017

@refack

This comment has been minimized.

Copy link
Member

commented May 20, 2017

landed in c60a7fa

@refack refack closed this May 20, 2017

refack added a commit that referenced this pull request May 20, 2017
test: move net reconnect error test to sequential
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

This comment has been minimized.

Copy link
Member

commented May 20, 2017

@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: move net reconnect error test to sequential
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>
@MylesBorins MylesBorins referenced this pull request Jul 18, 2017

@refack refack removed their assignment Oct 20, 2018

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.