Bug 91639 – [EFL][WK2] Add unit tests for Ewk_Cookie_Manager

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

We need to add unit tests for Ewk_Cookie_Manager.

[Chris Dumez]

Created attachment 153055 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153055 [details]
Patch

Attachment 153055 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13277603

[Chris Dumez]

Created attachment 153077 [details]
Patch

Reuploading the same patch again to make ewk-efl happy, now that the dependency landed.

[Gyuyoung Kim]

Comment on attachment 153077 [details]
Patch

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

> Source/WebKit2/PlatformEfl.cmake:7
> +    ${CAIRO_LIBRARIES}

This one should be changed CAIRO_LIBRARIES with CAIRO_LDFLAGS.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:50
> +    gchar* uriString = soup_uri_to_string(uri, FALSE);

Should we use gchar ? I think we avoid to use glib type in EFL port, if possible.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.h:29
> +

Nit: Unneeded line.

[Chris Dumez]

Created attachment 153188 [details]
Patch

Take Gyuyoung's feedback into consideration.

[Thiago Marcos P. Santos]

Comment on attachment 153188 [details]
Patch

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

One final consideration: I can see that you are using a lot of ASSERT_EQ. They are all "fatal" exceptions and will stop that particular test block in case it is hit. If the the tests on the block don't depend on each other, I recommend using EXPECT_* instead. If you did it on purpose, just ignore this comment.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:51
> +    CString ret = uriString;

WebKit doesn't like variables named "ret". You should use something more descriptive.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:68
> +    ASSERT(!error);

Use gtest assertions (i.e. ASSERT_TRUE/ASSERT_FALSE) on the test. Otherwise you wont get this error when testing a release build.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:84
> +    ASSERT(!error);

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:120
> +    g_type_init();
> +    ecore_main_loop_glib_integrate();

This would be a nice addition for the fixture SetUp().

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:126
> +    ASSERT(cookieManager);

Use gtest fatal assertion.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:218
> +    ASSERT(cookieManager);

Use gtest.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_cookie_manager.cpp:224
> +    ewk_cookie_manager_persistent_storage_set(cookieManager, TMP_COOKIE_FILE_TXT, EWK_COOKIE_PERSISTENT_STORAGE_TEXT);

I'm concerned here because we always use the same file name for the cookie storage. This has the following implications: your test might be tainted by a previous run that crashed before the unlink and it will not work when running the same test parallel (i.e. you have 2 build bots on the same machine). A cheap solution for this is to use tempnam() at this test case.

Ideally the TestEvironment should provide a temporary directory and cleanup this directory at its teardown. IMO it can be done in another bug.

[Chris Dumez]

Comment on attachment 153188 [details]
Patch

Clearing flags until I update the patch.

[Chris Dumez]

Well, the good news is that this patch revealed a regression in libsoup v2.39.3 (bumped yesterday). Thankfully, the issue has been resolved with v2.39.4.1 (confirmed with the unit tests) so I'm bumping libsoup again in bug 91741).

[Chris Dumez]

Created attachment 153258 [details]
Patch

Take Thiago's feedback into consideration.

[Thiago Marcos P. Santos]

Comment on attachment 153258 [details]
Patch

LGTM. Thanks!

[Chris Dumez]

Created attachment 153436 [details]
Patch

Rebase on master.

[Gyuyoung Kim]

Comment on attachment 153436 [details]
Patch

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

LGTM except for trivial nit.

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:50
> +    char* uri = soup_uri_to_string(soupURI, FALSE);

nit(?) : is FALSE gtk style ? EFL port have used false so far except for public APIs.

[Chris Dumez]

(In reply to comment #12)
> (From update of attachment 153436 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153436&action=review
> 
> LGTM except for trivial nit.
> 
> > Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:50
> > +    char* uri = soup_uri_to_string(soupURI, FALSE);
> 
> nit(?) : is FALSE gtk style ? EFL port have used false so far except for public APIs.

Yes, I missed that one. Will fix it.

[Chris Dumez]

Created attachment 153443 [details]
Patch

Remove some glib'ism.

[Gyuyoung Kim]

Comment on attachment 153443 [details]
Patch

LGTM, thanks.

[Thiago Marcos P. Santos]

Comment on attachment 153443 [details]
Patch

LGTM and the mini webserver is a great addition to the test suite. Thanks.

[Chris Dumez]

All the dependencies have landed, this patch is ready to land.

[Antonio Gomes]

Comment on attachment 153443 [details]
Patch

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

> Source/WebKit2/ChangeLog:12
> +        to indicate that only cookies for the main page are accepted by default.

main frame?

> Source/WebKit2/UIProcess/API/efl/tests/UnitTestUtils/EWK2UnitTestServer.cpp:58
> +}
> +
> +

nit: extra line

[Chris Dumez]

Created attachment 153829 [details]
Patch for landing

Fix nits spotted by Antonio.

[WebKit Review Bot]

Comment on attachment 153829 [details]
Patch for landing

Clearing flags on attachment: 153829

Committed r123372: <http://trac.webkit.org/changeset/123372>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.