Summary: | REGRESSION(r275267) [GLIB] API test /webkit/WebKitWebsiteData/configuration is failing | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Lauro Moura <lmoura> | ||||||
Component: | WebKitGTK | Assignee: | Lauro Moura <lmoura> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bugs-noreply, cgarcia, webkit-bug-importer | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=223808 https://bugs.webkit.org/show_bug.cgi?id=234497 |
||||||||
Attachments: |
|
Description
Lauro Moura
2021-04-04 20:33:43 PDT
At least two other tests are flaky timing out more frequently since the same revision in the EWS bots: /WebKit2Gtk/TestBackForwardList:/webkit/BackForwardList/navigation /WebKit2Gtk/TestWebsiteData:/webkit/WebKitWebsiteData/itp Created attachment 438007 [details]
Patch
Comment on attachment 438007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438007&action=review > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebsiteData.cpp:807 > + // Make sure the cache was opened before asking for it > + { > + unsigned triesCount = 4; > + bool createdCache = false; > + while (!createdCache && triesCount--) { > + auto domCacheResult = test->runJavaScriptAndWaitUntilFinished("domCacheOpened", nullptr); > + auto jsResult = webkit_javascript_result_get_js_value(domCacheResult); > + g_assert_true(jsc_value_is_boolean(jsResult)); > + if (jsc_value_to_boolean(jsResult)) > + createdCache = true; > + else > + test->wait(0.25); > + } I guess we could move this duplicated code to WebsiteDataTest class. > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:256 > +void WebViewTest::waitUntilFileExists(const char *filename, double intervalInSeconds, unsigned tries) Why are intervalInSeconds and tries parameters if we never pass them explicitly? or did I miss any call? > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:260 > + g_assert_true(g_file_test(filename, G_FILE_TEST_EXISTS)); I'm not sure about asserting here in a method called wait until. I would either leave the infinite loop and test will timeout if file is never created, or keep the tries but assert in the callers. Or keep the asser here, but rename the method as something like assertFileIsCreated() Comment on attachment 438007 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=438007&action=review >> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebsiteData.cpp:807 >> + } > > I guess we could move this duplicated code to WebsiteDataTest class. Ok >> Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:256 >> +void WebViewTest::waitUntilFileExists(const char *filename, double intervalInSeconds, unsigned tries) > > Why are intervalInSeconds and tries parameters if we never pass them explicitly? or did I miss any call? I added them while testing different timeouts, and left them already in case we need it. I'll remove it. >> Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:260 >> + g_assert_true(g_file_test(filename, G_FILE_TEST_EXISTS)); > > I'm not sure about asserting here in a method called wait until. I would either leave the infinite loop and test will timeout if file is never created, or keep the tries but assert in the callers. Or keep the asser here, but rename the method as something like assertFileIsCreated() IIRC, the issue was that the message while timing out was not that useful. I'll change the name of the function to better indicate its intent. Thanks for the suggestion. Created attachment 438439 [details]
Patch
Committed r282654 (241799@main): <https://commits.webkit.org/241799@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 438439 [details]. |