Add test to check that cookies are imported from KCookieServer at creation

Authored by stefanocrocco on Oct 24 2018, 5:23 PM.



In theory it should be possible to test this by connecting to
QWebEngineCookieStore::cookieAdded signal. However, in tests this signal
is not emitted (or, at least, it's emitted after the test has run). To
work around this issue, if the BUILD_TESTING cmake variable is true, a new
instance variable is created for WebEnginePartCookieJar and cookies added on
creation are stored there. The contents of this variable are then checked by

Currently, the tests fail because there's a small difference (of the
order of milliseconds, it seems) on the expiration dates. These tests
are currently marked as expected failures

Test Plan

run autotests

Diff Detail

R226 Konqueror
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
stefanocrocco requested review of this revision.Oct 24 2018, 5:23 PM
stefanocrocco created this revision.

Fix failing tests

Tests were failing because the expiration dates used in the tests had an
accuracy of milliseconds, while those returned by KCookieServer have an accuracy
of the second.

Also fixed handling of cookies with empty domains in tests

dfaure requested changes to this revision.Nov 4 2018, 9:42 AM
dfaure added inline comments.

Isn't this always true in this directory?




Const ref missing for c1 and c2






Move to cpp file, no need to be here in header.


Should this be cleared at some point? For users of a debug build...

This revision now requires changes to proceed.Nov 4 2018, 9:42 AM
stefanocrocco added inline comments.Nov 4 2018, 10:50 AM

Unfortunately, it's quite difficult to find out when to do so. Of course it can't be done in the constructor because it is needed by the test method after the constructor has finished. Maybe we could use a timer, but we can't know how much time the test needs. Two options come to my mind:

  • at the end of the constructor, if BUILD_TESTING is defined, look at the application name: if it doesn't contain the word "test" clear the list, otherwise leave it as it is
  • instead of using BUILD_TESTING to decide whether to define the m_testCookies variable, use a more specialized CMake variable

I don't like magic based on process name. Just call a setter from the tests that sets a bool to enable the logging into m_testCookies? Inside the ifdef if you want, although the performance impact (of checking a bool) is negligible, so actually I'd remove the define/ifdef... (but I'm also OK with it if you want to keep it).

Thinking again about it, it should be easier than that: m_testCookies can be cleared from WebEnginePartCookieJar::addCookie, without even needing a conditional. addCookie is called in response to the QWebEngineCookieStore::cookieAdded signal which in tests, for reasons I haven't exactly figured out (but I believe are related to QWebEngineCookieStore::setCookie being asynchronous) is never emitted, so m_testCookies is never cleared in tests but it's cleared when using Konqueror as soon as a new cookie is received.

stefanocrocco updated this revision to Diff 45264.EditedNov 11 2018, 9:01 AM
Made requested changes

Move currentDateTime to webenginepartcookiejar_test.cpp as requested

dfaure accepted this revision.Feb 3 2019, 12:41 PM
dfaure added inline comments.

(nonsense, never mind, I was confusing cmake vars and compiler defines)

This revision is now accepted and ready to land.Feb 3 2019, 12:41 PM
This revision was automatically updated to reflect the committed changes.