FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix flaky timers-block-eventloop test by apapirovski · Pull Request #18567 · nodejs/node · GitHub

/ node Public

test: fix flaky timers-block-eventloop test - #18567

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:fix-flaky-timers-block-eventloop
ConversationCommits (1)ChecksFiles changed
Closed

test: fix flaky timers-block-eventloop test#18567
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:fix-flaky-timers-block-eventloop

Conversation

apapirovski commented Feb 4, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

test: fix flaky timers-block-eventloop test
bbfb411 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.
nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 4, 2018
apapirovski commented Feb 4, 2018
View reviewed changes
Comment thread test/sequential/test-timers-block-eventloop.js
setTimeout(function() {
fs.stat('/dev/nonexistent', () => {
assert(!called);
called = true;

apapirovski Feb 4, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is here to prevent an infinite loop before anyone asks :)

apapirovski commented Feb 6, 2018

Copy link
Copy Markdown
Contributor Author

ping @nodejs/collaborators — this is a test that has failed a few times recently. I would appreciate a review so we can continue making the CI a little less flaky.

mcollina approved these changes Feb 6, 2018
View reviewed changes

mcollina left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

BridgeAR commented Feb 6, 2018

Copy link
Copy Markdown
Member

Does this really still test the original issue?

apapirovski commented Feb 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@BridgeAR yep, just in a very simplified way. It fails on all the same Node versions as the original test case. In fact, the previous test case wasn't even strictly correct which is why it was flaky. Here's an outline of what's going on:

  1. A "now" time gets recorded, let's call it 0
  2. First interval executes (close to no run time), gets added to the back of the interval list with startTime of 0 or 1
  3. Second interval executes, runs for 20ms
  4. We come back to the first interval since it is at the end of the list, the difference between 0 and 0 or 1 is smaller than 10, so it enters the if condition for deferral
  5. it checks the new now value which is now 20+ms ahead of the old now value, so the actual diff is now > 10ms, this means that it sets the timeRemaining to 0 or 1 (depending on whether the bug exists)
  6. If it sets it to 0, then when it comes back to C++, this list fires again and the test case fails

apapirovski commented Feb 7, 2018

Copy link
Copy Markdown
Contributor Author

@BridgeAR Did the explanation above make sense or do you still have concerns about this PR? I would like to land this.

BridgeAR approved these changes Feb 8, 2018
View reviewed changes

BridgeAR left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the detailed description. LGTM

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 8, 2018
apapirovski added a commit that referenced this pull request Feb 8, 2018
test: fix flaky timers-block-eventloop test
6963a93 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

PR-URL: #18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

apapirovski commented Feb 8, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 6963a93

apapirovski closed this Feb 8, 2018
apapirovski deleted the fix-flaky-timers-block-eventloop branch February 8, 2018 20:34
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: fix flaky timers-block-eventloop test
59c1cfc 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

PR-URL: #18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: fix flaky timers-block-eventloop test
5d54a04 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

PR-URL: #18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: fix flaky timers-block-eventloop test
af3841b 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

PR-URL: #18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Feb 21, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
test: fix flaky timers-block-eventloop test
a667ac1 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

PR-URL: #18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
test: fix flaky timers-block-eventloop test
ab8edc9 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

PR-URL: #18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
gibfahn pushed a commit that referenced this pull request Apr 13, 2018
test: fix flaky timers-block-eventloop test
44ab850 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

PR-URL: #18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Apr 13, 2018
Merged
MylesBorins mentioned this pull request May 2, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
test: fix flaky timers-block-eventloop test
4a1c10f 
Due to extensive reliance on timings and the fs module, this test
is currently inherently flaky. Refactor it to simply use setImmediate
and only one busy loop.

PR-URL: nodejs#18567
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL