test: refactor test-net-connect-options-allowhalfopen.js #13003

refack · 2017-05-12T19:49:37Z

Try to stabilize test/parallel/test-net-connect-options-allowhalfopen.js
Fixes: #12951

Test is stable but could use some loving.

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,net

refack · 2017-05-12T19:52:37Z

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

refack · 2017-05-12T23:25:11Z

Trying to fail on freeBSD: https://ci.nodejs.org/job/node-test-commit-freebsd/9006/

mscdex · 2017-05-13T00:30:16Z

test/parallel/test-net-connect-local-error.js

@@ -6,6 +6,7 @@ const net = require('net');
 const server = net.createServer();
 server.listen(0);
 const port = server.address().port;
+console.log(`server port ${port}`);


This should be removed.

Ack.
I just added it to try to check something.
That test is interacting with the test of this PR.

refack · 2017-05-13T02:19:22Z

Another BSD CI:https://ci.nodejs.org/job/node-test-commit-freebsd/9007/

mscdex · 2017-05-13T02:38:03Z

A stress test CI job is better for checking flakiness fixes.

refack · 2017-05-13T02:43:47Z

I need to test the interaction of two test:
test/parallel/test-net-connect-local-error.js
and
test/parallel/test-net-connect-options-allowhalfopen.js
One is connecting to the other's server...

refack · 2017-05-13T03:38:34Z

Try to fail on freeBSD: https://ci.nodejs.org/job/node-stress-single-test/nodes=freebsd10-64/

refack · 2017-05-16T03:38:50Z

Stress test pass: https://ci.nodejs.org/job/node-stress-single-test/1221/nodes=freebsd10-64/

jasnell · 2017-06-13T20:04:38Z

test/parallel/test-net-connect-options-allowhalfopen.js

+ const host = common.localhostIPv4;
+
+ function serverOnConnection(socket) {
+ console.error(`'connection' ${++serverConnections} emitted on server`);


if the console output is not necessary for the test, please remove them :-)

They were very usefull for debugging. Gone now.

refack · 2017-06-13T20:34:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8640/

BridgeAR · 2017-08-26T23:58:05Z

This needs a rebase and there are a couple linter errors.

refack · 2017-09-03T22:52:25Z

ping @nodejs/testing

refack · 2017-09-03T23:13:41Z

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

BridgeAR · 2017-09-19T22:46:49Z

Ping @nodejs/testing @Trott

Trott · 2017-09-19T23:43:16Z

I need to test the interaction of two test:
test/parallel/test-net-connect-local-error.js
and
test/parallel/test-net-connect-options-allowhalfopen.js
One is connecting to the other's server...

I may be missing something important here, but you can run a stress test on just those two tests with something like:

-j 4 --repeat 100 test/parallel/test-net-connect-local-error.js test/parallel/test-net-connect-options-allowhalfopen.js

Trott · 2017-09-19T23:44:11Z

I have to attend to something else right now but I'll try to make a point to look at this in the next 24 hours...

Trott · 2017-09-20T02:22:17Z

Any chance we can get a more detailed commit message?

Trott · 2017-09-20T02:24:32Z

Change seem good to me but might be useful to get @bnoordhuis, @indutny, @nodejs/streams to take a look.

refack · 2017-09-20T02:40:52Z

-j 4 --repeat 100 test/parallel/test-net-connect-local-error.js test/parallel/test-net-connect-options-allowhalfopen.js

Yeah I didn't think about it at the time.
It was just hard to follow so I massaged it a little bit so it would be easier to track why it failed. Turns out this test on it's own is stable.
What happened is:

test/parallel/test-net-connect-local-error.js is trying to connect to something like listen(0).port + 1 so it would fail.
on openBSD ephemeral ports are given sequentially.
This test runs in parallel and actually binds said port.
both test fail.

I think we solved this dis-harmony by moving test-net-connect-local-error.js to sequential. So what's left in this PR if just refactoring.

AFAICT this was blocked because @jasnell asked to remove the logging, but I'm -0 on that since the order of events in the test is tricky. Eventually it was the log that made me wonder where the 7th connection is coming from if the clients are only making 6 🤔

Mismatched serverOnConnection function calls. Expected 6, actual 7.
    at forAllClients (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-net-connect-options-allowhalfopen.js:44:38)
...
No. 4 connection is closing server:
  7 FIN received by server,
  6 FIN received by client,
  6 FIN sent by client,
  7 FIN sent by server
Server has been closed:
  7 FIN received by server,
  6 FIN received by client,
  6 FIN sent by client,
  7 FIN sent by server

mcollina

LGTM

PR-URL: nodejs#13003 Fixes: nodejs#12951 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

refack · 2017-09-20T20:43:58Z

Quick sanity of master: https://ci.nodejs.org/job/node-test-commit-linuxone/8705/

PR-URL: #13003 Fixes: #12951 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs/node#13003 Fixes: nodejs/node#12951 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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

refack changed the title ~~test: to to stabilize test~~ test: try to stabilize test May 12, 2017

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

mscdex reviewed May 13, 2017

View reviewed changes

refack mentioned this pull request May 14, 2017

test: introduce tag functions to handle long strings in code #13016

Closed

4 tasks

refack changed the title ~~test: try to stabilize test~~ test: refactor test-net-connect-options-allowhalfopen.js May 15, 2017

refack force-pushed the try-fix-test-halfopen-12951 branch from d09d2c0 to dfed152 Compare May 15, 2017 22:46

jasnell reviewed Jun 13, 2017

View reviewed changes

refack force-pushed the try-fix-test-halfopen-12951 branch from ce5ba90 to ec67d66 Compare September 3, 2017 22:50

refack force-pushed the try-fix-test-halfopen-12951 branch from ec67d66 to e02d7b0 Compare September 3, 2017 22:59

mcollina approved these changes Sep 20, 2017

View reviewed changes

test: refactor test for readability

01c680b

PR-URL: nodejs#13003 Fixes: nodejs#12951 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

refack force-pushed the try-fix-test-halfopen-12951 branch from e02d7b0 to 01c680b Compare September 20, 2017 20:41

refack merged commit 01c680b into nodejs:master Sep 20, 2017

refack deleted the try-fix-test-halfopen-12951 branch September 20, 2017 20:44

jasnell pushed a commit that referenced this pull request Sep 20, 2017

test: refactor test for readability

59f1836

PR-URL: #13003 Fixes: #12951 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017

test: refactor test for readability

90470f6

PR-URL: nodejs/node#13003 Fixes: nodejs/node#12951 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: refactor test-net-connect-options-allowhalfopen.js #13003

test: refactor test-net-connect-options-allowhalfopen.js #13003

refack commented May 12, 2017 •

edited

Loading

refack commented May 12, 2017

refack commented May 12, 2017

mscdex May 13, 2017

refack May 13, 2017

refack commented May 13, 2017

mscdex commented May 13, 2017

refack commented May 13, 2017

refack commented May 13, 2017

refack commented May 16, 2017

jasnell Jun 13, 2017

refack Jun 13, 2017

refack commented Jun 13, 2017

BridgeAR commented Aug 26, 2017

refack commented Sep 3, 2017

refack commented Sep 3, 2017

BridgeAR commented Sep 19, 2017

Trott commented Sep 19, 2017

Trott commented Sep 19, 2017

Trott commented Sep 20, 2017

Trott commented Sep 20, 2017

refack commented Sep 20, 2017 •

edited

Loading

mcollina left a comment

refack commented Sep 20, 2017

test: refactor test-net-connect-options-allowhalfopen.js #13003

test: refactor test-net-connect-options-allowhalfopen.js #13003

Conversation

refack commented May 12, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented May 12, 2017

refack commented May 12, 2017

mscdex May 13, 2017

Choose a reason for hiding this comment

refack May 13, 2017

Choose a reason for hiding this comment

refack commented May 13, 2017

mscdex commented May 13, 2017

refack commented May 13, 2017

refack commented May 13, 2017

refack commented May 16, 2017

jasnell Jun 13, 2017

Choose a reason for hiding this comment

refack Jun 13, 2017

Choose a reason for hiding this comment

refack commented Jun 13, 2017

BridgeAR commented Aug 26, 2017

refack commented Sep 3, 2017

refack commented Sep 3, 2017

BridgeAR commented Sep 19, 2017

Trott commented Sep 19, 2017

Trott commented Sep 19, 2017

Trott commented Sep 20, 2017

Trott commented Sep 20, 2017

refack commented Sep 20, 2017 • edited Loading

mcollina left a comment

Choose a reason for hiding this comment

refack commented Sep 20, 2017

refack commented May 12, 2017 •

edited

Loading

refack commented Sep 20, 2017 •

edited

Loading