Bug 224175 - REGRESSION(r275267) [GLIB] API test /webkit/WebKitWebsiteData/configuration is failing
Summary: REGRESSION(r275267) [GLIB] API test /webkit/WebKitWebsiteData/configuration i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Lauro Moura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-04 20:33 PDT by Lauro Moura
Modified: 2021-12-19 20:13 PST (History)
3 users (show)

See Also:


Attachments
Patch (12.47 KB, patch)
2021-09-12 20:18 PDT, Lauro Moura
no flags Details | Formatted Diff | Diff
Patch (12.13 KB, patch)
2021-09-16 21:01 PDT, Lauro Moura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].