advancedqueryparsertest: Add more tests
ClosedPublic

Authored by michaelh on Apr 3 2018, 6:27 PM.

Diff Detail

Repository
R293 Baloo
Branch
advancedqueryparsertest (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
michaelh created this revision.Apr 3 2018, 6:27 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptApr 3 2018, 6:27 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
michaelh requested review of this revision.Apr 3 2018, 6:27 PM

@bruns: This is mainly to confirm I understood D11826 correctly. The tests pass with or without your patch.

bruns added inline comments.Apr 4 2018, 9:02 PM
autotests/unit/lib/advancedqueryparsertest.cpp
328

better use a different name, phrase has a special meaning, i.e. Baloo:EngineQuery:Phrase

bruns added a comment.Apr 4 2018, 9:38 PM

Although this is probably a useful addition (haven't checked which cases the existing unit test already cover), it does not cover the changes in D11826.

The code in https://cgit.kde.org/baloo.git/tree/src/lib/advancedqueryparser.cpp#n100 replicates the new logic in the Term::Term(Term&, Operation&, Term&) constructor (merging of compatible terms), and, after the changes in D11826, could be replaced with just

static void addTermToStack(QStack<Term>& stack, const Term& termInConstruction, Term::Operation op)
{
    auto top = stack.pop();
    stack.push(Term(top, op, termInConstruction));
}

operator&& is e.g. used in
https://cgit.kde.org/baloo.git/tree/src/lib/query.cpp#n189

To test operator&&, the following should be true:

Term{Term:And, {Term{"", "a"}, Term{"", "b"}, Term{"", "c"}, Term{"", "d"} }
==
Term{"", "a"} && Term{"", "b"} && Term{"", "c"} && Term{"", "d"}
michaelh updated this revision to Diff 31373.Apr 5 2018, 11:11 AM
  • Rename testOptimizedLogic
  • Add a different testOptimizedLogic
michaelh retitled this revision from advancedqueryparsertest: Add optimization test to advancedqueryparsertest: Add more test.Apr 5 2018, 11:12 AM
michaelh updated this revision to Diff 31375.Apr 5 2018, 11:27 AM
  • Remove duplicated data
michaelh updated this revision to Diff 31381.Apr 5 2018, 12:00 PM
  • Add (a AND b) AND (c OR d) can be merged as (a AND b AND (c OR D)

With this D11826 makes a difference. In terms of comprehension I'm also almost there.
testNestedParentheses still has some value as it tests nested parentheses more extensively than the one already there.

Unless you have more comments, I suggest to land these tests before D11826, rebase D11826 and change the respective failmessages to an empty string.

michaelh updated this revision to Diff 31390.Apr 5 2018, 12:21 PM
  • Remove duplicated data again
michaelh added inline comments.Apr 5 2018, 12:23 PM
autotests/unit/lib/advancedqueryparsertest.cpp
255

What about this one? Drop it?

bruns added inline comments.Apr 5 2018, 8:31 PM
autotests/unit/lib/advancedqueryparsertest.cpp
255

I think it should stay as it exercises the stack in the query parser

Pseudocode:

s[0] = a, AND
s[1] = b, AND
t = construct (c AND d)
t = Term( s[1].term, t, s[1].op)  
t = Term( s[0].term, t, s[0].op)
290

Use "a && b && c && d", makes grepping in case of failure easier?

305

dito, "(a && b) && (c || d)"

bruns added inline comments.Apr 5 2018, 8:48 PM
autotests/unit/lib/advancedqueryparsertest.cpp
235

Term{QString(), QStringLiteral("a"), Term::Contains},

see http://blog.qt.io/blog/2014/06/13/qt-weekly-13-qstringliteral/

michaelh updated this revision to Diff 31428.Apr 5 2018, 9:25 PM
  • Apply suggested changes
michaelh marked 5 inline comments as done.Apr 5 2018, 9:26 PM
michaelh retitled this revision from advancedqueryparsertest: Add more test to advancedqueryparsertest: Add more tests.Apr 5 2018, 11:22 PM
bruns accepted this revision.Apr 6 2018, 6:09 PM
bruns added inline comments.
autotests/unit/lib/advancedqueryparsertest.cpp
229

remove

231

Remove the searchInput temporary and ...

233

... use the QStringLiteral here

253

Just << QString()

264

Adds no information, just use QString() here (and twice below)

This revision is now accepted and ready to land.Apr 6 2018, 6:09 PM
bruns requested changes to this revision.Apr 6 2018, 6:10 PM
This revision now requires changes to proceed.Apr 6 2018, 6:10 PM
michaelh updated this revision to Diff 31512.Apr 6 2018, 7:13 PM
  • Apply suggested changes
michaelh marked 4 inline comments as done.Apr 6 2018, 7:15 PM
michaelh added inline comments.
autotests/unit/lib/advancedqueryparsertest.cpp
264

This test fails. "no optimization" serves as a flag, that it's expected to fail and as message why. See lines 217-219. That's why I thought of dropping it, because D11826 does not make it pass.

If we drop this test the failmessage column can be removed.

bruns added a comment.Apr 6 2018, 7:48 PM

Can you add the output of the failing tests here verbatim?

autotests/unit/lib/advancedqueryparsertest.cpp
264

Ah, this makes it somewhat complicated ...

So the parsed query is semantically correct, only the term are not merged?
If yes, the best way here is to probably keep this row as is, but also add a check testing for the unoptimized term. The second check should then carry a comment mentioning it is only a fallback.

Anyway, please make the message somewhat more meaningful, e.g. "Fails to optimize for unknown reason, but output is semantically correct".

michaelh updated this revision to Diff 31518.Apr 6 2018, 8:10 PM
  • Add 'fallback' test
$ ctest -R advanced -V 

michaelh updated this revision to Diff 31519.Apr 6 2018, 8:27 PM
  • Distinguish tests
bruns accepted this revision.Apr 6 2018, 11:21 PM

Ok, this is ready now. Thanks!

This revision is now accepted and ready to land.Apr 6 2018, 11:21 PM
This revision was automatically updated to reflect the committed changes.
michaelh marked 3 inline comments as done.