Bug 101208 – fast/events/popup-allowed-from-gesture-initiated-event.html is flaky

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

[Chris Dumez]

fast/events/popup-allowed-from-gesture-initiated-event.html is flaky.

We sometimes get the following diff on EFL port:
--- /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/fast/events/popup-allowed-from-gesture-initiated-event-expected.txt
+++ /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/layout-test-results/fast/events/popup-allowed-from-gesture-initiated-event-actual.txt
@@ -1,4 +1,3 @@
 Click Here  Click Here Too
 PASS win is non-null.
-PASS win is non-null.

[Chris Dumez]

Created attachment 172315 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 172315 [details]
Patch

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

> LayoutTests/fast/events/popup-allowed-from-gesture-initiated-event.html:34
> +                setTimeout(doneHandler, 1);

why not just 0?

> LayoutTests/fast/events/popup-allowed-from-gesture-initiated-event.html:41
> +                    setTimeout(doneHandler, 1);

So it can take a while to close? hmm, so we are just making it less flaky?

[Chris Dumez]

Comment on attachment 172315 [details]
Patch

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

>> LayoutTests/fast/events/popup-allowed-from-gesture-initiated-event.html:34
>> +                setTimeout(doneHandler, 1);
> 
> why not just 0?

I honestly copied this from another test (fast/dom/Window/new-window-opener.html). 0 would certainly work but since the window takes a while to close apparently, I would actually do the opposite and put a bigger value here (like 250 milliseconds) to avoid invoking doneHandler() too many times.

>> LayoutTests/fast/events/popup-allowed-from-gesture-initiated-event.html:41
>> +                    setTimeout(doneHandler, 1);
> 
> So it can take a while to close? hmm, so we are just making it less flaky?

It completely resolves flakiness for me. I don't see why this would make it only "less" flaky. We are properly waiting for the window to close so no flakiness is possible here.

[Kenneth Rohde Christiansen]

Comment on attachment 172315 [details]
Patch

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

>>> LayoutTests/fast/events/popup-allowed-from-gesture-initiated-event.html:41
>>> +                    setTimeout(doneHandler, 1);
>> 
>> So it can take a while to close? hmm, so we are just making it less flaky?
> 
> It completely resolves flakiness for me. I don't see why this would make it only "less" flaky. We are properly waiting for the window to close so no flakiness is possible here.

Sure, I See that now. It keeps calling until it is closed. So what if it never closes? Should you have a recursion count?

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 172315 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172315&action=review
> 
> >>> LayoutTests/fast/events/popup-allowed-from-gesture-initiated-event.html:41
> >>> +                    setTimeout(doneHandler, 1);
> >> 
> >> So it can take a while to close? hmm, so we are just making it less flaky?
> > 
> > It completely resolves flakiness for me. I don't see why this would make it only "less" flaky. We are properly waiting for the window to close so no flakiness is possible here.
> 
> Sure, I See that now. It keeps calling until it is closed. So what if it never closes? Should you have a recursion count?

Other tests I've seen that wait for a window to close() don't seem to do anything specific regarding recursion. Actually, what will happen is that the test will time out (and therefore fail).

[WebKit Review Bot]

Comment on attachment 172315 [details]
Patch

Clearing flags on attachment: 172315

Committed r133461: <http://trac.webkit.org/changeset/133461>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.