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: refactor test-http-exit-delay #4055

Closed
wants to merge 1 commit into from
Closed

test: refactor test-http-exit-delay #4055

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 28, 2015

test-http-exit-delay was introduced to confirm the removal of a 1 second
delay that used to occur when exiting node after an http request.

This change refactors the test for simplicity and also in the hopes of
either eliminating flakiness on CI or, if not that, at least making
the source of the flakiness easier to track down.

Ref: 16b59cbc
Fixes: #4045

test-http-exit-delay was introduced to confirm the removal of a 1 second
delay that can occur when exiting node after an http request.

This change refactors the test for simplicity and also in the hopes of
either eliminating flakiness on CI or, if not that, at least making
the source of the flakiness easier to track down.

Ref: 16b59cbc
Fixes: #4045
@Trott Trott added test Issues and PRs related to the tests. http Issues or PRs related to the http subsystem. labels Nov 28, 2015
@Trott
Copy link
Member Author

Trott commented Nov 28, 2015

So, about the flakiness... the test failed once for no apparent reason:

But I have been unable to make it fail with the stress test:

So... ¯\_(ツ)_/¯

@Trott
Copy link
Member Author

Trott commented Nov 28, 2015

@Trott
Copy link
Member Author

Trott commented Nov 28, 2015

Stress test with this change:

If nothing else, the test should at least run marginally faster now.

EDIT: Actually, it seems to run a lot faster...

@Trott
Copy link
Member Author

Trott commented Dec 2, 2015

Bump! Simplified/improved/faster test FTW? Or unnecessary churn?

@bnoordhuis
Copy link
Member

LGTM. Is a fixed 1000 ms enough for the slower buildbots?

@jbergstroem
Copy link
Member

For debug builds likely not.

@Trott
Copy link
Member Author

Trott commented Dec 2, 2015

Putting aside the debug issue for a moment, I'm pretty sure one second is long enough for the Raspberry Pi machines since the existing test was never flaky on the Raspberry Pi machines as far as I know. And they would be the slowest of what we test on in general, right? I'll run a stress test...

https://ci.nodejs.org/job/node-stress-single-test/129/nodes=pi2-raspbian-wheezy/console

EDIT: Guess I need to do it again and not typo on the test name...

https://ci.nodejs.org/job/node-stress-single-test/130/nodes=pi2-raspbian-wheezy/console

@Trott
Copy link
Member Author

Trott commented Dec 2, 2015

999 consecutive susccessful runs on Raspberry Pi, hooray!

I'm inclined to not worry about debug builds until this test is shown to actually be a problem. The implementation in this PR allows for 1 second for the test to run. That is actually longer than the current test allows (no more than 900 ms). So in that regard, this is more debug build friendly than the current test, at least. (And it might not be a problem on debug builds after all? Hopefully?)

Aside: The reason I don't want to use common.platformTimeout() here is because this test is supposed to detect a 1-second delay that used to show up in old versions of Node. Anything that allows the test to take longer than 1 second means you're not really testing to make sure the 1-second delay is gone. But I'm all kinds of ignorantly sunny and optimistic that this will work just fine as-is on debug builds.

@jasnell
Copy link
Member

jasnell commented Dec 3, 2015

LGTM

@Trott
Copy link
Member Author

Trott commented Dec 4, 2015

Landed in 8a2acd4

@Trott Trott closed this Dec 4, 2015
Trott added a commit that referenced this pull request Dec 4, 2015
test-http-exit-delay was introduced to confirm the removal of a 1 second
delay that can occur when exiting node after an http request.

This change refactors the test for simplicity and also in the hopes of
either eliminating flakiness on CI or, if not that, at least making
the source of the flakiness easier to track down.

Ref: 16b59cbc
Fixes: #4045
PR-URL: #4055
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the flaky-test branch December 5, 2015 06:43
Trott added a commit that referenced this pull request Dec 5, 2015
test-http-exit-delay was introduced to confirm the removal of a 1 second
delay that can occur when exiting node after an http request.

This change refactors the test for simplicity and also in the hopes of
either eliminating flakiness on CI or, if not that, at least making
the source of the flakiness easier to track down.

Ref: 16b59cbc
Fixes: #4045
PR-URL: #4055
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit that referenced this pull request Dec 15, 2015
test-http-exit-delay was introduced to confirm the removal of a 1 second
delay that can occur when exiting node after an http request.

This change refactors the test for simplicity and also in the hopes of
either eliminating flakiness on CI or, if not that, at least making
the source of the flakiness easier to track down.

Ref: 16b59cbc
Fixes: #4045
PR-URL: #4055
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Dec 17, 2015
Trott added a commit that referenced this pull request Dec 17, 2015
test-http-exit-delay was introduced to confirm the removal of a 1 second
delay that can occur when exiting node after an http request.

This change refactors the test for simplicity and also in the hopes of
either eliminating flakiness on CI or, if not that, at least making
the source of the flakiness easier to track down.

Ref: 16b59cbc
Fixes: #4045
PR-URL: #4055
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Trott added a commit that referenced this pull request Dec 23, 2015
test-http-exit-delay was introduced to confirm the removal of a 1 second
delay that can occur when exiting node after an http request.

This change refactors the test for simplicity and also in the hopes of
either eliminating flakiness on CI or, if not that, at least making
the source of the flakiness easier to track down.

Ref: 16b59cbc
Fixes: #4045
PR-URL: #4055
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
test-http-exit-delay was introduced to confirm the removal of a 1 second
delay that can occur when exiting node after an http request.

This change refactors the test for simplicity and also in the hopes of
either eliminating flakiness on CI or, if not that, at least making
the source of the flakiness easier to track down.

Ref: nodejs@16b59cbc
Fixes: nodejs#4045
PR-URL: nodejs#4055
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@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