test: increase test timeout to prevent flakiness - #31716
Conversation
Sorry, something went wrong.
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.
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
LGTM, but – and I know I’ve asked this a couple times – why are we having timeouts in the REPL tests at all?
Sorry, something went wrong.
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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… |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@BridgeAR Feel free to land this if you think it’s the right call. |
Sorry, something went wrong.
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) |
Sorry, something went wrong.
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