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 timeout for test-tls-fast-writing #5466

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 28, 2016

Increase timeout for test from 500ms to 5000ms so busy slow machines
don't produce false positives.

Fixes: #4964

@Trott Trott added tls Issues and PRs related to the tls subsystem. test Issues and PRs related to the tests. lts-watch-v4.x labels Feb 28, 2016
@silverwind
Copy link
Contributor

Factor 10 seems a bit much, no? Do CI machines do multiple runs in parallel?

@Trott
Copy link
Member Author

Trott commented Feb 28, 2016

@silverwind asked:

Factor 10 seems a bit much, no?

There's no real advantage to "right-sizing" the timeout other than a user waits a second or two less on a failed test at the command line. So I'd rather put in a preposterous increase now and never have to deal with the problem again instead of putting in a modest increase now and needing to put in another modest increase or two in a month.

Do CI machines do multiple runs in parallel?

I don't think they do yet but I believe @jbergstroem is not far from having that implemented. (Or was the switch already flipped on that?)

@jbergstroem
Copy link
Member

@Trott still not implemented.

@Trott
Copy link
Member Author

Trott commented Feb 29, 2016

@silverwind If reducing the increase from a factor of 10 to a factor of (say) 2 would make you more comfortable with the change, I'm totally OK with doing that instead.

@silverwind
Copy link
Contributor

My issue is that these tests usually set a performance goal through these timeouts, I think I'd be okay with something more modest like 2x or 3x.

Increase timeout for test from 500ms to 1000ms so busy slow machines
don't produce false positives.

Fixes: nodejs#4964
@Trott
Copy link
Member Author

Trott commented Feb 29, 2016

@silverwind OK, changed to 1000ms instead of 5000ms.

CI: https://ci.nodejs.org/job/node-test-pull-request/1783/

@Trott
Copy link
Member Author

Trott commented Mar 1, 2016

CI is green except for one host that failed to finish building. Looks good to anyone? @nodejs/testing

@orangemocha
Copy link
Contributor

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2016
Increase timeout for test from 500ms to 1000ms so busy slow machines
don't produce false positives.

Fixes: nodejs#4964
PR-URL: nodejs#5466
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@Trott
Copy link
Member Author

Trott commented Mar 2, 2016

Landed in c133d07

@Trott Trott closed this Mar 2, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 2, 2016
Increase timeout for test from 500ms to 1000ms so busy slow machines
don't produce false positives.

Fixes: #4964
PR-URL: #5466
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
Increase timeout for test from 500ms to 1000ms so busy slow machines
don't produce false positives.

Fixes: #4964
PR-URL: #5466
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
Increase timeout for test from 500ms to 1000ms so busy slow machines
don't produce false positives.

Fixes: #4964
PR-URL: #5466
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
Increase timeout for test from 500ms to 1000ms so busy slow machines
don't produce false positives.

Fixes: #4964
PR-URL: #5466
Reviewed-By: Alexis Campailla <orangemocha@nodejs.org>
@Trott Trott deleted the arbitrary-timeout-sadness branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants