Bug 111434 – [GTK] WebKit2 test TestWebKitFaviconDatabase times out with recent glib

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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:

[Carlos Garcia Campos]

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:

[Carlos Garcia Campos]

Created attachment 191488 [details]
Patch

[WebKit Review Bot]

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

[Martin Robinson]

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.

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

(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

[Zan Dobersek]

*** Bug 117690 has been marked as a duplicate of this bug. ***

[Zan Dobersek]

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

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

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

[Martin Robinson]

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

[Carlos Garcia Campos]

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

[Xabier Rodríguez Calvar]

Created attachment 206116 [details]
Patch

Converted all tests in a single one.

[Martin Robinson]

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.

[Xabier Rodríguez Calvar]

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.

[Xabier Rodríguez Calvar]

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

[Martin Robinson]

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.

[Carlos Garcia Campos]

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.

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

Created attachment 206128 [details]
Patch

Changed according to Carlos' comments.

[WebKit Commit Bot]

Comment on attachment 206128 [details]
Patch

Clearing flags on attachment: 206128

Committed r152412: <http://trac.webkit.org/changeset/152412>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.