test(search): Add test case for baloo parsing model
ClosedPublic

Authored by iasensio on Nov 11 2019, 1:12 PM.

Details

Summary

Adds a new test unit for the model which parses baloo search URLs
14/19 tests are set to XFAIL on current implementation, as they will be fixed in a final revision.

Supersedes D25135.

Depends on: D25257

Test Plan

bin/dolphinquerytest

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
iasensio created this revision.Nov 11 2019, 1:12 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptNov 11 2019, 1:12 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
iasensio requested review of this revision.Nov 11 2019, 1:12 PM
iasensio edited the summary of this revision. (Show Details)Nov 11 2019, 1:45 PM
elvisangelaccio requested changes to this revision.Mon, Nov 11, 9:54 PM
elvisangelaccio added inline comments.
src/tests/dolphinsearchboxtest.cpp
57 ↗(On Diff #69584)

Please try to use QVERIFY(QTest::qWaitForWindowExposed(m_searchBox)); in the actual test instead.

132 ↗(On Diff #69584)

Coding style: we never use a leading underscore in function names.

Missing pass-by-reference for searchString.

139–140 ↗(On Diff #69584)

One-liner:

const QString queryString = QString::fromUtf8(doc.toJson(QJsonDocument::Compact));
163 ↗(On Diff #69584)

const

192 ↗(On Diff #69584)

Coding style: missing space before/after =

This revision now requires changes to proceed.Mon, Nov 11, 9:54 PM
bruns requested changes to this revision.Mon, Nov 11, 10:15 PM

Please move everything added to a new file, this is independent from the searchbox itself.

src/tests/dolphinsearchboxtest.cpp
90 ↗(On Diff #69584)

The parsing is independent from the config.

170 ↗(On Diff #69584)

Just add a column "failureReason" to the data, and do

if (!failureReason.empty()) {
    QEXPECT_FAIL("", qPrintable(failureReason), Continue);
}

Keeps test and result together.

193 ↗(On Diff #69584)

the second .trimmed() should better be not necessary.

bruns added inline comments.Mon, Nov 11, 10:24 PM
src/tests/dolphinsearchboxtest.cpp
57 ↗(On Diff #69584)

And unrelated, separate review please. Also likely unnecessary, as testTextClearing already has a show().

98 ↗(On Diff #69584)

use a fixed datetime here.

iasensio updated this revision to Diff 69661.Wed, Nov 13, 12:26 AM
iasensio marked 8 inline comments as done.
  • Move to a different test unit
  • Address comments

Most of the boilerplate went away after separating the test and not needing DolphinSearchBox anymore
I also removed the #ifdef HAVE_BALOO guards since the test is within if (KF5Baloo_FOUND) in cmake.

iasensio updated this revision to Diff 69662.Wed, Nov 13, 12:27 AM
  • Proper arc diff
iasensio edited the summary of this revision. (Show Details)Wed, Nov 13, 12:28 AM
iasensio edited the test plan for this revision. (Show Details)
iasensio updated this revision to Diff 69663.Wed, Nov 13, 12:30 AM
  • Fix comment in CMakeLists
iasensio updated this revision to Diff 69664.Wed, Nov 13, 12:31 AM
  • Fix comment in CMakeLists.txt
iasensio updated this revision to Diff 69665.Wed, Nov 13, 12:45 AM
  • Do not trim search terms
iasensio marked an inline comment as done.Wed, Nov 13, 12:46 AM
iasensio added inline comments.
src/tests/dolphinsearchboxtest.cpp
170 ↗(On Diff #69584)

Since this is temporary until the fix commit, I think this would keep the revisions simpler

193 ↗(On Diff #69584)

I removed both since this is a remnant from my first attempt comparing searchStrings directly.
Now every searchTerm should be already trimmed by the parsing.

src/tests/dolphinquerytest.cpp
43–49

Why empty? We should just remove them if we don't need any custom init/cleanup logic.

100

const

124

just query as variable name.

iasensio updated this revision to Diff 69709.Wed, Nov 13, 9:35 PM
iasensio marked 3 inline comments as done.

Address comments

iasensio marked an inline comment as done.Wed, Nov 13, 9:36 PM
elvisangelaccio accepted this revision.Wed, Nov 13, 9:44 PM

Please wait for @bruns approval too.

bruns accepted this revision.Thu, Nov 14, 12:15 AM
This revision is now accepted and ready to land.Thu, Nov 14, 12:15 AM
This revision was automatically updated to reflect the committed changes.