Differential D15381

Add basic tests for WebEnginePartCookieJar
ClosedPublic
Actions

Authored by stefanocrocco on Sep 9 2018, 4:23 PM.

Details

Reviewers
dfaure
Commits
R226:fac5b7802d80: Add basic tests for WebEnginePartCookieJar
Summary

Currently, this only tests that cookies are correctly added to
KCookieServer in response to the QWebEngineCookieStore::cookieAdded
signal.

There are some limitations:

  • ideally, to test the correct behaviour of WebEnginePartCookieJar::cookieAdded, one should call QWebEngineCookieStore::setCookie, so that QWebEngineCookieStore processes the cookie and emits the cookieAdded signal as it would in the real application. Unfortunately, in the tests, calling setCookie doesn't trigger the cookieAdded signal. Because of this, I manually emitted the signal. However, this bypasses any processing by the cookie store (such as calling QNetworkCookie::normalize, which I do manually);
  • I don't know how to start a separate instance of the Cookie Server service, so the tests use the default one. This has two drawbacks: it's impossible to start with a clean state and the tests pollute the user's configuration (this second issue could be avoided by adding clean up code to each test)

Diff Detail

Repository
R226 Konqueror
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
stefanocrocco requested review of this revision.Sep 9 2018, 4:23 PM
stefanocrocco created this revision.
stefanocrocco updated this revision to Diff 41399.Sep 11 2018, 12:39 PM

Attempt to avoid polluting the user's cookies. To achieve this, all cookies
created by the tests have a name starting with webenginepartcookiejartest. In
the cleanup phase, all the cookies in the KCookieServer which start with this
name are deleted.

dfaure accepted this revision.Sep 16 2018, 9:19 AM

I would sprinkle some const on local variables, but looks good otherwise.

Note that bool secure (in CookieData) is always set to true... Missing tests for false?

This revision is now accepted and ready to land.Sep 16 2018, 9:19 AM
stefanocrocco updated this revision to Diff 41768.Sep 16 2018, 7:02 PM
Made local variables const whenever possible and added a test for cookies without
the secure attribute.

Also changed the name of cookies so they're different for the different rows and
corrected the way findCookies is called by passing only the origin host and not
the full URL, because using the full URL fails in case of cookies without a
domain.
dfaure added inline comments.Sep 17 2018, 7:00 AM
webenginepart/autotests/webenginepartcookiejar_test.cpp
127

nitpick: qPrintable() instead of .toLatin1()

(to fix utf8, and to avoid an implicit cast from QByteArray to const char*)

130

add space between type and var

135

const

165

space after if

171

const

172

space before '(', not after

179

missing a space after for, before '+', and before '{' ;)

That is, if you agree with using the Qt/KF5 coding style.

In that case you can run kde/kdesdk/kde-dev-scripts/uncrustify-kf5 to automate these changes, btw.

180

s/){/) {/ globally

198

space before {

(and after foreach -- you could use a range-for, btw)

stefanocrocco added inline comments.Sep 17 2018, 12:20 PM
webenginepart/autotests/webenginepartcookiejar_test.cpp
179

Thanks for the hint

stefanocrocco updated this revision to Diff 41820.Sep 17 2018, 12:21 PM

Made some other variables const and fixed style

Closed by commit R226:fac5b7802d80: Add basic tests for WebEnginePartCookieJar (authored by stefanocrocco). · Explain WhySep 23 2018, 9:57 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 42172

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/webenginepartcookiejar.h

Loading...