Skip to content

test: fix flaky test-http2-session-timeout - #20692

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:one-less-timer
Closed

test: fix flaky test-http2-session-timeout#20692
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:one-less-timer

Conversation

@Trott

@TrottTrott commented May 13, 2018

Copy link
Copy Markdown
Member

Check actual expired time rather than relying on a number of calls to setTimeout() in test-http2-session-timeout more robust.

Fixes: #20628

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 13, 2018
@TrottTrott added the wip Issues and PRs that are still a work in progress. label May 13, 2018
@TrottTrott changed the title test: fix flaky test-http2-session-timeout[WIP] test: fix flaky test-http2-session-timeoutMay 13, 2018
@TrottTrott changed the title [WIP] test: fix flaky test-http2-session-timeouttest: fix flaky test-http2-session-timeoutMay 13, 2018
@TrottTrott removed the wip Issues and PRs that are still a work in progress. label May 13, 2018
@Trott

Copy link
Copy Markdown
MemberAuthor

This version of the test is demonstrably more reliable than the current version when tested this way:

  • Copy the current version of the test to test/parallel.
  • Run the test with tools/test.py -j 16 --repeat 192 tools/test/parallal/test-<WHATEVER_YOU_COPIED_IT_TO>
  • Repeat with the new version of the test (copied to test/parallel).

You may need to adjust the value of the -j option to be not too large that both versions fail so often that it's hard to tell the difference and to not be so small that both versions fail so infrequently as to not make a different. 16 was a good value on my laptop. Here are the last three runs of each version of the test with that value (successes/failures):

Current version of test: 142/50, 159/33, 153/39
This PR's version of the test: 184/8, 191/1, 186/6

/cc @apapirovski

@Trott

Copy link
Copy Markdown
MemberAuthor

@apapirovskiapapirovskiMay 13, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -1 on this change. process.hrtime isn't cheap so it's more — not less — likely to create situations where this test fails on slow platforms.

Actually, scratch that, I see what's making this more reliable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if I'm not wrong we can pass makeReq directly without wrapping it in another function.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Changed. Thanks!

@lpincalpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2018
@richardlau

Copy link
Copy Markdown
Member

#20692 (comment) suggests this doesn't fix the test but instead makes it less likely to fail, i.e., it's still flaky? If so can we reword the commit to something other than fix (maybe improve or make less flaky)?

Trott added 2 commits May 14, 2018 16:09
Check actual expired time rather than relying on a number of calls to
setTimeout() in test-http2-session-timeout more robust.
Fixes: nodejs#20628
@Trott

Copy link
Copy Markdown
MemberAuthor

@richardlau I went with "improve reliability" although I had to abbreviate the test name to keep it under 50 chars. (I removed test- which is implied by the test: for the subsystem. And the full name of the test appears in the main text.)

@Trott

Copy link
Copy Markdown
MemberAuthor

@apapirovski

Copy link
Copy Markdown
Contributor

Landed in 9e4ae56

apapirovski pushed a commit that referenced this pull request May 16, 2018
Check actual expired time rather than relying on a number of calls to
setTimeout() in test-http2-session-timeout more robust.
PR-URL: #20692Fixes: #20628
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Check actual expired time rather than relying on a number of calls to
setTimeout() in test-http2-session-timeout more robust.
PR-URL: #20692Fixes: #20628
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleaxaddaleax mentioned this pull request May 22, 2018
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Check actual expired time rather than relying on a number of calls to
setTimeout() in test-http2-session-timeout more robust.
PR-URL: nodejs#20692Fixes: nodejs#20628
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Sep 19, 2018
Check actual expired time rather than relying on a number of calls to
setTimeout() in test-http2-session-timeout more robust.
PR-URL: nodejs#20692Fixes: nodejs#20628
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Check actual expired time rather than relying on a number of calls to
setTimeout() in test-http2-session-timeout more robust.
PR-URL: nodejs#20692Fixes: nodejs#20628
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Check actual expired time rather than relying on a number of calls to
setTimeout() in test-http2-session-timeout more robust.
Backport-PR-URL: #22850
PR-URL: #20692Fixes: #20628
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Oct 30, 2018
@Trott
Trott deleted the one-less-timer branch January 13, 2022 22:49
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate flaky test-http2-session-timeout

8 participants

@Trott@richardlau@apapirovski@jasnell@lpinca@BridgeAR@trivikr@nodejs-github-bot