Bug 224175 – REGRESSION(r275267) [GLIB] API test /webkit/WebKitWebsiteData/configuration is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Lauro Moura]

/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?

[Lauro Moura]

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

[Lauro Moura]

Created attachment 438007 [details]
Patch

[Carlos Garcia Campos]

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()

[Lauro Moura]

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.

[Lauro Moura]

Created attachment 438439 [details]
Patch

[EWS]

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].