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: fix flaky test-https-agent-create-connection #11649

Merged
merged 1 commit into from
Mar 2, 2017

Conversation

santigimeno
Copy link
Member

Use a different server instance for every test to avoid that they reuse
the same port.

Fixes: #11644

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 Mar 2, 2017
@gibfahn
Copy link
Member

gibfahn commented Mar 2, 2017

This seems reasonable, but what is the problem with reusing the same port (i.e. do you know why the current version of the test is failing)?

@santigimeno
Copy link
Member Author

@gibfahn I guess if the first test finishes before the 2nd has connected, the server will be closed, thus the ECONNREFUSED errors

@mscdex mscdex added the https Issues or PRs related to the https subsystem. label Mar 2, 2017
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

mhdawson commented Mar 2, 2017

Assume we'd like this to land sooner than later due to the failures its causing in the builds.

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

@Trott
Copy link
Member

Trott commented Mar 2, 2017

Only CI failure is unrelated build failure on a Raspberry Pi.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM. I'm definitely +1 on making this an exception to the 48-hour rule and landing it much sooner.

@Trott
Copy link
Member

Trott commented Mar 2, 2017

Stress tests are probably in order, so here we go:

Current master: https://ci.nodejs.org/job/node-stress-single-test/1111/nodes=centos5-32/console

This PR: https://ci.nodejs.org/job/node-stress-single-test/nodes=centos5-32/1112/console

@gibfahn
Copy link
Member

gibfahn commented Mar 2, 2017

+1 to landing sooner than 48 hours (as long as the stress test looks okay).

@Trott
Copy link
Member

Trott commented Mar 2, 2017

Straightforward stress test didn't fail on master, so I did some shenanigans to increase load (run 4 tests at once) and that seems to have worked to reproduce the problem, although only 1 in 100 times. https://ci.nodejs.org/job/node-stress-single-test/1113/nodes=centos5-32/console

So, now I'm going to bump it up to 8 parallel tests and do the same for this PR and compare the results.

@Trott
Copy link
Member

Trott commented Mar 2, 2017

Master branch with this test running 8 instances in parallel: https://ci.nodejs.org/job/node-stress-single-test/1114/nodes=centos5-32/console

This PR with this test running 8 instances in parallel: https://ci.nodejs.org/job/node-stress-single-test/1115/nodes=centos5-32/console

@Trott
Copy link
Member

Trott commented Mar 2, 2017

Last stress test failed 100 out of 100 times on master, passed 100 out of 100 times on this PR. :shipit:

Use a different server instance for every test to avoid that they reuse
the same port.

PR-URL: nodejs#11649
Fixes: nodejs#11644
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@santigimeno
Copy link
Member Author

Landed in 5269e95. Thanks!

@santigimeno santigimeno closed this Mar 2, 2017
@santigimeno santigimeno merged commit 5269e95 into nodejs:master Mar 2, 2017
@santigimeno santigimeno deleted the fix-create-connection branch March 2, 2017 17:22
addaleax pushed a commit that referenced this pull request Mar 5, 2017
Use a different server instance for every test to avoid that they reuse
the same port.

PR-URL: #11649
Fixes: #11644
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
Use a different server instance for every test to avoid that they reuse
the same port.

PR-URL: #11649
Fixes: #11644
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Use a different server instance for every test to avoid that they reuse
the same port.

PR-URL: #11649
Fixes: #11644
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Use a different server instance for every test to avoid that they reuse
the same port.

PR-URL: nodejs/node#11649
Fixes: nodejs/node#11644
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-https-agent-create-connection on multiple platforms
8 participants