Add STATIC_CHECK and STATIC_CHECK_FALSE

[Morwenn]

Description

This PR adds two new macros to Catch2: STATIC_CHECK and STATIC_CHECK_FALSE. As mentioned on Discord, my reason to propose those is to allow the following development workflow:

• Run my tests locally and have static_asserts so that failures happen soon at compile time, so that I can be noticed early without having to compile and run the rest of the tests.
• Have a complete and readable report in the CI by defining CATCH_CONFIG_RUNTIME_STATIC_REQUIRE, transforming those failure into runtime ones that can be aggregated in the reporter.

Questions

There are two potential design issues with this PR:

• It is called STATIC_CHECK but effectively acts as a REQUIRE when it fails at compile time. It might be a bit confusing, but it is the behaviour I want so unless we invent a new name there isn't much we can do about it.
• The switch from compile time to runtime reporting currently depends on CATCH_CONFIG_RUNTIME_STATIC_REQUIRE. I think that it makes sense to have a single macro for both, but this macro name specifically mentions STATIC_REQUIRE, which is mildly confusing. Catch2 v3 isn't out, so it's theoretically still time to change the macro name if you feel like it. I personally don't mind.

[Morwenn]

I tried to approve the new tests as explained in the documentation, but some of the new files contain absolute file paths, which I believe is a problem.

It only seems to affect the xml and sonarqube files.

[horenmar]

What do the paths look like? The Approvals should already do some path normalization to avoid this.

[Morwenn]

xml.sw.approved.txt ends up with lines like this:

<Expression success="true" type="REQUIRE" filename="D:/Morwenn/Projets/Catch2/tests/<exe-name>/UsageTests/Decomposition.tests.cpp" >

And sonarqube.sw.approved.txt with lines like this:

<file path="D:/Morwenn/Projets/Catch2/tests/<exe-name>/IntrospectiveTests/InternalBenchmark.tests.cpp">

I ran buildAndTest.sh from Git Bash on Windows 10, by calling ./tools/scripts/buildAndTest.sh from the Catch2 directory, as explained in the documentation. I don't know whether it's got any influence on the result.

[horenmar]

So the problem is that the scripts taking care of normalization don't check for the right path.

I vaguely remember having this problem before (in fact I think the dev-stupid-windows-things branch came up due to that, but I never got around figuring out how to always get the right path.

Can you try adding print(catchPath) to tools/scripts/scriptCommon.py and running it?

[horenmar]

This should probably be edited to be along the lines of "STATIC_REQUIRE was introduced ..." and then having a "STATIC_CHECK was introduced in ..." separately.

[Morwenn]

Done.

[horenmar]

tests/ExtraTests/X01-PrefixedMacros.cpp should also be extended to include the new macro.

[Morwenn]

Can you try adding print(catchPath) to tools/scripts/scriptCommon.py and running it?

This prints D:\Morwenn\Projets\Catch2 when I run it from the root of the repository as python ./tools/scripts/scriptCommon.py. Both with cmd and from Git bash.

I also tried to run it with WSL2 just in case, where I get /mnt/d/Morwenn/Projets/Catch2. I don't know whether it's information you care about, but here we go.

[Morwenn]

tests/ExtraTests/X01-PrefixedMacros.cpp should also be extended to include the new macro.

Done, though I didn't notice a difference in the test output. Are the extra tests run by builtAndTest.sh or only built? I might have just missed it, the current result is quite built due to the other issue we're debugging :x

[horenmar]

D:\Morwenn\Projets\Catch2

D:/Morwenn/Projets/Catch2/tests//IntrospectiveTests/InternalBenchmark.tests.cpp

So the Python uses windows-standard path separators, while the compiler uses forward slashes. 🤦

[horenmar]

Can you try cherry-picking 83d9a63 to see if it fixes the problem?

[Morwenn]

Yeah, it's much better with that, it solves the issue 😄

How do we proceed next? Do you want to commit on devel so that I can rebase and push the approval updates?

[horenmar]

Do you want to commit on devel so that I can rebase and push the approval updates?

Yeah, it is merged to devel now.

Also don't worry about the CI failure, that one is caused by AppVeyor having some workers stuck with bad image.

[Morwenn]

Ok then. Are there still things to address, or can I proceed with the rebase squash?

[horenmar]

MORE TESTS!!!! 😈

Bit more seriously, I realized that at least one easy-to-write test is missing. Please add tests for both STATIC_REQUIRE and STATIC_CHECK to tests/ExtraTests/X02-DisabledMacros.cpp, specifically give them expression that evaluates to false -- if the cpp file keeps on compiling, then they are properly disabled.

It would also be nice to have a test for the motivating behaviour, which is that runtime-deferred STATIC_CHECK behaves as a CHECK, but there are currently no tests for runtime behaviour of STATIC_REQUIRE either, so I am willing to merge this PR without that and then add those later.

[Morwenn]

MORE TESTS!!!! 😈

No bully 😢

Bit more seriously, I realized that at least one easy-to-write test is missing. Please add tests for both STATIC_REQUIRE and STATIC_CHECK to tests/ExtraTests/X02-DisabledMacros.cpp, specifically give them expression that evaluates to false -- if the cpp file keeps on compiling, then they are properly disabled.

I added really simple ones to the existing Disabled Macros tests, I hope it's good enough.

It would also be nice to have a test for the motivating behaviour, which is that runtime-deferred STATIC_CHECK behaves as a CHECK, but there are currently no tests for runtime behaviour of STATIC_REQUIRE either, so I am willing to merge this PR without that and then add those later.

I'm willing to add those if needed, but I'm not sure where to put them. Maybe in SelfTests/UsageTestsMisc.tests.cpp?

[codecov]

Codecov Report

Merging #2318 (550ee7a) into devel (c4df47c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            devel    #2318   +/-   ##
=======================================
  Coverage   90.97%   90.97%           
=======================================
  Files         151      151           
  Lines        7235     7235           
=======================================
  Hits         6582     6582           
  Misses        653      653           

[horenmar]

I added really simple ones to the existing Disabled Macros tests, I hope it's good enough.

Yeah, those are fine.

I'm willing to add those if needed, but I'm not sure where to put them. Maybe in SelfTests/UsageTestsMisc.tests.cpp?

They would require a bunch of work, because they have to be integration-level checks (there needs to be check that 2 assertions failed in the test case that contains the STATIC_CHECKs, etc). I am going to merge this later today, and finish the tests on my own tomorrow.

[Morwenn]

Oh damn, integration tests? I guess that I will remain on the sidelines and just watch what you're doing just in case I ever need to do something similar in the future 😅