Bug 197063

Summary: [Mac Debug] Layout Test resize-observer/observe-element-from-other-frame.html is a flaky failure
Product: WebKit Reporter: Shawn Roberts <sroberts>
Component: Tools / TestsAssignee: cathiechen <cathiechen>
Status: RESOLVED FIXED    
Severity: Normal CC: ajuma, cathiechen, commit-queue, fred.wang, lforschler, rniwa, sabouhallawa, simon.fraser, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=157743
https://bugs.webkit.org/show_bug.cgi?id=177484
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Shawn Roberts 2019-04-18 10:41:28 PDT
The following layout test is flaky on Mac Debug 

resize-observer/observe-element-from-other-frame.html

Probable cause:

Test was added in https://trac.webkit.org/changeset/243643/webkit and is a flaky failure since

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=resize-observer%2Fobserve-element-from-other-frame.html

Diff:

--- /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/resize-observer/observe-element-from-other-frame-expected.txt
+++ /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/resize-observer/observe-element-from-other-frame-actual.txt
@@ -1,5 +1,5 @@
 
 
 PASS ResizeObserver implemented 
-PASS test0: Observe element from other frame 
+FAIL test0: Observe element from other frame assert_equals: expected "success" but got "fail"
Comment 1 Radar WebKit Bug Importer 2019-04-18 10:48:56 PDT
<rdar://problem/50020775>
Comment 2 cathiechen 2019-04-22 02:04:20 PDT
Created attachment 367929 [details]
Patch
Comment 3 cathiechen 2019-04-22 03:22:17 PDT
Created attachment 367933 [details]
Patch
Comment 4 Frédéric Wang (:fredw) 2019-04-22 03:29:05 PDT
Comment on attachment 367933 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        In order to reduce this failure, extend ResizeTestHelper.TIMEOUT to 1000ms like 177484.

like in bug 177484
Comment 5 cathiechen 2019-04-22 03:32:18 PDT
Created attachment 367934 [details]
Patch
Comment 6 Simon Fraser (smfr) 2019-04-22 10:32:21 PDT
Comment on attachment 367929 [details]
Patch

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

> LayoutTests/resize-observer/resources/resizeTestHelper.js:55
> +ResizeTestHelper.TIMEOUT = 1000;

Does this make a bunch of tests take 1s to run? That's really long.
Comment 7 cathiechen 2019-04-22 17:00:10 PDT
Comment on attachment 367929 [details]
Patch

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

Hi Simon,
Thanks:)

>> LayoutTests/resize-observer/resources/resizeTestHelper.js:55
>> +ResizeTestHelper.TIMEOUT = 1000;
> 
> Does this make a bunch of tests take 1s to run? That's really long.

Normally, this wouldn't affect the test time if the tests running as expected. But there are 4 tests in imported/w3c/web-platform-tests/resize-observer which would take 1s to test unwanted notification. They are test4(), test5(), test9() in notify.html and test3() in observe.html.

If not running as expected, because they are promise tests, this would extend timeout time once for each resizeObserver test file at most.
Comment 8 Ryosuke Niwa 2019-04-22 19:38:31 PDT
We could try 500ms first if any. It would be better if we can somehow expedite / emulate the rendering timing though. e.g. add some internal method to speed up the process only inside WebKitTestRunner.
Comment 9 cathiechen 2019-04-22 20:26:44 PDT
Created attachment 368008 [details]
Patch
Comment 10 Ryosuke Niwa 2019-04-22 20:31:04 PDT
Comment on attachment 368008 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/resize-observer/resources/resizeTestHelper.js:55
> -ResizeTestHelper.TIMEOUT = 1000;
> +ResizeTestHelper.TIMEOUT = 500;

Oh, please don't modify the imported tests. They're supposed to be only modified in the upstream first.
It's fine if you're uploading this patch just to see if EWS is happy with it
but you should probably manually run it like 100-500 times to make sure it's actually not flaky.
If you're on macOS, then Instruments has the ability to reduce the number of effective cores.
Reducing it to 2 cores, for example, makes it easier to induce flakiness.
Comment 11 cathiechen 2019-04-22 20:32:02 PDT
(In reply to Ryosuke Niwa from comment #8)
> We could try 500ms first if any. It would be better if we can somehow
> expedite / emulate the rendering timing though. e.g. add some internal
> method to speed up the process only inside WebKitTestRunner.

Hi Ryosuke,

Thanks:)

500ms seems work well in my local env.
I also changed the one in imported/w3c/web-platform-tests/resize-observer/resources/resizeTestHelper.js to make them consistent.
Comment 12 cathiechen 2019-04-22 21:29:36 PDT
Comment on attachment 368008 [details]
Patch

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

>> LayoutTests/imported/w3c/web-platform-tests/resize-observer/resources/resizeTestHelper.js:55
>> +ResizeTestHelper.TIMEOUT = 500;
> 
> Oh, please don't modify the imported tests. They're supposed to be only modified in the upstream first.
> It's fine if you're uploading this patch just to see if EWS is happy with it
> but you should probably manually run it like 100-500 times to make sure it's actually not flaky.
> If you're on macOS, then Instruments has the ability to reduce the number of effective cores.
> Reducing it to 2 cores, for example, makes it easier to induce flakiness.

Ah, OK, I'll remove the change in imported tests.

And I tried reducing the effective core number like you said, it seems to work well. Thanks:)
Comment 13 cathiechen 2019-04-22 21:45:45 PDT
Created attachment 368010 [details]
Patch
Comment 14 Frédéric Wang (:fredw) 2019-04-24 08:53:57 PDT
(In reply to Ryosuke Niwa from comment #10)
> Oh, please don't modify the imported tests. They're supposed to be only
> modified in the upstream first.

Note that this test was already changed in https://trac.webkit.org/changeset/244182/webkit#file10 ; probably someone should do the change upstream (if that's not already done).
Comment 15 WebKit Commit Bot 2019-04-24 09:21:37 PDT
Comment on attachment 368010 [details]
Patch

Clearing flags on attachment: 368010

Committed r244593: <https://trac.webkit.org/changeset/244593>
Comment 16 WebKit Commit Bot 2019-04-24 09:21:38 PDT
All reviewed patches have been landed.  Closing bug.