Details
- Reviewers
bruns - Group Reviewers
Baloo - Commits
- R293:1ea0b94497c3: advancedqueryparsertest: Add more tests
Diff Detail
- Repository
- R293 Baloo
- Branch
- advancedqueryparsertest (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
autotests/unit/lib/advancedqueryparsertest.cpp | ||
---|---|---|
331 | better use a different name, phrase has a special meaning, i.e. Baloo:EngineQuery:Phrase |
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"}
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.
autotests/unit/lib/advancedqueryparsertest.cpp | ||
---|---|---|
255 | What about this one? Drop it? |
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)" |
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/ |
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. |
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? Anyway, please make the message somewhat more meaningful, e.g. "Fails to optimize for unknown reason, but output is semantically correct". |