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

Fix catch_discover_tests() from breaking on long test names #1658

Merged

Conversation

Quincunx271
Copy link
Contributor

Fixes #1645 by rolling back d4eec01 ; reopens #1590 .

Parsing --list-tests is broken, as Catch automatically line wraps the line when it gets too long, stripping any whitespace in the process. This means that it's impossible to reproduce the exact name of the
test if the test's name is long enough to line-wrap.

Furthermore, overwriting the LABELS property with the discovered labels breaks users who manually added custom ctest labels. However, this seems to not actually overwrite the labels due to buggy handling of our strange test names by CMake.

Rolling back to using --list-test-names-only for now, as it does not wrap lines even on very long test names.

We may be able parse the output of --list-tags to produce the ctest labels. However, the straightforward way of doing this is to use CMake's get_property(TEST ...) and set_property(TEST ... APPEND ...), which don't work if the test name has spaces or other special characters. Such test names may be illegal test names anyway—the documentation doesn't state whether it's allowed or disallowed for the variant of add_test(...) we are using—although they appear to work.

Upstream CMake issue on allowing custom strings as test names, or at least custom test descriptions: https://gitlab.kitware.com/cmake/cmake/issues/19391

Parsing --list-tests is broken, as Catch automatically line wraps the
line when it gets too long, stripping any whitespace in the process.
This means that it's impossible to reproduce the exact name of the
test if the test's name is long enough to line-wrap.

Furthermore, overwriting the LABELS property with the discovered labels
breaks users who manually added custom ctest labels.

Rolling back to using --list-test-names-only for now, as it does not
wrap lines even on very long test names.

We may be able parse the output of --list-tags to produce the ctest labels.
However, the straightforward way of doing this is to use CMake's
get_property(TEST ...) and set_property(TEST ... APPEND ...), which don't
work if the test name has spaces or other special characters. We would
need to mangle the test name to a valid CMake identifier to do it that way.
@Quincunx271
Copy link
Contributor Author

I have a partial implementation of making Catch labels work as CTest labels: https://github.com/Quincunx271/Catch2/blob/3047ee81052b35f9867b61c15a81c9df2b539621/contrib/CatchAddTests.cmake . But this is broken due to the handling of get_property(TEST ...) and set_property(TEST ... APPEND ...) as mentioned above.

@codecov
Copy link

codecov bot commented Jun 16, 2019

Codecov Report

Merging #1658 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1658   +/-   ##
=======================================
  Coverage   83.66%   83.66%           
=======================================
  Files         123      123           
  Lines        3403     3403           
=======================================
  Hits         2847     2847           
  Misses        556      556

1 similar comment
@codecov
Copy link

codecov bot commented Jun 16, 2019

Codecov Report

Merging #1658 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1658   +/-   ##
=======================================
  Coverage   83.66%   83.66%           
=======================================
  Files         123      123           
  Lines        3403     3403           
=======================================
  Hits         2847     2847           
  Misses        556      556

@horenmar horenmar added the Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration. label Jun 17, 2019
@horenmar
Copy link
Member

Looks good and it is good to hear that the test name restriction is likely to be lifted.

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.

catch_discover_tests breaks when test description is long (wraps with --list-tests)
2 participants