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
Make catch tags work with ctest labels #1590
Comments
That sounds like an interesting and useful feature for the |
This has been implemented by @MaciejPatro. |
As it turns out, the original PR for this issue introduced bugs to the original functionality due to various factors, so it was reverted. There are also problems with adding tags as labels to the tests because of CMake upstream issue |
The upstream CMake issue has been merged into CMake 3.18. As such, we can go ahead and pull this feature back online after verifying the CMake version. As I mentioned in #1658, the LABELS should be APPEND-ed, not just set. See the partial implementation here: #1658 (comment) . That implementation should work after some adjustments. |
Hi @Quincunx271 - what's the status of this? I tried using the
But these tests do exist, for example (after reverting to the upstream versions of
|
There may be some mishandled quoting going on there. Check the generated set_property(TEST
utils:Binary Decoding
...
) It might need to look like this: set_property(TEST
"utils:Binary Decoding"
...
) That's my best guess. |
I got it working with this fix:
Once I got it working I noticed that it is adding the labels in the form
Because square brackets are special regex characters and
I would recommend not including the square brackets in the labels. What do you think? |
I agree with removing the square brackets. |
Ahh - I see what you mean about the It looks like maybe there is a CMake bug with Let me see about calculating all labels for a test and adding at once.
|
Got it working
Here's the patch. Basically changing the structure to
|
Seems to work ok with CMake 3.16.2, even with tags with spaces in them. What is a case that would break pre-3.18 that the "CMake upstream issue" fixes? |
It may have worked, but it was breaking the documented rules. To quote the CMake upstream issue:
And also:
So it might actually work pre-3.18, but relies on unintentional behavior. For that matter, there is another issue with the |
Ok - I'll add Is there any backwards-compatibility logic that will need to be added for pre-3.18 CMake versions? |
I would probably include a CMake version check that skips the label management if it didn't work. What I mean is, I would work backward to find the earliest CMake version for which the code works, then simply don't add labels if the CMake version is older than that. Or alternatively skip the "work backwards" step and only support this feature for 3.18+ |
I was starting to think about joining the result of I'll test older versions of CMake and see where it breaks. |
Except the user could decide to set properties of their own, e.g. their own labels: catch_discover_tests(target
PROPERTIES
LABEL CustomLabel
) |
I see, ok. |
I tried the
But CMake failed with For example:
I had to take another route and add Also I caught a bug in the previous iteration where it was only saving the first label because
when it should have been
Here's the full patch:
|
I tested with CMake versions back to CMake 3.4.3. Where there are multiple test executables that If there are multiple test executables then the second call to
Label support works for a single call to
|
I posted a question on the CMake Discourse: https://discourse.cmake.org/t/unknown-cmake-command-get-test-property-in-test-include-files/3828 about the |
From that discussion came this CMake issue: https://gitlab.kitware.com/cmake/cmake/-/issues/22473 |
@sourcedelica what's the status on this one? Is your patch ready to be applied, or does it need upstream modifications from CMake first? I tried to apply your patch, but didn't find commit 3047ee8. |
It's in my fork: https://github.com/Quincunx271/Catch2/tree/fix-catch-discover-tests-with-labels-from-tags |
@jdumas - it's ready to be applied as is @Quincunx271's branch. |
Ok thanks. Could this be merged into the main repo then? Seems the upstream CMake issue that was blocking this has been fixed now. |
Is there any update on this? |
The above is good to include all the test names into ctest so that
ctest -N
lists the test names. But it doesn't work with tags. ctest provides the LABEL property for a test so maybe the idea of tags in Catch is close to that.ctest --print-labels
lists all the test labels andctest -L <regex>
runs tests with labels matching the regex.The text was updated successfully, but these errors were encountered: