Bug 111434

Summary: [GTK] WebKit2 test TestWebKitFaviconDatabase times out with recent glib
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: Tools / TestsAssignee: Xabier Rodríguez Calvar <calvaris>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, commit-queue, gustavo, mrobinson, webkit.review.bot, zan
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Fixing according to Martin's comments
none
Patch none

Description Carlos Garcia Campos 2013-03-05 06:43:38 PST
The problem is that now glib groups the tests by test suite, so they are not executed in the same order when different test suites are used in the same test executable. This is not a problem if tests don't depend on each other, but in the case of the favicon database, the test that deletes the database should always be the last one. 

/webkit2/WebKitFaviconDatabase/not-initialized: OK
/webkit2/WebKitFaviconDatabase/set-directory: OK
/webkit2/WebKitFaviconDatabase/get-favicon: OK
/webkit2/WebKitFaviconDatabase/get-favicon-uri: OK
/webkit2/WebKitFaviconDatabase/clear-database: OK
/webkit2/WebKitFaviconDatabase/WebKitWebView/favicon:
Comment 1 Carlos Garcia Campos 2013-03-05 06:46:54 PST
Sorry, I pasted the wrong example, it was:

/webkit2/WebKitFaviconDatabase/not-initialized: OK
/webkit2/WebKitFaviconDatabase/set-directory: OK
/webkit2/WebKitFaviconDatabase/get-favicon: OK
/webkit2/WebKitFaviconDatabase/get-favicon-uri: OK
/webkit2/WebKitFaviconDatabase/clear-database: OK
/webkit2/WebKitWebView/favicon:
Comment 2 Carlos Garcia Campos 2013-03-05 06:49:18 PST
Created attachment 191488 [details]
Patch
Comment 3 WebKit Review Bot 2013-03-05 06:51:16 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 4 Martin Robinson 2013-03-05 07:30:47 PST
Comment on attachment 191488 [details]
Patch

Hrm. Isn't there a way to rework the tests so that they are order-independent? It seems like a bug that they are.
Comment 5 Martin Robinson 2013-03-05 07:31:04 PST
(In reply to comment #4)
> (From update of attachment 191488 [details])
> Hrm. Isn't there a way to rework the tests so that they are order-independent? It seems like a bug that they are.

Sorry. It seems like a bug that they are not order-independent.
Comment 6 Carlos Garcia Campos 2013-03-05 08:08:36 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 191488 [details] [details])
> > Hrm. Isn't there a way to rework the tests so that they are order-independent? It seems like a bug that they are.
> 
> Sorry. It seems like a bug that they are not order-independent.

In this particular case it's not possible, webkit_web_context_set_favicon_database_directory() can only be called once, and it has to be called first. We could do it in beforeAll() but then, not-initialized test wouldn't work. We are using a temp favison database so that the tests don't affect the user favicon database, the test that clears the database has to run the last one, or we would need to fill it again. I think this is the only case and the patch is simple enough.
Comment 7 Martin Robinson 2013-03-05 08:17:57 PST
(In reply to comment #6)

> In this particular case it's not possible, webkit_web_context_set_favicon_database_directory() can only be called once, and it has to be called first. We could do it in beforeAll() but then, not-initialized test wouldn't work. We are using a temp favison database so that the tests don't affect the user favicon database, the test that clears the database has to run the last one, or we would need to fill it again. I think this is the only case and the patch is simple enough.

Perhaps put that test into an afterAll() section?
Comment 8 Carlos Garcia Campos 2013-03-05 08:30:34 PST
(In reply to comment #7)
> (In reply to comment #6)
> 
> > In this particular case it's not possible, webkit_web_context_set_favicon_database_directory() can only be called once, and it has to be called first. We could do it in beforeAll() but then, not-initialized test wouldn't work. We are using a temp favison database so that the tests don't affect the user favicon database, the test that clears the database has to run the last one, or we would need to fill it again. I think this is the only case and the patch is simple enough.
> 
> Perhaps put that test into an afterAll() section?

In afterAll() get_test_run() has finished, you can't add tests there. And that wouldn't make the other tests cases independent from each other so I don't see why that's better than just using the same test suite name.
Comment 9 Martin Robinson 2013-03-05 08:41:16 PST
(In reply to comment #8)

> In afterAll() get_test_run() has finished, you can't add tests there. And that wouldn't make the other tests cases independent from each other so I don't see why that's better than just using the same test suite name.

Oh, I wasn't suggesting adding a test there, but simply running the assertions.
Comment 10 Martin Robinson 2013-03-05 08:41:33 PST
(In reply to comment #9)

> Oh, I wasn't suggesting adding a test there, but simply running the assertions.

Basically we should have a way to validate cleanup.
Comment 11 Carlos Garcia Campos 2013-03-05 08:55:49 PST
(In reply to comment #9)
> (In reply to comment #8)
> 
> > In afterAll() get_test_run() has finished, you can't add tests there. And that wouldn't make the other tests cases independent from each other so I don't see why that's better than just using the same test suite name.
> 
> Oh, I wasn't suggesting adding a test there, but simply running the assertions.

We are already cleaning up the database there, but there's a test case to test database_clear that doesn't remove the file but just clears the database, we can make it independent, but the other tests would need to make fill the database first, I don't think it's worth it. What's wrong with the simple one line patch?
Comment 12 Martin Robinson 2013-03-05 08:58:57 PST
(In reply to comment #11)

> We are already cleaning up the database there, but there's a test case to test database_clear that doesn't remove the file but just clears the database, we can make it independent, but the other tests would need to make fill the database first, I don't think it's worth it. What's wrong with the simple one line patch?

It's very unusual for unit tests to be order-dependent and interferes with testing strategies that randomize test order.
Comment 13 Carlos Garcia Campos 2013-03-05 09:02:28 PST
(In reply to comment #12)
> (In reply to comment #11)
> 
> > We are already cleaning up the database there, but there's a test case to test database_clear that doesn't remove the file but just clears the database, we can make it independent, but the other tests would need to make fill the database first, I don't think it's worth it. What's wrong with the simple one line patch?
> 
> It's very unusual for unit tests to be order-dependent and interferes with testing strategies that randomize test order.

That's a different issue, although related, can we just fix the tests so that I can make a release tomorrow and then you can fix it to make it order independent if you want?
Comment 14 Martin Robinson 2013-03-05 10:19:37 PST
(In reply to comment #13)
 
> That's a different issue, although related, can we just fix the tests so that I can make a release tomorrow and then you can fix it to make it order independent if you want?


I'm not sure I understand the issue here. webkit_favicon_database_clear just calls removeAllIcons. It doesn't actually delete the database, does it? If so perhaps the API is broken, since the documentation doesn't mention that the favicon database does not work after calling it.
Comment 15 Carlos Garcia Campos 2013-03-05 10:26:04 PST
(In reply to comment #14)
> (In reply to comment #13)
> 
> > That's a different issue, although related, can we just fix the tests so that I can make a release tomorrow and then you can fix it to make it order independent if you want?
> 
> 
> I'm not sure I understand the issue here. webkit_favicon_database_clear just calls removeAllIcons. It doesn't actually delete the database, does it? If so perhaps the API is broken, since the documentation doesn't mention that the favicon database does not work after calling it.

WebView favicon test assumes the icon is already in the database, see the comment in the test:

// The icon is known and hasn't changed in the database, so notify::favicon is emitted                                                                                                    
// but WebKitFaviconDatabase::icon-changed isn't.

Maybe that's wrong and it shouldn't make any assumption, but it's *another* issue.
Comment 16 Martin Robinson 2013-03-05 10:34:40 PST
(In reply to comment #15)

> Maybe that's wrong and it shouldn't make any assumption, but it's *another* issue.

The issue is that the test isn't independent of execution order. The changes in GLib just exposed this.
Comment 17 Carlos Garcia Campos 2013-03-05 10:37:53 PST
(In reply to comment #16)
> (In reply to comment #15)
> 
> > Maybe that's wrong and it shouldn't make any assumption, but it's *another* issue.
> 
> The issue is that the test isn't independent of execution order. The changes in GLib just exposed this.

And I'm fine with it, but why can we land the workaround to make the test pass and then you can make the unit tests independent of execution order?
Comment 18 Martin Robinson 2013-03-05 10:42:22 PST
This will serve to make the test independent of order, right?

diff --git a/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp b/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
index d5f3076..5c24643 100644
--- a/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
+++ b/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
@@ -215,9 +215,6 @@ static void testWebViewFavicon(FaviconDatabaseTest* test, gconstpointer)
     test->loadURI(kServer->getURIForPath("/foo").data());
     test->waitUntilFaviconChanged();
     g_assert(test->m_faviconNotificationReceived);
-    // The icon is known and hasn't changed in the database, so notify::favicon is emitted
-    // but WebKitFaviconDatabase::icon-changed isn't.
-    g_assert(test->m_faviconURI.isNull());
 
     iconFromWebView = webkit_web_view_get_favicon(test->m_webView);
     g_assert(iconFromWebView);
Comment 19 Carlos Garcia Campos 2013-03-05 10:46:23 PST
(In reply to comment #18)
> This will serve to make the test independent of order, right?
> 
> diff --git a/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp b/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
> index d5f3076..5c24643 100644
> --- a/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
> +++ b/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
> @@ -215,9 +215,6 @@ static void testWebViewFavicon(FaviconDatabaseTest* test, gconstpointer)
>      test->loadURI(kServer->getURIForPath("/foo").data());
>      test->waitUntilFaviconChanged();
>      g_assert(test->m_faviconNotificationReceived);
> -    // The icon is known and hasn't changed in the database, so notify::favicon is emitted
> -    // but WebKitFaviconDatabase::icon-changed isn't.
> -    g_assert(test->m_faviconURI.isNull());
> 
>      iconFromWebView = webkit_web_view_get_favicon(test->m_webView);
>      g_assert(iconFromWebView);

I don't think so, not that the test doesn't fail, but times out I guess because favicon changed is not emitted, I don't know
Comment 20 Carlos Garcia Campos 2013-03-05 11:03:00 PST
(In reply to comment #19)
> (In reply to comment #18)
> > This will serve to make the test independent of order, right?
> > 
> > diff --git a/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp b/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
> > index d5f3076..5c24643 100644
> > --- a/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
> > +++ b/Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp
> > @@ -215,9 +215,6 @@ static void testWebViewFavicon(FaviconDatabaseTest* test, gconstpointer)
> >      test->loadURI(kServer->getURIForPath("/foo").data());
> >      test->waitUntilFaviconChanged();
> >      g_assert(test->m_faviconNotificationReceived);
> > -    // The icon is known and hasn't changed in the database, so notify::favicon is emitted
> > -    // but WebKitFaviconDatabase::icon-changed isn't.
> > -    g_assert(test->m_faviconURI.isNull());
> > 
> >      iconFromWebView = webkit_web_view_get_favicon(test->m_webView);
> >      g_assert(iconFromWebView);
> 
> I don't think so, not that the test doesn't fail, but times out I guess because favicon changed is not emitted, I don't know

And that wouldn't make the other tests cases independent, so I still don't understant why we can't simply run the test in the order it was thought to be run and then make all tests independent if you think it's so important
Comment 21 Zan Dobersek 2013-06-17 07:31:22 PDT
*** Bug 117690 has been marked as a duplicate of this bug. ***
Comment 22 Zan Dobersek 2013-06-17 07:32:41 PDT
The unit test was skipped in r151633 because it was timing out. The patch fixes the problem, so the test should be unskipped when the patch lands.
http://trac.webkit.org/changeset/151633
Comment 23 Xabier Rodríguez Calvar 2013-07-02 15:04:47 PDT
(In reply to comment #12)
> It's very unusual for unit tests to be order-dependent and interferes with testing strategies that randomize test order.
Comment 24 Xabier Rodríguez Calvar 2013-07-02 15:09:46 PDT
(In reply to comment #12)
> It's very unusual for unit tests to be order-dependent and interferes with testing strategies that randomize test order.

As far as I could see all almost every test depends on the previous to function. It's highly difficult to create a proper setup an tear down for all tests so we can do two things to make them independent from each other:

* Creating one big test with the whole functionatity
* Replicating in the every test what is needed to get it working, what would create a cascade of duplicated code.

What do you think it is best?
Comment 25 Martin Robinson 2013-07-02 17:09:24 PDT
(In reply to comment #24)

> * Creating one big test with the whole functionatity

If the tests all really do depend on each other, then this is a simple fix that still allows for running other tests in a random order. I prefer this. Ideally we'd fix the tests, but it appears to be tricky and not worth the time.
Comment 26 Carlos Garcia Campos 2013-07-03 00:07:39 PDT
(In reply to comment #24)
> (In reply to comment #12)
> > It's very unusual for unit tests to be order-dependent and interferes with testing strategies that randomize test order.
> 
> As far as I could see all almost every test depends on the previous to function. It's highly difficult to create a proper setup an tear down for all tests so we can do two things to make them independent from each other:
> 
> * Creating one big test with the whole functionatity
> * Replicating in the every test what is needed to get it working, what would create a cascade of duplicated code.
> 
> What do you think it is best?

Let's move everything into a single test case, then.
Comment 27 Xabier Rodríguez Calvar 2013-07-04 17:53:27 PDT
Created attachment 206116 [details]
Patch

Converted all tests in a single one.
Comment 28 Martin Robinson 2013-07-04 17:57:55 PDT
Comment on attachment 206116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206116&action=review

Looks good in principle to me, but perhaps Carlos has further comments.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:156
> +    GOwnPtr<char> iconURI(webkit_favicon_database_get_favicon_uri(database, kServer->getURIForPath("/foo").data()));

I'm not sure exactly why this change is necessary.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:220
> +    test->m_faviconURI = static_cast<char*>(0);

Do you really need to cast 0 to a pointer?

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:242
> +    // This tests depend on this order to run properly so we declare
> +    // them in a single one
> +    testNotInitialized(test, 0);

English nit: "This tests" -> "These tests" It would probably be good to include a link to this bug.
Comment 29 Xabier Rodríguez Calvar 2013-07-04 19:33:37 PDT
Created attachment 206119 [details]
Fixing according to Martin's comments

(In reply to comment #28)
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:156
> > +    GOwnPtr<char> iconURI(webkit_favicon_database_get_favicon_uri(database, kServer->getURIForPath("/foo").data()));
> 
> I'm not sure exactly why this change is necessary.

The problem with this is that the clear database test was phony because it is not actually testing for an already existing icon so it could never find it.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:220
> > +    test->m_faviconURI = static_cast<char*>(0);
> 
> Do you really need to cast 0 to a pointer?

Changed.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:242
> > +    // This tests depend on this order to run properly so we declare
> > +    // them in a single one
> > +    testNotInitialized(test, 0);
> 
> English nit: "This tests" -> "These tests" It would probably be good to include a link to this bug.

Changed.

I also removed the code to slice the testGetFavicon in three as this could be a subject of improving the tests to make them actually independent from each other and not needing to run them sorted.
Comment 30 Xabier Rodríguez Calvar 2013-07-04 19:35:21 PDT
(In reply to comment #29)
> > > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:156
> > > +    GOwnPtr<char> iconURI(webkit_favicon_database_get_favicon_uri(database, kServer->getURIForPath("/foo").data()));
> > 
> > I'm not sure exactly why this change is necessary.
> 
> The problem with this is that the clear database test was phony because it is not actually testing for an already existing icon so it could never find it.

I forgot to say that I removed this part of the code and will fix it in another bug as agreed with Martin.
Comment 31 Martin Robinson 2013-07-04 19:36:11 PDT
Comment on attachment 206119 [details]
Fixing according to Martin's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=206119&action=review

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:234
> +    // These tests depend on this order to run properly so we declare
> +    // them in a single one. See
> +    // https://bugs.webkit.org/show_bug.cgi?id=111434.

Just a note for the future: comments in WebKit can be long lines (up to and past 120 characters), so feel free not to break them into more lines.
Comment 32 Carlos Garcia Campos 2013-07-04 22:45:15 PDT
Comment on attachment 206119 [details]
Fixing according to Martin's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=206119&action=review

hehe, you had to move everything into a single test in the end :-) Thanks for fixing it!

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:240
> +    testNotInitialized(test, 0);
> +    testSetDirectory(test, 0);
> +    testGetFavicon(test, 0);
> +    testGetFaviconURI(test, 0);
> +    testWebViewFavicon(test, 0);
> +    testClearDatabase(test, 0);

These methods are no longer added in a Foo::add method, so it doesn't make sense to pass an unused NULL pointer, simply remove it.
Comment 33 Xabier Rodríguez Calvar 2013-07-04 23:54:57 PDT
(In reply to comment #32)
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:240
> > +    testNotInitialized(test, 0);
> > +    testSetDirectory(test, 0);
> > +    testGetFavicon(test, 0);
> > +    testGetFaviconURI(test, 0);
> > +    testWebViewFavicon(test, 0);
> > +    testClearDatabase(test, 0);
> 
> These methods are no longer added in a Foo::add method, so it doesn't make sense to pass an unused NULL pointer, simply remove it.

I left it just in case somebody wanted to split them again, but I can remove it, no problem.
Comment 34 Xabier Rodríguez Calvar 2013-07-05 02:11:56 PDT
Created attachment 206128 [details]
Patch

Changed according to Carlos' comments.
Comment 35 WebKit Commit Bot 2013-07-05 03:14:34 PDT
Comment on attachment 206128 [details]
Patch

Clearing flags on attachment: 206128

Committed r152412: <http://trac.webkit.org/changeset/152412>
Comment 36 WebKit Commit Bot 2013-07-05 03:14:39 PDT
All reviewed patches have been landed.  Closing bug.