Summary: | [GTK] WebKit2 test TestWebKitFaviconDatabase times out with recent glib | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||
Component: | Tools / Tests | Assignee: | 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
Carlos Garcia Campos
2013-03-05 06:43:38 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: Created attachment 191488 [details]
Patch
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 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.
(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. (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. (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 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. (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. (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. (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? (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. (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? (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. (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. (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. (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? 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); (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 (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 *** Bug 117690 has been marked as a duplicate of this bug. *** 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 (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. (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? (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. (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. Created attachment 206116 [details]
Patch
Converted all tests in a single one.
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. 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. (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 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 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. (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. Created attachment 206128 [details]
Patch
Changed according to Carlos' comments.
Comment on attachment 206128 [details] Patch Clearing flags on attachment: 206128 Committed r152412: <http://trac.webkit.org/changeset/152412> All reviewed patches have been landed. Closing bug. |