Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cmake labels (take-over #2832) #2871

Closed
wants to merge 10 commits into from
Closed

Conversation

Bidski
Copy link

@Bidski Bidski commented May 10, 2024

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]==]]=]]]

Copy link

codecov bot commented May 10, 2024

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     

extras/CatchAddTests.cmake Outdated Show resolved Hide resolved
# [[...test case list...]]
string(LENGTH "${test_case}" test_case_length)
math(EXPR test_case_length "${test_case_length} - 4")
string(SUBSTRING "${test_case}" 2 ${test_case_length} test_case)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@LecrisUT LecrisUT May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

out << ';';
if ( tags.empty() ) { return out; }

out << "[=[" << "[==[" << tags.front().original << "]==]";
Copy link
Contributor

@LecrisUT LecrisUT May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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.

@horenmar horenmar added the Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration. label Aug 13, 2024
@Bidski Bidski closed this Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants