This patch is meant to increase the stability of tests for the web API statistics. The original tests had unnecessary dependencies we can remove in order to improve stability.
<rdar://problem/44396413>
Created attachment 349586 [details] Patch
Comment on attachment 349586 [details] Patch Attachment 349586 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9197802 New failing tests: http/tests/webAPIStatistics/font-load-data-collection.html http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html http/tests/webAPIStatistics/navigator-functions-accessed-data-collection.html http/tests/webAPIStatistics/screen-functions-accessed-data-collection.html
Created attachment 349615 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 349586 [details] Patch Attachment 349586 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9198691 New failing tests: http/tests/webAPIStatistics/font-load-data-collection.html http/tests/webAPIStatistics/screen-functions-accessed-data-collection.html http/tests/webAPIStatistics/navigator-functions-accessed-data-collection.html fast/animation/css-animation-resuming-when-visible-with-style-change2.html http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html
Created attachment 349622 [details] Archive of layout-test-results from ews107 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 349586 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349586&action=review > LayoutTests/http/tests/webAPIStatistics/screen-functions-accessed-data-collection.html:16 > testRunner.dumpResourceLoadStatistics(); The testRunner.dump*() calls should likely happen much earlier, before your hostUnderTest initialization above.
Created attachment 349686 [details] Patch
Comment on attachment 349686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349686&action=review > LayoutTests/ChangeLog:8 > + Do you have an explanation why not using js-test would improve stability? > LayoutTests/http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html:16 > testRunner.notifyDone(); Note that the tests should have been using finishJSTest() instead of testRunner.notifyDone() when using js-test.js.
(In reply to Chris Dumez from comment #9) > Comment on attachment 349686 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349686&action=review > > > LayoutTests/ChangeLog:8 > > + > > Do you have an explanation why not using js-test would improve stability? Using js-test added some data to the font loading collection, which made the tests dependent on js-test.js. Thus, if any changes were made to js-test.js the tests for web API statistics would fail. There seems to be some flakiness in clearing the resource load statistics map that are occurring seen here https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FwebAPIStatistics%2Fnavigator-functions-accessed-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Fcanvas-read-and-write-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Ffont-load-data-collection.html%20http%2Ftests%2FwebAPIStatistics%2Fscreen-functions-accessed-data-collection.html I am currently trying to determine why the flakiness exists, but have been unable to reproduce any similar failures locally. > > > LayoutTests/http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html:16 > > testRunner.notifyDone(); > > Note that the tests should have been using finishJSTest() instead of > testRunner.notifyDone() when using js-test.js.
Created attachment 349717 [details] Patch
We can significantly improve the amount of time each test takes by overriding the timer initially relied upon in the ResourceLoadObserver with an existing testRunner function.
Comment on attachment 349717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349717&action=review > LayoutTests/http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html:-18 > - testRunner.statisticsResetToConsistentState(function() { > - testRunner.notifyDone(); > - }); There is no need to call this function anymore?
Created attachment 349781 [details] Patch
Comment on attachment 349781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349781&action=review > LayoutTests/ChangeLog:8 > + Please explain *In the changelog* why this helps with stability. > LayoutTests/http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html:12 > + testRunner.dumpAsText(); I think these testRunner calls should be with the one to testRunner.waitUntilDone(); at the end of the test. Sorry, I know I recommended this locally but I did not see at the time that there was already a section taking care of testRunner calls. Same comment in other tests. > LayoutTests/http/tests/webAPIStatistics/font-load-data-collection.html:39 > + }, 100); Why 100? Either 0 works consistently or this will introduce flakiness.
(In reply to Chris Dumez from comment #15) > Comment on attachment 349781 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349781&action=review > > > LayoutTests/ChangeLog:8 > > + > > Please explain *In the changelog* why this helps with stability. Will do. > > > LayoutTests/http/tests/webAPIStatistics/canvas-read-and-write-data-collection.html:12 > > + testRunner.dumpAsText(); > > I think these testRunner calls should be with the one to > testRunner.waitUntilDone(); at the end of the test. > Sorry, I know I recommended this locally but I did not see at the time that > there was already a section taking care of testRunner calls. > > Same comment in other tests. I'll move it accordingly! > > > LayoutTests/http/tests/webAPIStatistics/font-load-data-collection.html:39 > > + }, 100); > > Why 100? Either 0 works consistently or this will introduce flakiness. 0 does work consistently locally, so I will change it to 0 and monitor the bots to ensure there's no unforeseen flakiness.
Created attachment 349787 [details] Patch
Comment on attachment 349787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349787&action=review r=me > LayoutTests/http/tests/webAPIStatistics/font-load-data-collection.html:34 > + // Adds a timeout to allow font loads to be recorded Style: period needed at the end of the comment.
Created attachment 349796 [details] Patch
(In reply to Woodrow Wang from comment #19) > Created attachment 349796 [details] > Patch Made a mistake rebasing with style fix, so I am reuploading the patch.
Created attachment 349804 [details] Patch
Comment on attachment 349804 [details] Patch Clearing flags on attachment: 349804 Committed r236021: <https://trac.webkit.org/changeset/236021>
All reviewed patches have been landed. Closing bug.
This test: http/tests/webAPIStatistics/font-load-data-collection.html is still flakey after https://trac.webkit.org/changeset/236021/webkit I do want to note that the test is now passing faster. before it took 5 seconds now it passes or fails in under 1 second. This also seems to be the only test still failing from the tests edited in this patch. History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FwebAPIStatistics%2Ffont-load-data-collection.html Diff: https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r236073%20(6775)/http/tests/webAPIStatistics/font-load-data-collection-pretty-diff.html
(In reply to Truitt Savell from comment #24) > This test: http/tests/webAPIStatistics/font-load-data-collection.html > > is still flakey after https://trac.webkit.org/changeset/236021/webkit > > I do want to note that the test is now passing faster. before it took 5 > seconds now it passes or fails in under 1 second. This also seems to be the > only test still failing from the tests edited in this patch. This was an expected enhancement by this patch :) > > History: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=http%2Ftests%2FwebAPIStatistics%2Ffont-load-data- > collection.html > > Diff: > https://build.webkit.org/results/ > Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r236073%20(6775)/http/tests/ > webAPIStatistics/font-load-data-collection-pretty-diff.html https://bugs.webkit.org/show_bug.cgi?id=189632 is a speculative fix for the flakiness of the font-load-data-collection.html test.
(In reply to Woodrow Wang from comment #25) > (In reply to Truitt Savell from comment #24) > https://bugs.webkit.org/show_bug.cgi?id=189632 is a speculative fix for the > flakiness of the font-load-data-collection.html test. http/tests/webAPIStatistics/font-load-data-collection.html continues to be extremely flaky on the bots after that fix.
(In reply to Ryan Haddad from comment #26) > (In reply to Woodrow Wang from comment #25) > > (In reply to Truitt Savell from comment #24) > > https://bugs.webkit.org/show_bug.cgi?id=189632 is a speculative fix for the > > flakiness of the font-load-data-collection.html test. > > http/tests/webAPIStatistics/font-load-data-collection.html continues to be > extremely flaky on the bots after that fix. I'm actively investigating the flakiness and am working on https://bugs.webkit.org/show_bug.cgi?id=189684 as another speculative fix. I am not able to reproduce the flakiness locally as of now, so if this speculative fix does not work, I will mark the test as flaky.