Cmake labels (take-over #2832)

[Bidski]

Continuing where @uyha (#2690) and @LecrisUT (#2832) left off

Adds a CMake reporter to list test cases.

Test cases are reported as a CMake list of lists with list elements wrapped in long form arguments.

Here is example output from ./debug-build/tests/ctest-registration-test/tests --reporter cmake --list-tests

[[[=[@Script[C:\EPM1A]=x;"SCALA_ZERO:"]=];[=[]=];[=[/home/abiddulph/Projects/Catch2/tests/TestScripts/DiscoverTests/register-tests.cpp]=];[=[11]=];[=[[==[script regressions]==]]=]]];[[[=[Some test]=];[=[]=];[=[/home/abiddulph/Projects/Catch2/tests/TestScripts/DiscoverTests/register-tests.cpp]=];[=[12]=];]];[[[=[Let's have a test case with a long name. Longer. No, even longer. Really looooooooooooong. Even longer than that. Multiple lines worth of test name. Yep, like this.]=];[=[]=];[=[/home/abiddulph/Projects/Catch2/tests/TestScripts/DiscoverTests/register-tests.cpp]=];[=[13]=];]];[[[=[And now a test case with weird tags.]=];[=[]=];[=[/home/abiddulph/Projects/Catch2/tests/TestScripts/DiscoverTests/register-tests.cpp]=];[=[16]=];[=[[==[foo,bar]==];[==[tl;dr]==];[==[tl;dw]==]]=]]]

[codecov]

Codecov Report

Attention: Patch coverage is 76.82927% with 19 lines in your changes missing coverage. Please review.

Project coverage is 90.97%. Comparing base (4e8d92b) to head (c69ca3d).
Report is 22 commits behind head on devel.

Additional details and impacted files

@@            Coverage Diff             @@
##            devel    #2871      +/-   ##
==========================================
- Coverage   91.10%   90.97%   -0.12%     
==========================================
  Files         198      199       +1     
  Lines        8493     8575      +82     
==========================================
+ Hits         7737     7801      +64     
- Misses        756      774      +18     

[LecrisUT]

I was thinking more along the lines of set or if that doesn't work, eval. Something built-in to handle this.

Better yet, can't the reporter spit out a full cmake script with all the relevant set, list instructions etc.? Technically it is a security vulnerability, but I think there are better ways to attack the test script itself rather than test names

[Bidski]

Generating code to be eval'd just sounds like a horrible idea, and as you point out, it's a security issue waiting to happen and I don't think it would be wise to take this approach.

[LecrisUT]

The only reason I suggested the block syntax was to take advantage of the built-in cmake handling. If we don't plan on using it I would prefer the original list of list approach with the escaping mechanism described in this file.

    # Escape characters in test case names that would be parsed by Catch2
    # Note that the \ escaping must happen FIRST! Do not change the order.
    foreach(char \\ , [ ])
      string(REPLACE ${char} "\\${char}" test_name "${test_name}")
    endforeach(char)

Security wise I don't think it's a significant attack vector, but you can still minimize it by disallowing ]=] syntax in the name, tag, file, etc.

[Bidski]

By "block syntax" I assume you mean [=[/]=]? Unfortunately, CMake is seeing those and assuming that is the content of the string and never parses further than than (which I believe is how these strings are intended to work), which I why I have to strip the brackets off in order to process the list within.

We can revert to what I had before I opened this PR (I only escaped the the ; originally) and not use the extended bracket syntax. I admit that syntax is pretty clunky when it doesn't really work the way we would like. I left the escaping of \, ,, [, and ] in the CMake file since that escaping is more specific to putting it into the CTest file and may not be a necessary thing for others you may want to use the CMake reporter for a different purpose (I am assuming we should make the CMake reporter slightly more generic -- if we were to make it with the explicit purpose of using it in this CMake script then we may as well skip the CMake script entirely and just have the reporter dump the CTest script, but this is just as much of a security issue as generating CMake set commands)

Square brackets can't appear in tags already (I tested this, Catch2 forbids it). I'm not sure it is worth forbidding them from the test names.

It may not be a significant attack vector, but willingly introducing an attack vector just seems like a bad idea.

[LecrisUT]

By "block syntax" I assume you mean [=[/]=]? Unfortunately, CMake is seeing those and assuming that is the content of the string and never parses further

Indeed, the block/bracket syntax is only meant to be used within a CMake script. You only see it used in generated files to be parsed or eval.

I left the escaping of \, ,, [, and ] in the CMake file since that escaping is more specific to putting it into the CTest file

For the reporter you would have to be even more thorough unfortunately, escaping $ and other special symbols. This escape might also be needed with the manual handling of brackets here.

you may want to use the CMake reporter for a different purpose

The reporter should do only one thing, generate a CMake list of list. Which format is more appropriate is debatable. The choice is between the complexity of escaping list of lists and the potential security threats.

I personally think we can address the later quite easily in this case and having a cmake script would be more readable.

if we were to make it with the explicit purpose of using it in this CMake script then we may as well skip the CMake script entirely and just have the reporter dump the CTest script

I'm ok with either. The reusability of such a script outside ctest file generation is super limited since you have to finish building the whole project at that point. The only usage of this splitting is readability, but at that point the ctest file itself is more readable.

Hooking into the ctest file generation directly could be very useful for customizing the test properties, e.g. setting PROCESSOR test property for individual tests. It would require expanding the macro syntax though.

[Bidski]

Do we just want to replace the cmake reporter with a utility (a reporter??) that just generates the ctest file then?

[LecrisUT]

I would vote for that approach. @horenmar can you chime in on this? Unfortunately I don't have a reference for other test-suites to see how they implement this, gtest for example doesn't have tight CMake support (gtest_discover_tests is defined in CMake).

For simplicity I would export all input variables like ${TEST_PROPERTIES} to environment variables like $ENV{CATCH_TEST_PROPERTIES} instead of defining additional --flags.

I don't think it needs to be a separate utility, but I didn't dig deep enough in the code to know what pitfalls I am overlooking. Maybe the confusion would be that "reporter" actually outputs the output of the run, and not just the structure, but we seem to be using the JSON one in a similar way.

[Bidski]

@horenmar do you have an opinion on this? Would really like to get this sorted out

[LecrisUT]

Can the number of equal signs be a variable, just in case? The brackets should also be limited to where we want to escape the strings, i.e. name, tag, etc. If we go with writing a full script it would be even more minimal.

An example script could be

set(test_data)
set(test_name [[<insert_test_name>]])
set(test_file [[<insert_test_file>]])
set(test_tags [[<insert_test_tag_0>]] ...)
list(APPEND test_data "${test_name};${test_file};${test_tags}")

You might need to play around with join/split and the need for escaping or not for list parameters like test_tags

[horenmar]

I just noticed that this PR was supposed to supersede the previous work using the JSON reporter.

Sorry for keeping it open this long, but this is a hard no. I am not interested in adding a CMake-specific test lister to the default build.