Layout Test streams/reference-implementation/readable-stream.html is flaky

[Tim Horton]

The following layout test recently started flaking on Mac:

streams/reference-implementation/readable-stream.html

See:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=streams%2Freference-implementation%2Freadable-stream.html

[Tim Horton]

Youenn, this looks like a feature you've been working on. Any ideas why this might have recently started flaking?

[youenn fablet]

Tim, thanks for spotting these flakiness.
Calvaris, it seems the changes brought on the tests when inverting the order of the promise resolution have some bad effects. Will you have some time to check these?

[Xabier Rodríguez Calvar]

(In reply to comment #2)
> Tim, thanks for spotting these flakiness.
> Calvaris, it seems the changes brought on the tests when inverting the order
> of the promise resolution have some bad effects. Will you have some time to
> check these?

Sure, I'm on it now.

[Xabier Rodríguez Calvar]

Created attachment 259755 [details]
Patch

It is true that timeout hitting earlier happens a bit more often after the promise changes but it was already happening before aparently. Let's try to increase the timeout and see if the problem disappears.

[youenn fablet]

(In reply to comment #4)
> Created attachment 259755 [details]
> Patch
> 
> It is true that timeout hitting earlier happens a bit more often after the
> promise changes but it was already happening before aparently. Let's try to
> increase the timeout and see if the problem disappears.

Then, maybe we could rewrite the test cases a bit to reduce the flakiness  probability.
In particular we could call setTimeout at the latest time.

For instance, in test 6, we could move the call to setTimeout within startPromise.then() callback.

[Xabier Rodríguez Calvar]

Created attachment 259791 [details]
Patch

Set timeouts later as suggested by Youenn. I thought of that too but I was reluctant because we'd lose the structure of the original reference test, but I decided to add a comment to help with that.

[Xabier Rodríguez Calvar]

This flakiness seemed important. Can we get this reviewed?

[Alexey Proskuryakov]

Comment on attachment 259791 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=259791&action=review

Sad, but looks like there is no other way to test for this.

> LayoutTests/streams/reference-implementation/readable-stream.html:273
> +        // In the original test this timeout is outside the promise fulfillment callback, but it makes test flakier so
> +        // we better move it inside here.

This comment is not helpful without a reference to original. Are we modifying a W3C test here?

I would just remove the comment (and other copies of it) unless there is a really important reason for it - otherwise anyone interested can just blame the source to see history.

[youenn fablet]

If feasible, one option might be to backport these changes to the reference tests.

[Xabier Rodríguez Calvar]

Created attachment 260141 [details]
Patch for landing

[Xabier Rodríguez Calvar]

(In reply to comment #9)
> If feasible, one option might be to backport these changes to the reference
> tests.

I'll propose the change, let's see if Domenic accepts it.

[WebKit Commit Bot]

Comment on attachment 260141 [details]
Patch for landing

Clearing flags on attachment: 260141

Committed r189093: <http://trac.webkit.org/changeset/189093>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xabier Rodríguez Calvar]

(In reply to comment #11)
> (In reply to comment #9)
> > If feasible, one option might be to backport these changes to the reference
> > tests.
> 
> I'll propose the change, let's see if Domenic accepts it.

Done:

https://github.com/whatwg/streams/pull/392