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 Integration does not support test names containing semicolons and backslashes #2674

Closed
robinchrist opened this issue Apr 19, 2023 · 1 comment
Labels
Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration.

Comments

@robinchrist
Copy link
Contributor

robinchrist commented Apr 19, 2023

Describe the bug
One of our working students has created several test cases for the parser of a custom DSL which often contains ; (semicolon) and Windows-style file paths in expressions and has named them after the expression that is being tested if short enough, e.g.

TEST_CASE("@Script[C:\\EPM1A]=x;\"SCALA_ZERO:\"", "[script regressions]")

This leads to the creation of a lot of wrong test cases, potentially leading to false positives:

The following tests FAILED:
         13 - @Script[C:\EPM1A]=x (Failed)
         14 - "SCALA_ZERO:" (Failed)

This can also be seen in the corresponding tests-<hash>-cmake:

add_test( [==[@Script[C:\EPM1A]=x]==] /censored/path [==[@Script\[C:\EPM1A\]=x]==]  )
set_tests_properties( [==[@Script[C:\EPM1A]=x]==] PROPERTIES WORKING_DIRECTORY /censored/path)
add_test( [==["SCALA_ZERO:"]==] /censored/path [==["SCALA_ZERO:"]==]  )
set_tests_properties( [==["SCALA_ZERO:"]==] PROPERTIES WORKING_DIRECTORY /censored/path)

By adding some debug logging to CatchAddTests.cmake, we can see that it's a bug in the foreach logic:

Debug log statements:

message("Line: ${test}")
message("Escaped Line: ${test_name}")
Line: @Script[C:\EPM1A]=x
Escaped Line: @Script\[C:\EPM1A\]=x
Line: "SCALA_ZERO:"
Escaped Line: "SCALA_ZERO:"

I believe this is due to

string(REPLACE "\n" ";" output "${output}")

string(REPLACE "\n" ";" output "${output}")

Obviously, if the test case name already contains ;, the test case will be split and it won't work.

string(REPLACE ";" "\;" output "${output}")
string(REPLACE "\n" ";" output "${output}")

seems to fix this.

One thing we can also see in the resulting output file (one we have applied the fix above):

add_test( [==[@Script[C:\EPM1A]=x;"SCALA_ZERO:"]==] /censored/path [==[@Script\[C:\EPM1A\]=x;"SCALA_ZERO:"]==]  )

If we take the part @Script\[C:\EPM1A\]=x;"SCALA_ZERO:" and do
test-binary '@Script\[C:\EPM1A\]=x;"SCALA_ZERO:"' we will get:

No tests ran

Uh oh!

Well, there's another issue: \ in test names are not properly escaped.
We need to change foreach(char , [ ]) to foreach(char \\ , [ ])
Note that the order is important: The replacement of \ needs to happen first!

Additionally, the strings must be handles correctly (i.e. set in quotes everywhere to preserve the ;, otherwise CMake will do funny list stuff...)

So in total this looks like:

string(REPLACE ";" "\;" output "${output}")
string(REPLACE "\n" ";" output "${output}")

...

# Parse output
foreach(line ${output})
  set(test "${line}")
  # Escape characters in test case names that would be parsed by Catch2
  set(test_name "${test}")
  foreach(char \\ , [ ])
    string(REPLACE ${char} "\\${char}" test_name "${test_name}")
  endforeach(char)
  # ...add output dir
  if(output_dir)
    string(REGEX REPLACE "[^A-Za-z0-9_]" "_" test_name_clean "${test_name}")
    set(output_dir_arg "--out ${output_dir}/${output_prefix}${test_name_clean}${output_suffix}")
  endif()

Expected behavior
Only a single test should be created

Reproduction steps
Create a test case containing a semicolon in its name and use the CMake integration, e.g.

TEST_CASE("@Script[C:\\EPM1A]=x;\"SCALA_ZERO:\"", "[script regressions]")

Platform information:
CMake 3.22.2
Catch2 v3.3.2

Additional context
Sorry, I added the backslashes part later, so this is all a bit incoherent...

I should probably create a pull request with the fixed code?

@horenmar horenmar added the Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration. label Apr 19, 2023
@horenmar
Copy link
Member

That's a nice and high quality issue, thanks. I see you already opened a PR so I am gonna write the rest there.

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

No branches or pull requests

2 participants