Skip to content

test: relax timer check in test-report-uv-handles.js - #26434

Merged
richardlau merged 1 commit into
nodejs:masterfrom
richardlau:relaxtimers
Mar 6, 2019
Merged

test: relax timer check in test-report-uv-handles.js#26434
richardlau merged 1 commit into
nodejs:masterfrom
richardlau:relaxtimers

Conversation

@richardlau

Copy link
Copy Markdown
Member

The underlying JavaScript runtime may schedule tasks at its discretion
so there may be more timer handles than the one created by the test.

Refs: #25852 (comment)

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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Mar 4, 2019
@richardlaurichardlau mentioned this pull request Mar 4, 2019

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.

Ah I left this in accidentally. I actually think printing the uv handle section of the report is quite useful for debugging failures in this test, so I'm inclined to leave it in unless there are objections.

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 don't mind keeping this. It might not be necessary though. If you use any of the report validation functions in test/common/report.js, the full report is printed on failure. That said, if you want to leave this, I'm OK with it.

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.

In #25852 the test was failing before the call to helper.validateContent().

@richardlau

Copy link
Copy Markdown
MemberAuthor

@richardlaurichardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2019
@richardlau

richardlau commented Mar 4, 2019

Copy link
Copy Markdown
MemberAuthor

The underlying JavaScript runtime may schedule tasks at its discretion
so there may be more timer handles than the one created by the test.
PR-URL: nodejs#26434
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@richardlau
richardlau merged commit 80209cc into nodejs:masterMar 6, 2019
@richardlau

Copy link
Copy Markdown
MemberAuthor

Landed in 80209cc

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
The underlying JavaScript runtime may schedule tasks at its discretion
so there may be more timer handles than the one created by the test.
PR-URL: nodejs#26434
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
BridgeAR pushed a commit that referenced this pull request Mar 14, 2019
The underlying JavaScript runtime may schedule tasks at its discretion
so there may be more timer handles than the one created by the test.
PR-URL: #26434
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
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.

7 participants

@richardlau@nodejs-github-bot@addaleax@lpinca@cjihrig@gireeshpunathil@BridgeAR