-
Notifications
You must be signed in to change notification settings - Fork 30k
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: increase test timeout to prevent flakiness #31716
Conversation
This increases the waiting time for each event from 500 to 750 ms. The former timeout could be hit on very slow machines with high load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but – and I know I’ve asked this a couple times – why are we having timeouts in the REPL tests at all?
@addaleax the test was written in a way that it expects output that is triggered due to the input. That is received as event. If input does not provide any output, the test will hang. As such the timeout stops the test early to inform the user about the last input. |
@BridgeAR Okay, but in that case, couldn’t we set the timeout to a larger value like 10 seconds or so without any real downsides? I know 750ms is already “a lot” in computing terms, but I don’t think it’s entirely unreasonable for a process to hang that long on e.g. a slow single-core system… |
A large value would be a bad user experience in case they run the test alone and this will most likely be triggered when working onthe test or when working on repl code. I guess a value up to 1000ms would still be fine and 1500 if necessary. It only came up rarely as being flaky so doubling the timeout should be very safe. |
@BridgeAR Feel free to land this if you think it’s the right call. |
This increases the waiting time for each event from 500 to 750 ms. The former timeout could be hit on very slow machines with high load. PR-URL: #31716 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Landed in 41ac192 (I updated the timeout to 1000 ms while landing) |
This increases the waiting time for each event from 500 to 750 ms. The former timeout could be hit on very slow machines with high load. PR-URL: #31716 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This increases the waiting time for each event from 500 to 750 ms. The former timeout could be hit on very slow machines with high load. PR-URL: #31716 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This increases the waiting time for each event from 500 to 750 ms. The former timeout could be hit on very slow machines with high load. PR-URL: #31716 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This increases the waiting time for each event from 500 to 750 ms. The former timeout could be hit on very slow machines with high load. PR-URL: #31716 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This increases the waiting time for each event from 500 to 750 ms.
The former timeout could be hit on very slow machines with high load.
See e.g. https://ci.nodejs.org/job/node-test-commit-linux-containered/17948/nodes=ubuntu1804_sharedlibs_zlib_x64/testReport/junit/(root)/test/parallel_test_repl/
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes