test: refactor test-http-exit-delay - #4055
Conversation
Sorry, something went wrong.
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
|
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... ¯\_(ツ)_/¯ |
Sorry, something went wrong.
Sorry, something went wrong.
|
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... |
Sorry, something went wrong.
|
Bump! Simplified/improved/faster test FTW? Or unnecessary churn? |
Sorry, something went wrong.
|
LGTM. Is a fixed 1000 ms enough for the slower buildbots? |
Sorry, something went wrong.
|
For debug builds likely not. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
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>
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>
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>
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>
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>
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>
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