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

backport v4.x: test: fix flaky test-tls-wrap-timeout #12567

Closed

Conversation

BethGriggs
Copy link
Member

Competing timers were causing a race condition and thus the test was
flaky. Instead, we check an object property on process exit.

Fixes: #7650
PR-URL: #7857
Reviewed-By: Santiago Gimeno [email protected]
Reviewed-By: Fedor Indutny [email protected]
Reviewed-By: jasnell - James M Snell [email protected]

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

Competing timers were causing a race condition and thus the test was
flaky. Instead, we check an object property on process exit.

Fixes: nodejs#7650
PR-URL: nodejs#7857
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. v4.x labels Apr 21, 2017
@gibfahn
Copy link
Member

gibfahn commented Apr 21, 2017

@MylesBorins it'd be really helpful to have this land on v4.x if possible. This test is flaky, and we've seen it fail several times.

I'm also not sure why it wasn't backported a while ago, as it landed in master last July.

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Apr 21, 2017
@MylesBorins
Copy link
Contributor

@gibfahn by the maintenance contract we are no longer supposed to be backporting fixes to tests. It didn't land because we have not been super adamant about making sure tests that don't land cleanly backport.

Personally I'm -1 on making exceptions for tests, but willing to reconsider if @nodejs/lts thinks this should land

@gibfahn
Copy link
Member

gibfahn commented Apr 28, 2017

@MylesBorins did you say you were going to land this, or should we close this PR?

MylesBorins pushed a commit that referenced this pull request May 2, 2017
Competing timers were causing a race condition and thus the test was
flaky. Instead, we check an object property on process exit.

Fixes: #7650
Backport-PR-URL: #12567
PR-URL: #7857
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

landed in 6040efd

@MylesBorins MylesBorins closed this May 2, 2017
@BethGriggs BethGriggs deleted the backport-7857-to-v4 branch February 21, 2018 15:36
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.

7 participants