Summary: | imported/w3c/web-platform-tests/streams/readable-streams/general.https.html is a flaky failure | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||
Component: | Tools / Tests | Assignee: | youenn fablet <youennf> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, jiewen_tan, lforschler, ryanhaddad, youennf | ||||||
Priority: | P2 | ||||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2016-03-22 11:24:40 PDT
It's also frequently reported as a crash on EWS, although I'm not sure if it's this test's fault. Committed r199507: <http://trac.webkit.org/changeset/199507> Reopen to track the fix. It seems that on slow bots, the order of the promise resolution does not play well with DOM timers. It seems that this can be solved by changing delay(1) to delay(10) in imported/w3c/web-platform-tests/streams/readable-streams/general.js. Will that make any tests take 10 seconds as part of normal execution, or is that code just to handle failing tests nicely? Having tests take 10 seconds would be pretty horrible. Created attachment 276705 [details]
Patch
(In reply to comment #5) > Will that make any tests take 10 seconds as part of normal execution, or is > that code just to handle failing tests nicely? > > Having tests take 10 seconds would be pretty horrible. It is 10 milliseconds. The issue can probably be summarized to the following code: let pullCount1 = 0; let pullCount2 = 0; Promise.resolve().then(() => { ++pullCount1; Promise.resolve().then(() => { ++pullCount2; }); }); return delay(1).then(() => { // Not flaky assert_equals(pullCount1, 1); // May be flaky assert_equals(pullCount2, 1); }); By going to delay(10), the flakiness does not happen anymore on my machine. The test could probably be improved to further limit the flakiness (returning a resolved promise to the start method). This can be tackled directly when upstreaming that change to W3C WPT repo. Comment on attachment 276705 [details]
Patch
Seems fine as a temporary workaround.
Generally speaking, bots tend to still have flakiness when increasing timer delays from 1ms to 100ms, because that's just how much variability we get when running the tests. We can't really have any non-zero timeouts as part of normal test flow, they are only useful when catching failures (and then set them to 20 seconds or so).
(In reply to comment #8) > Comment on attachment 276705 [details] > Patch > > Seems fine as a temporary workaround. > > Generally speaking, bots tend to still have flakiness when increasing timer > delays from 1ms to 100ms, because that's just how much variability we get > when running the tests. We can't really have any non-zero timeouts as part > of normal test flow, they are only useful when catching failures (and then > set them to 20 seconds or so). Thanks for the review. 10 ms is used in several streams tests without causing issue apparently. I'll land it and see whether this is a good enough fix. If so, I will upstream this change or a more deterministic version of the test to WPT or streams repo Comment on attachment 276705 [details] Patch Clearing flags on attachment: 276705 Committed r199736: <http://trac.webkit.org/changeset/199736> All reviewed patches have been landed. Closing bug. test is still flaky, although less than before. Let's fix it properly. Created attachment 281722 [details]
Patch
*** Bug 153095 has been marked as a duplicate of this bug. *** Comment on attachment 281722 [details] Patch Clearing flags on attachment: 281722 Committed r202338: <http://trac.webkit.org/changeset/202338> All reviewed patches have been landed. Closing bug. |