D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Michael Heidelbach
This revision was automatically updated to reflect the committed changes.
michaelh marked 3 inline comments as done.
Closed by commit R293:1ea0b94497c3: advancedqueryparsertest: Add more tests 
(authored by michaelh).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11907?vs=31519=31560

REVISION DETAIL
  https://phabricator.kde.org/D11907

AFFECTED FILES
  autotests/unit/lib/advancedqueryparsertest.cpp

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Stefan Brüns
bruns accepted this revision.
bruns added a comment.
This revision is now accepted and ready to land.


  Ok, this is ready now. Thanks!

REPOSITORY
  R293 Baloo

BRANCH
  advancedqueryparsertest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11907

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Michael Heidelbach
michaelh updated this revision to Diff 31519.
michaelh added a comment.


  - Distinguish tests

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11907?vs=31518=31519

BRANCH
  advancedqueryparsertest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11907

AFFECTED FILES
  autotests/unit/lib/advancedqueryparsertest.cpp

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Michael Heidelbach
michaelh added a comment.


$ ctest -R advanced -V 
  
  F5795958: AdvancedQueryParserTest 

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D11907

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Michael Heidelbach
michaelh updated this revision to Diff 31518.
michaelh added a comment.


  - Add 'fallback' test

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11907?vs=31512=31518

BRANCH
  advancedqueryparsertest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11907

AFFECTED FILES
  autotests/unit/lib/advancedqueryparsertest.cpp

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Stefan Brüns
bruns added a comment.


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

INLINE COMMENTS

> michaelh wrote in 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.

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

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D11907

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Michael Heidelbach
michaelh marked 4 inline comments as done.
michaelh added inline comments.

INLINE COMMENTS

> bruns wrote in advancedqueryparsertest.cpp:264
> Adds no information, just use QString() here (and twice below)

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.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D11907

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Michael Heidelbach
michaelh updated this revision to Diff 31512.
michaelh added a comment.


  - Apply suggested changes

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11907?vs=31428=31512

BRANCH
  advancedqueryparsertest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11907

AFFECTED FILES
  autotests/unit/lib/advancedqueryparsertest.cpp

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Stefan Brüns
bruns requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D11907

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-06 Thread Stefan Brüns
bruns accepted this revision.
bruns added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> advancedqueryparsertest.cpp:229
> +QTest::addColumn("failmessage");
> +const QString nofail;
> +QString searchInput;

remove

> advancedqueryparsertest.cpp:231
> +QString searchInput;
> +searchInput = QStringLiteral("a AND b AND c AND d");
> +QTest::newRow(qPrintable(searchInput))

Remove the searchInput temporary and ...

> advancedqueryparsertest.cpp:233
> +QTest::newRow(qPrintable(searchInput))
> +<< searchInput
> +<< Term{Term::And, QList{

... use the QStringLiteral here

> advancedqueryparsertest.cpp:253
> +}}
> +<< nofail
> +;

Just `<< QString()`

> advancedqueryparsertest.cpp:264
> +}}
> +<< QStringLiteral("no optimization")
> +;

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

REPOSITORY
  R293 Baloo

BRANCH
  advancedqueryparsertest (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D11907

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin


D11907: advancedqueryparsertest: Add more tests

2018-04-05 Thread Michael Heidelbach
michaelh retitled this revision from "advancedqueryparsertest: Add more test" 
to "advancedqueryparsertest: Add more tests".

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D11907

To: michaelh, #baloo, bruns
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, alexeymin