Improve reporting of unmatched filters

[sfranzen]

Description

This PR is for two issues related to filtering tests, reported in issue #1449.

It modifies various existing structures to allow checking whether any filters passed to a test executable did not result in any matching test cases, giving the user better feedback about the input. It also adds small helper classes to aid readability of the code. Additionally, the option -w NoTests is now correctly checked and an exit code 2 returned if there were any unmatched filters.

GitHub Issues

Fixes #1449. Also fixes #1683.

[codecov]

Codecov Report

Merging #1684 into master will increase coverage by 0.26%.
The diff coverage is 91.76%.

@@            Coverage Diff             @@
##           master    #1684      +/-   ##
==========================================
+ Coverage   86.18%   86.44%   +0.26%     
==========================================
  Files         136      136              
  Lines        5180     5266      +86     
==========================================
+ Hits         4464     4552      +88     
+ Misses        716      714       -2

[sfranzen]

The remaining failure is with the AppVeyor coverage build and is caused by OpenCppCoverage marking the exit code as an error where CTest does not (because it has WILL_FAIL TRUE). Is there an elegant way to ignore the exit code in this specific case?

[horenmar]

I'll investigate the problem -- it is my understanding that both OpenCppCoverage and the wrapper we use for CTest should just pass-through the return codes back to CTest, so it should not fail.

[horenmar]

After closer look, the helper executable does not actually pass the return code up the stack... I'll try to fix it tomorrow.

[sfranzen]

In the meantime I've downloaded this coverage tool and played with it, and I could also obtain a coverage report for all tests by running OpenCppCoverage [coverage options] -- ctest [ctest options]. CTest then hosts the individual test commands and completes with return code 0 as long as all test cases return 0 or else have WILL_FAIL TRUE. This seems a lot simpler than the current setup, though I haven't studied it in full detail -- would it be significantly different?

[horenmar]

Hmmm, I'll take a look -- it has honestly never occurred to me to run coverage over the top level ctest process and let it take coverage from the children processes...

[sfranzen]

I found it via this issue while googling about OpenCppCoverage. Oddly enough its documentation does not mention CMake or CTest at all, only the export + merge method that is currently used by Catch2.

[horenmar]

Good news: I fixed the helper to pass through return codes, and will merge it in the evening.

Bad news: I also found out that the coverage merging fails in a way I am unable to reproduce locally, and will have to poke the CI to fix (second one this week, ugh)

What this means is that you should rebase the PR tomorrow/late evening today and I'll start reviewing the changes.

---

I'll also open up an issue to investigate the different method of gathering coverage.

[horenmar]

You will probably need to pass through the path to the test binary from the main script.

[sfranzen]

I think you were right, here's to hoping this is the final change!

[horenmar]

There is 1 more thing that needs fixing and one that I am not quite sure about.

Needs fixing: the filter string in No test cases matched 'asdafs' should roundtrip, so that it can be again used as a filter provided to Catch. Currently it does not:

$  ./projects/SelfTest [asdafs]
Filters: [asdafs]
No test cases matched 'asdafs'
===============================================================================
No tests ran

The part I am not sure about is whether the extended reporting should be provided even if the -w NoTests flag was not specified.

[sfranzen]

This was slightly trickier as I've now had to mess with the test spec parser, but it does the trick and passes the tests on my machine. About the reporting, I would prefer to always leave at least a textual trace, so the user can see where they mistyped or misconfigured test cases.

[horenmar]

Okay, this looks good now.

Would you prefer if I squash-merge it, or if you rebase and cleanup the branch?

[sfranzen]

I'm fine with squashing, it is just improvements/fixes and regression tests that don't warrant multiple commits IMO. Thanks for the careful review!