Differential D16407

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

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

Details

Reviewers
dfaure
Commits
R226:4ff9f87b5167: Add test to check that cookies are imported from KCookieServer at creation
Summary

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

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

Repository
R226 Konqueror
Branch
test-cookie-loading-at-startup
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4818
Build 4836: arc lint + arc unit
stefanocrocco requested review of this revision.Oct 24 2018, 5:23 PM
stefanocrocco created this revision.
stefanocrocco updated this revision to Diff 44220.Oct 25 2018, 5:21 PM

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.
webenginepart/autotests/CMakeLists.txt
2

Isn't this always true in this directory?

webenginepart/autotests/webenginepartcookiejar_test.cpp
322

qAsConst

331

Const ref missing for c1 and c2

348

qAsConst

webenginepart/autotests/webenginepartcookiejar_test.h
67

Remove

85

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

webenginepart/src/webenginepartcookiejar.cpp
300

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
webenginepart/src/webenginepartcookiejar.cpp
300

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
dfaure added a comment.Nov 10 2018, 10:28 PM

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

stefanocrocco added a comment.Nov 11 2018, 8:57 AM

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
stefanocrocco updated this revision to Diff 45266.Nov 11 2018, 9:08 AM

Move currentDateTime to webenginepartcookiejar_test.cpp as requested

dfaure accepted this revision.Feb 3 2019, 12:41 PM
dfaure added inline comments.
webenginepart/autotests/CMakeLists.txt
2

(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
Closed by commit R226:4ff9f87b5167: Add test to check that cookies are imported from KCookieServer at creation (authored by stefanocrocco). · Explain WhyFeb 3 2019, 6:13 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 45264

View Options

webenginepart/autotests/CMakeLists.txt

Loading...
View Options

webenginepart/autotests/webenginepartcookiejar_test.h

Loading...
View Options

webenginepart/autotests/webenginepartcookiejar_test.cpp

Loading...
View Options

webenginepart/src/CMakeLists.txt

Loading...
View Options

webenginepart/src/webenginepartcookiejar.h

Loading...
View Options

webenginepart/src/webenginepartcookiejar.cpp

Loading...