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: remove timer from test-http-1.0 #5129

Closed
wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

It's possible that the end event is emitted after the timeout fires
causing the test to fail. Just remove the timer. If for some reason the
end would never fire, the test will fail with a timeout.

It tries to fix the issue described here: #4600 (comment), that I also have rarely experienced on Debian Jessie 64.

@Trott
Copy link
Member

Trott commented Feb 7, 2016

Can we change wrap the callback function in c.on('end', function() { in common.mustCall() to guarantee that the validation code runs?

@Trott
Copy link
Member

Trott commented Feb 7, 2016

I think one thing the setTimeout() and process.exit() were there for was to prevent side effects with other tests. They try to guarantee that the server gets closed so that subsequent tests don't fail with EADDRINUSE if this test fails.

@r-52 r-52 added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. lts-watch-v4.x labels Feb 7, 2016
@jasnell
Copy link
Member

jasnell commented Feb 7, 2016

LGTM if CI is green

It's possible that the `end` event is emitted after the timeout fires
causing the test to fail. Just remove the timer. If for some reason the
`end` would never fire, the test will fail with a timeout.
@santigimeno
Copy link
Member Author

PR update with @Trott suggestion.

@santigimeno
Copy link
Member Author

@Trott Do you think I should close the PR then? Or can we try how it goes without the timer?

@Trott
Copy link
Member

Trott commented Feb 8, 2016

@santigimeno Would a decent middle ground be to leave the process.on('exit', ...) but get rid of the setTimeout()?

(Or maybe we can confirm that the test runner on CI will close the server if the test leaves it open? Maybe create a test that runs before all others that opens a server on common.PORT and leaves it open and see what happens when you run make test-ci?)

@santigimeno
Copy link
Member Author

@Trott Sorry for the delay answering this, I've been very busy.
In order to confirm if we can remove the setTimeout I've done the following: I have created 64 copies of the new test-http-1.0.js test (without the Timeout). I've modified 2 of them: one would throw as soon as the server starts listening, the other would never close the server. Then I've run: make test-ci and make test. In both cases, the 2 tests failed but there were no issues with the port being in use.

@Trott
Copy link
Member

Trott commented Feb 16, 2016

@jasnell
Copy link
Member

jasnell commented Feb 17, 2016

Build bot failure on Windows in CI, running again: https://ci.nodejs.org/job/node-test-pull-request/1685/

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@Trott ... any further issues with this?

@Trott
Copy link
Member

Trott commented Mar 21, 2016

@jasnell No issues from me. LGTM

jasnell pushed a commit that referenced this pull request Mar 22, 2016
It's possible that the `end` event is emitted after the timeout fires
causing the test to fail. Just remove the timer. If for some reason the
`end` would never fire, the test will fail with a timeout.

PR-URL: #5129
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Landed in 6a3d38f

@jasnell jasnell closed this Mar 22, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
It's possible that the `end` event is emitted after the timeout fires
causing the test to fail. Just remove the timer. If for some reason the
`end` would never fire, the test will fail with a timeout.

PR-URL: #5129
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
It's possible that the `end` event is emitted after the timeout fires
causing the test to fail. Just remove the timer. If for some reason the
`end` would never fire, the test will fail with a timeout.

PR-URL: #5129
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
It's possible that the `end` event is emitted after the timeout fires
causing the test to fail. Just remove the timer. If for some reason the
`end` would never fire, the test will fail with a timeout.

PR-URL: #5129
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants