Bug 224175

Summary: REGRESSION(r275267) [GLIB] API test /webkit/WebKitWebsiteData/configuration is failing
Product: WebKit Reporter: Lauro Moura <lmoura>
Component: WebKitGTKAssignee: 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 Flags
Patch
none
Patch none

Description Lauro Moura 2021-04-04 20:33:43 PDT
/WPE/TestWebsiteData /webkit/WebKitWebsiteData/configuration

To run the single test case in GTK-Release:

./Tools/Scripts/run-gtk-tests --display-server=xvfb --release WebKitBuild/GTK/Release/bin/TestWebKitAPI/WebKit2Gtk/TestWebsiteData -p /webkit/WebKitWebsiteData/configuration

It was kinda flaky before on the bots, but after r275267 it is failing consistently, both on the bots and locally.

Checking the test, the fetch instruction at line 200 returns a WebKitWebsiteData structure with type WEBKIT_WEBSITE_DATA_DOM_CACHE set. Maybe it is not being cleared correctly?
Comment 1 Lauro Moura 2021-04-05 16:50:12 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
Comment 2 Lauro Moura 2021-09-12 20:18:42 PDT
Created attachment 438007 [details]
Patch
Comment 3 Carlos Garcia Campos 2021-09-12 23:14:25 PDT
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 4 Lauro Moura 2021-09-16 20:57:29 PDT
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.
Comment 5 Lauro Moura 2021-09-16 21:01:04 PDT
Created attachment 438439 [details]
Patch
Comment 6 EWS 2021-09-17 04:03:00 PDT
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].