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: increase test timeout to prevent flakiness #31716

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

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.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 10, 2020
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 11, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a 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?

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member Author

@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.

@addaleax
Copy link
Member

@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…

@BridgeAR
Copy link
Member Author

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.

@addaleax
Copy link
Member

addaleax commented Mar 9, 2020

@BridgeAR Feel free to land this if you think it’s the right call.

BridgeAR added a commit that referenced this pull request Mar 9, 2020
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>
@BridgeAR
Copy link
Member Author

BridgeAR commented Mar 9, 2020

Landed in 41ac192 (I updated the timeout to 1000 ms while landing)

@BridgeAR BridgeAR closed this Mar 9, 2020
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
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>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
codebytere pushed a commit that referenced this pull request Mar 17, 2020
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>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 23, 2020
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>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants