Skip to content

test: improve assertions in pummel/test-timers - #35216

Merged
Trott merged 1 commit into
nodejs:masterfrom
Trott:wiggle-room
Sep 20, 2020
Merged

test: improve assertions in pummel/test-timers#35216
Trott merged 1 commit into
nodejs:masterfrom
Trott:wiggle-room

Conversation

@Trott

Copy link
Copy Markdown
Member
  • Timers should not fire early. Check for that.
  • Allow the wiggle-room to increase on subsequent iterations of
    intervals.
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 Sep 15, 2020
@Trott

Copy link
Copy Markdown
MemberAuthor

@Trott

Copy link
Copy Markdown
MemberAuthor

This fixes the flakiness of the test on macOS in CI.

master branch, 71 failures in 200 runs: https://ci.nodejs.org/job/node-stress-single-test/175/

this PR, 0 failures in 200 runs: https://ci.nodejs.org/job/node-stress-single-test/176/

@nodejs/testing

@Trott

Copy link
Copy Markdown
MemberAuthor

@nodejs/timers

@TrottTrott added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Sep 18, 2020
@Trott

Copy link
Copy Markdown
MemberAuthor

@nodejs/collaborators This could use reviews.

@Flarna

Copy link
Copy Markdown
Member

As far as I know windows timers sometimes fire slightly earlier. At least I faced issues with flaky timer tests in an earlier project because of this.
It may depend on the timeout, the windows version and the load on the machine.

I did a fast test and at least for a 16 and a 30ms timeout I saw that callback was called 0.x ms earlier then expected.
I measured time using process.hrtime() not sure if this makes a difference.

@Trott

Copy link
Copy Markdown
MemberAuthor

As far as I know windows timers sometimes fire slightly earlier. At least I faced issues with flaky timer tests in an earlier project because of this.
It may depend on the timeout, the windows version and the load on the machine.

I did a fast test and at least for a 16 and a 30ms timeout I saw that callback was called 0.x ms earlier then expected.
I measured time using process.hrtime() not sure if this makes a difference.

Firing less than 1ms early based on process.hrtime() seems plausible to me due to rounding errors. I'm inclined to leave this as is and then allow for 1 millisecond early if it proves to be flaky.

* Timers should not fire early. Check for that.
* Allow the wiggle-room to increase on subsequent iterations of
intervals.
PR-URL: nodejs#35216
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
@Trott

Copy link
Copy Markdown
MemberAuthor

Landed in 87c433e

@Trott
Trott merged commit 87c433e into nodejs:masterSep 20, 2020
@Trott
Trott deleted the wiggle-room branch September 20, 2020 14:45
ruyadorno pushed a commit that referenced this pull request Sep 21, 2020
* Timers should not fire early. Check for that.
* Allow the wiggle-room to increase on subsequent iterations of
intervals.
PR-URL: #35216
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
@ruyadornoruyadorno mentioned this pull request Sep 21, 2020
4 tasks
addaleax pushed a commit that referenced this pull request Sep 22, 2020
* Timers should not fire early. Check for that.
* Allow the wiggle-room to increase on subsequent iterations of
intervals.
PR-URL: #35216
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
@codebyterecodebytere mentioned this pull request Sep 28, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
* Timers should not fire early. Check for that.
* Allow the wiggle-room to increase on subsequent iterations of
intervals.
PR-URL: nodejs#35216
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flaky-testIssues and PRs related to the tests with unstable failures on the CI.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@Trott@Flarna@antsmartian@benjamingr@puzpuzpuz@nodejs-github-bot