Bug 155760 – imported/w3c/web-platform-tests/streams/readable-streams/general.https.html is a flaky failure

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

imported/w3c/web-platform-tests/streams/readable-streams/general.https.html fails somewhat frequently:

@@ -13,7 +13,7 @@
 PASS ReadableStream should be able to enqueue different objects. 
 PASS ReadableStream: if pull rejects, it should error the stream 
 PASS ReadableStream: should only call pull once upon starting the stream 
-PASS ReadableStream: should call pull when trying to read from a started, empty stream 
+FAIL ReadableStream: should call pull when trying to read from a started, empty stream assert_equals: pull should be called when read is called expected 2 but got 1
 PASS ReadableStream: should only call pull once on a non-empty stream read from before start fulfills 
 PASS ReadableStream: should only call pull once on a non-empty stream read from after start fulfills 
 PASS ReadableStream: should call pull in reaction to read()ing the last chunk, if not draining 

http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fstreams%2Freadable-streams%2Fgeneral.https.html

[Alexey Proskuryakov]

It's also frequently reported as a crash on EWS, although I'm not sure if it's this test's fault.

[Jiewen Tan]

Committed r199507: <http://trac.webkit.org/changeset/199507>

[Jiewen Tan]

Reopen to track the fix.

[youenn fablet]

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.

[Alexey Proskuryakov]

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.

[youenn fablet]

Created attachment 276705 [details]
Patch

[youenn fablet]

(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.

[Alexey Proskuryakov]

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).

[youenn fablet]

(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

[WebKit Commit Bot]

Comment on attachment 276705 [details]
Patch

Clearing flags on attachment: 276705

Committed r199736: <http://trac.webkit.org/changeset/199736>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

test is still flaky, although less than before.
Let's fix it properly.

[youenn fablet]

Created attachment 281722 [details]
Patch

[youenn fablet]

*** Bug 153095 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 281722 [details]
Patch

Clearing flags on attachment: 281722

Committed r202338: <http://trac.webkit.org/changeset/202338>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.