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.

Sign up for GitHub

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

Jump to bottom

test: fix flaky test-http-set-timeout-server #6248

Closed
wants to merge 1 commit into from
Closed

test: fix flaky test-http-set-timeout-server #6248

wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Use different port numbers for every connection to avoid rare
EADDRINUSE errors on SmartOS.

Fixes: #6197

@santigimeno santigimeno added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. smartos Issues and PRs related to the SmartOS platform. labels Apr 16, 2016
@santigimeno
Copy link
Member Author

Ci: https://ci.nodejs.org/job/node-test-pull-request/2289/

@santigimeno santigimeno mentioned this pull request Apr 16, 2016
Closed
@mscdex
Copy link
Contributor

mscdex commented Apr 16, 2016

On a semi-related note, is there a real reason why we don't just allow the OS to assign a free port number for tests instead of using common.PORT? I tried looking back through the archive and the commit that added common.PORT was just trying to unify the various ports used in tests.

Would it be better to modify tests to use a random free port to avoid potential issues like this?

/cc @nodejs/testing

@santigimeno
Copy link
Member Author

Would it be better to modify tests to use a random free port to avoid potential issues like this?

@mscdex I think it is the correct solution. I'll update this test accordingly. The rest of the test suite will take more time. Thanks for the suggestion!

@santigimeno
Copy link
Member Author

Added a new commit so random free ports are used. @mscdex is this what you had in mind? Thanks

@mscdex
Copy link
Contributor

mscdex commented Apr 16, 2016

@santigimeno Yes.

@santigimeno santigimeno force-pushed the fix_ports_reuse branch 3 times, most recently from e205926 to 6b6c67a Compare April 16, 2016 21:41
@santigimeno
Copy link
Member Author

Rebased into a single commit.

@santigimeno
Copy link
Member Author

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

@jbergstroem
Copy link
Member

Perhaps even expand how common.PORT works, making it more resilient against similar things? If we run parallel tests we for instance increment it with +1, but testing another, random, port would be even better (before exiting), no?

@Trott
Copy link
Member

Trott commented Apr 17, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

I think refactoring or improving common.PORT to handle this better would be a great idea, but in a separate PR. This LGTM

@santigimeno
test: fix flaky test-http-set-timeout-server
d639bf6 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: nodejs#6197
PR-URL: nodejs#6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@santigimeno
Copy link
Member Author

Rebased. New CI: https://ci.nodejs.org/job/node-test-pull-request/2312/

@santigimeno
Copy link
Member Author

All is green except for one of the recurrent test failures in the pi2

@santigimeno
Copy link
Member Author

Thanks for the reviews

santigimeno added a commit that referenced this pull request Apr 19, 2016
@santigimeno
test: fix flaky test-http-set-timeout-server
57f0595 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@santigimeno
Copy link
Member Author

Landed in 57f0595

MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
@santigimeno
test: fix flaky test-http-set-timeout-server
e4d145a 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@santigimeno
test: fix flaky test-http-set-timeout-server
20255bf 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Apr 20, 2016
@santigimeno @Trott
test: fix flaky test-http-set-timeout-server
94e1da9 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: nodejs#6197
PR-URL: nodejs#6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@santigimeno
test: fix flaky test-http-set-timeout-server
4be2e15 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@santigimeno
test: fix flaky test-http-set-timeout-server
056a258 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Apr 21, 2016
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@santigimeno @MylesBorins
test: fix flaky test-http-set-timeout-server
1af0e44 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@santigimeno @joelostrowski
test: fix flaky test-http-set-timeout-server
564dbef 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: nodejs#6197
PR-URL: nodejs#6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@santigimeno @jasnell
test: fix flaky test-http-set-timeout-server
4dcb1a2 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 17, 2016
@santigimeno
test: fix flaky test-http-set-timeout-server
738859a 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2016
@santigimeno
test: fix flaky test-http-set-timeout-server
24ac16f 
Make the servers listen on a free port number picked by the OS to avoid
rare `EADDRINUSE` errors on `SmartOS`.

Fixes: #6197
PR-URL: #6248
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 18, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem. smartos Issues and PRs related to the SmartOS platform. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate (flaky?) test-http-set-timeout-server on SmartOS
6 participants
@santigimeno @mscdex @jbergstroem @Trott @jasnell @MylesBorins