[macOS Sierra] Fix flaky test: media/controls/picture-in-picture.html

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Ada Chan]

media/controls/picture-in-picture.html is flaky on macOS Sierra.

@@ -21,13 +21,13 @@
 Test for the pip placeholder visibility in pip mode
 
 PASS: Should be in pip mode
-PASS: Inline placeholder should be visible at this point
+FAIL: Inline placeholder should be visible at this point Expected to not contain "hidden". Actual: "hidden picture-in-picture"
 
 Test for the pip placeholder visibility in pip mode without a 'controls' attribute
 
 PASS: Should still be in pip mode
 PASS: No controls attribute
-PASS: Inline placeholder should still be visible
+FAIL: Inline placeholder should still be visible Expected to not contain "hidden". Actual: "hidden picture-in-picture"
 
 Testing finished.


[Ada Chan]

<rdar://problem/27574303>

[Ada Chan]

Created attachment 285672 [details]
Patch

[Eric Carlson]

Comment on attachment 285672 [details]
Patch

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

> LayoutTests/media/controls/picture-in-picture.html:75
> +                if (stateForPlaceholder.className.indexOf("hidden") >= 0) {
> +                    // Use 33 to match PlaceholderPollingDelay in mediaControlsApple.js.
> +                    setTimeout(pollPIPPlaceholderVisibilityChange, 33);
> +                    return;
> +                }

Nit: in case something goes wrong I think it would be better to fail and log an error message after some maximum number of retrys, rather than trying until the test times out.

[Ada Chan]

(In reply to comment #3)
> Comment on attachment 285672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285672&action=review
> 
> > LayoutTests/media/controls/picture-in-picture.html:75
> > +                if (stateForPlaceholder.className.indexOf("hidden") >= 0) {
> > +                    // Use 33 to match PlaceholderPollingDelay in mediaControlsApple.js.
> > +                    setTimeout(pollPIPPlaceholderVisibilityChange, 33);
> > +                    return;
> > +                }
> 
> Nit: in case something goes wrong I think it would be better to fail and log
> an error message after some maximum number of retrys, rather than trying
> until the test times out.

OK, I'll set a maximum number retrys to 10.

Thanks for the review!

[Ada Chan]

Committed:
https://trac.webkit.org/changeset/204304

[Alexey Proskuryakov]

Comment on attachment 285672 [details]
Patch

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

>>> LayoutTests/media/controls/picture-in-picture.html:75
>>> +                }
>> 
>> Nit: in case something goes wrong I think it would be better to fail and log an error message after some maximum number of retrys, rather than trying until the test times out.
> 
> OK, I'll set a maximum number retrys to 10.
> 
> Thanks for the review!

10 retries is just 330 ms. I suggest waiting ~10 seconds before failing.

[Ada Chan]

(In reply to comment #6)
> Comment on attachment 285672 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285672&action=review
> 
> >>> LayoutTests/media/controls/picture-in-picture.html:75
> >>> +                }
> >> 
> >> Nit: in case something goes wrong I think it would be better to fail and log an error message after some maximum number of retrys, rather than trying until the test times out.
> > 
> > OK, I'll set a maximum number retrys to 10.
> > 
> > Thanks for the review!
> 
> 10 retries is just 330 ms. I suggest waiting ~10 seconds before failing.

The placeholder is not guaranteed to be visible right when the presentation mode changes but it should be visible basically very soon after.  If it takes in the order of seconds to become visible, then that's a bug.