-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
test: fix flaky wpt/test-timers #37691
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I don't understand this well enough to review but just wanted to say thanks for all the work on these tasks 🙇 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Feel free to skip the tests again in the mean time |
The test that fails ( setup({ single_test: true });
setTimeout(done, -100);
setTimeout(assert_unreached, 10); While the ordering may be guaranteed in browsers, I don't think it is in Node.js due to the linked list stuff that underpins timers. But maybe I'm confused. /ping @nodejs/timers for confirmation. If the ordering in the above case is not in fact guaranteed in Node.js, then skipping the test is certainly the right thing to do. |
@Trott I think that there's something wrong with done(). Rewriting the test to the following is not flaky on my machine (while the original is): setup({ single_test: true });
setTimeout(() => { clearTimeout(y); done(); }, -100);
var y = setTimeout(assert_unreached, 10); I assume that |
OK. The Parallel to Simple change fixed most of them. The one it didn't fix is now marked flaky in the status file. This is ready for review. |
Stress test against this PR: https://ci.nodejs.org/view/Stress/job/node-stress-single-test/224/ ✅ |
Stress test and CI were both green on the first run. It would be great to get this frequent-CI-failure addressed. @nodejs/testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Moving test harness from Parallel to Simple resolves most of the failures. negative-settimeout.any.js still needs to be marked flaky. Refs: nodejs#37672 PR-URL: nodejs#37691 Fixes: nodejs#37672 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Landed in 66f0eb7" |
Change the wpt tests to not run in parallel in our Python test runner.
On my local machine, this changes the time needed to run the suite from
about 2 seconds to about 5 seconds, but it makes the test suite much
more reliable.
Fixes: #37672