Fix code coverage test

[JoshuaSBrown]

PR Summary

This fixes the problem raised by @pgrete where code coverage tests were being run when the CODE COVERAGE flag was disabled.

If you follow the commits, there are two solutions, the first makes use of cmake config variables that are inserted into the .cpp files. I am not a fan of this solution because it feel clunky, and it means treating all the unit tests files as config files.

The second solution is much more elegant, but requires bumping the cmake version to 3.12. This one does not require changes to any of the cpp source files. Instead it parses the tests after Catch2 has created them and appends a coverage label to any tests that does not contain the performance label.

#254

PR Checklist

• Code passes cpplint
• Code is formatted

[pgrete]

Did you confirm that this fully addresses the issue?
I'm still getting that label (coverage) based split between tests.

[codecov]

Codecov Report

Merging #262 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #262   +/-   ##
========================================
  Coverage    51.92%   51.92%           
========================================
  Files           95       95           
  Lines         9904     9904           
========================================
  Hits          5143     5143           
  Misses        4761     4761           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5979a09...7398a76. Read the comment docs.

[JoshuaSBrown]

Did you confirm that this fully addresses the issue?
I'm still getting that label (coverage) based split between tests.

Without the coverage flag I get this:

[ppc64le] [joshuasbrown@cn2023 build]$ ctest -R coverage
Test project /home/joshuasbrown/Software/parthenon_x86_64/parthenon/build
No tests were found!!!

[JoshuaSBrown]

Ok, I see what you are getting at, it will be hard to remove the labels from the unit tests because they are added in the actual cpp files not in cmake. The way Catch2 is setup they the labels are parsed in through the TEST_CASE macro in the cpp files, I can see if the coverage label can instead be added through cmake.

[JoshuaSBrown]

This works, I had hoped there was a better way to do this, the problem is that the only way to add labels that I can see to catch2 tests is within the .cpp files, I had hoped to use compile definitions but because the catch2 macros that resolve the labels are also handled by the preprocessor the labels are not correctly resolved by doing this. I'm still open to better ways of doing this.

[Yurlungur]

I don't love this solution, but I don't see a better one. If it fixes things, I say we merge it in and don't nitpick. Thanks for digging in and finding a fix.

One potential solution, which I shudder to consider, would be using a text processing like sed or awk. I do not advocate it though---I think the cmake solution is better for us.

[Yurlungur]

@JoshuaSBrown looks like formatting check is failing. A make format or a call to par-hermes will probably fix it.

[JoshuaSBrown]

@par-hermes format

[JoshuaSBrown]

I don't love this solution, but I don't see a better one. If it fixes things, I say we merge it in and don't nitpick. Thanks for digging in and finding a fix.

One potential solution, which I shudder to consider, would be using a text processing like sed or awk. I do not advocate it though---I think the cmake solution is better for us.

I have now improved it, I like this implementation much more too.

[Yurlungur]

I have now improved it, I like this implementation much more too.

Aha! Yes, this is a massive improvement. Does any documentation need to change? On a first pass, I think no.

[JoshuaSBrown]

I have now improved it, I like this implementation much more too.

Aha! Yes, this is a massive improvement. Does any documentation need to change? On a first pass, I think no.

Thanks! And yes, I need to update some documentation, I want to make sure no one has issues with bumping the cmake version though before proceeding. If the Athena++ guys are ok with this the cmake version used in the ci will also need to be updated.

[pgrete]

I agree. This approach looks way cleaner (and a first step towards cleaning up the testing infrastructure).
Feel free to bump the cmake version in both CI files. If I didn't miss anything using 3.12.4 should work in both cases out of the box.

[JoshuaSBrown]

@AndrewGaspar Do I have your approval to merge this?