Add test tags as cmake label #2690

uyha · 2023-05-21T12:03:28Z

Description

This allows people running ctest -L 'tag' to run tests matching the tags

GitHub Issues

#1590, #2613

codecov · 2023-05-21T12:33:22Z

Codecov Report

Merging #2690 (3cf405b) into devel (733b901) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##            devel    #2690   +/-   ##
=======================================
  Coverage   91.46%   91.46%           
=======================================
  Files         194      194           
  Lines        8188     8188           
=======================================
  Hits         7489     7489           
  Misses        699      699

uyha · 2023-05-23T07:49:54Z

@horenmar sorry for pinging, but could you have a look at this PR?

horenmar · 2023-05-23T21:20:57Z

I want to do #2676 first, and that waits until I have a free weekend that I can spend on it. 🤷

extras/CatchAddTests.cmake

horenmar · 2023-06-15T18:54:16Z

extras/CatchAddTests.cmake

      add_command(set_tests_properties
        "${prefix}${test}${suffix}"
        PROPERTIES
-        ENVIRONMENT_MODIFICATION "${environment_modifications}")
+        LABELS "${tags}"


IIRC this needs to APPEND the labels property, to avoid overwriting user-provided labels for the same tests. There should be a discussion of this somewhere in #1590.

horenmar · 2023-06-15T19:06:20Z

Well, I finally had time to fix the other PR and unblock this one. I also looked around various other attempts and old issues, and there is one more problem that is not handled by this, and I am not sure it can be handled with this approach, see #1658.

Basically with a long test name like "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec eu est ut lacus luctus volutpat. Duis convallis massa non neque eleifend porta vel est.", --list-tests will linebreak the test name roughly like this

  Lorem ipsum dolor sit amet, consectetur adipiscing elit. Donec eu est ut
  lacus luctus volutpat. Duis convallis massa non neque eleifend porta vel est.

This will break the registration script, as it does not understand that it needs to reconstruct the two lines into one test, and in general it cannot figure this out -> if we have test cases without tags, there is no way to tell whether the two lines above are two different test cases, or one looong test case.

And even if it was possible to figure this out (either a different indentation width for subsequent lines, or a line break marker), we still cannot figure out what whitespace separator is supposed to be there (in the example above, it is a space, but it could also be a tab, or even some weirder WS char).

This leads me to conclude that parsing --list-tests will not work, and the script has to do this differently.

As I see it, there are two options

Ask for the xml output and parse it with ad-hoc regexes.

This is kinda brittle, but it would work, and IIRC the XML reporter output is already tested for being unchanged, so we could just make it so that the registration script needs to be updated if the XML reporter output changes.

Add a JSON reporter and parse that -> CMake has builtin support for JSON since 3.19, so we could use that for work with the structured output.

This requires more changes to Catch2 and forces newer CMake versions.

extras/CatchAddTests.cmake

uyha · 2023-06-15T19:10:51Z

Add a JSON reporter and parse that -> CMake has builtin support for JSON since 3.19, so we could use that for work with the structured output.

ok, so I guess I should open a new PR following this direction. Are you ok with requiring CMake 3.19?

uyha · 2023-06-15T19:20:40Z

for adding a JSON reporter, I guess the most straightforward approach is to follow the XML reporter, right? or is there a specification for JSON reporter also?

LecrisUT · 2023-06-15T19:26:52Z

2. Add a JSON reporter and parse that -> [CMake has builtin support for JSON](https://cmake.org/cmake/help/latest/command/string.html#id2) since 3.19, so we could use that for work with the structured output.

This requires more changes to Catch2 and forces newer CMake versions.

This seems reasonable and modern. But what's the plan on how to get the json parser? Presumably using library, but then would it be a hard dependency? Would it be bundled and/or use FetchContent?

uyha · 2023-06-15T19:42:22Z

But what's the plan on how to get the json parser?

I think a custom JSON writer will be written, similar to the XML writer.

LecrisUT · 2023-06-15T20:47:19Z

I think a custom JSON writer will be written, similar to the XML writer.

Maybe the reporter is doable (probably already have a string sanitizer for quotations etc.), but the parser sounds like could be a bit tricky.

uyha · 2023-06-15T21:49:28Z

I don't think we need a JSON parser in Catch2 code, we only need to use the CMake JSON parser for parsing the tests and tags.

LecrisUT · 2023-06-15T22:12:33Z

Oh yeah, I misread the previous comment, only the reporter is needed. Isn't there a widely used json format for test-suite data junit? I've recently seen that github action is annotating warnings and some errors directly in the code source. Maybe if there's a format that github action can pick up, it would be nice to have these tests reported in source as well.

nilsvu · 2023-07-31T16:35:32Z

Hi @uyha, thanks for working on this. Do you have plans to complete this feature? The Catch2+CTest community would thank you :)

uyha · 2023-07-31T16:37:50Z

@nilsvu yes, you can follow the progress of #2706 for the JSON reporter, after the reporter is done, then this should be trivial.

uyha · 2023-10-31T22:06:56Z

@horenmar This PR is complete now, as soon as #2706 is complete, this PR should be good to merge too.

extras/CatchAddTests.cmake

This allows people running `cmake -L 'tag'` to run tests matching the tags

horenmar · 2023-11-15T22:43:02Z

extras/CatchAddTests.cmake

+      foreach(tag_index RANGE "${tag_end}")
+        string(JSON tag GET "${tags}" "${tag_index}")
+        add_command(set_tests_properties
+          "${prefix}${test}${suffix}"
+          PROPERTIES
+            LABELS "${tags}"
+        )
+      endforeach()


I am not sure why, but this does not do what it is supposed to. The resulting ctest script looks like this:

add_test( [==[And now a test case with weird tags.]==] /home/xarn/builds/Catch2/uyha-json-reporter/tests/ctest-registration-test/tests [==[And now a test case with weird tags.]==] ) set_tests_properties( [==[And now a test case with weird tags.]==] PROPERTIES WORKING_DIRECTORY /home/xarn/builds/Catch2/uyha-json-reporter/tests/ctest-registration-test) set_tests_properties( [==[And now a test case with weird tags.]==] PROPERTIES LABELS [==[[ "foo,bar", "tl;dr", "tl;dw" ]]==]) set_tests_properties( [==[And now a test case with weird tags.]==] PROPERTIES LABELS [==[[ "foo,bar", "tl;dr", "tl;dw" ]]==]) set_tests_properties( [==[And now a test case with weird tags.]==] PROPERTIES LABELS [==[[ "foo,bar", "tl;dr", "tl;dw" ]]==])

Which has the obvious issue of setting the same set of labels multiple times, but also it should set each tag as its own label, rather than the amalgamation of all tags as one big label.

Also check #1658 for proper handling of labels -> they need to be appended, rather than just set.

The reason the amalgamation is there is LABELS "${tags}", rather than LABELS "${tag}".

horenmar · 2023-11-15T22:47:00Z

Also the add_command(set ${_TEST_LIST} ${tests}) around line 183 is currently broken, this is the result in the script file:

set( tests_TESTS [==[[
  {
    "class-name" : "",
    "name" : "@Script[C:\\EPM1A]=x;\"SCALA_ZERO:\"",
    "source-location" :
    {
      "filename" : "/mnt/c/ubuntu/Catch2-uyha/tests/TestScripts/DiscoverTests/register-tests.cpp",
      "line" : 11
    },
    "tags" : [ "script regressions" ]
  },
  {
    "class-name" : "",
    "name" : "Some test",
    "source-location" :
    {
      "filename" : "/mnt/c/ubuntu/Catch2-uyha/tests/TestScripts/DiscoverTests/register-tests.cpp",
      "line" : 12
    },
    "tags" : []
  },
  {
    "class-name" : "",
    "name" : "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.",
    "source-location" :
    {
      "filename" : "/mnt/c/ubuntu/Catch2-uyha/tests/TestScripts/DiscoverTests/register-tests.cpp",
      "line" : 13
    },
    "tags" : []
  },
  {
    "class-name" : "",
    "name" : "And now a test case with weird tags.",
    "source-location" :
    {
      "filename" : "/mnt/c/ubuntu/Catch2-uyha/tests/TestScripts/DiscoverTests/register-tests.cpp",
      "line" : 16
    },
    "tags" : [ "foo,bar", "tl;dr", "tl;dw" ]
  },
  {
    "class-name" : "",
    "name" : "Test with tags",
    "source-location" :
    {
      "filename" : "/mnt/c/ubuntu/Catch2-uyha/tests/TestScripts/DiscoverTests/register-tests.cpp",
      "line" : 17
    },
    "tags" : [ "a", "b", "c" ]
  },
  {
    "class-name" : "",
    "name" : "Test with different tags",
    "source-location" :
    {
      "filename" : "/mnt/c/ubuntu/Catch2-uyha/tests/TestScripts/DiscoverTests/register-tests.cpp",
      "line" : 18
    },
    "tags" : [ "a", "d", "e" ]
  },
  {
    "class-name" : "",
    "name" : "Test with more tags",
    "source-location" :
    {
      "filename" : "/mnt/c/ubuntu/Catch2-uyha/tests/TestScripts/DiscoverTests/register-tests.cpp",
      "line" : 19
    },
    "tags" : [ "b", "c", "g" ]
  }
]]==] [==[@Script[C:\EPM1A]=x]==] [==["SCALA_ZERO:"]==] [==[Some test]==] [==[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.]==] [==[And now a test case with weird tags.]==])

This has two issues

The JSON should not be there.
The file file with the semicolon in name is split into two elements in the list, which it is not supposed to be.

horenmar · 2023-11-15T23:05:03Z

Two more general thoughts

The iterative handling of JSON in CMake is slooooow. That is because every time there is a call like this string(JSON test_spec GET "${tests}" "${index}"), CMake has to reparse the whole ${tests} string as JSON, check that it is a list, retrieve the element at index and then promptly forget the json parsed representation. For tags it is unlikely to matter, but for tests this might eventually be an issue. Catch2's SelfTest has over 400 test cases, which leads to ~140k JSON file, and I would not consider that test suite to be particularly large. However, fixing this is not required for this to be merged.
The changes here introduce hard dependency on relatively new CMake versions (3.19 for JSON support, potentially more around the label handling), so in the end the new script should live side by side with the old one for some time.

Bidski · 2024-03-05T04:54:13Z

Is there any progress on this?

uyha · 2024-03-06T09:28:03Z

Is there any progress on this?

I currently am not working on this (no idea when I will resume), so I hope someone can pick this up or maybe I will find the time to work on this at some point.

LecrisUT · 2024-03-06T09:32:40Z

I could probably take a look and take-over. A todo list of what needs to be done and/or some discussion of how this feature should be tested would help a lot.

uyha · 2024-03-06T09:36:55Z

I think only the comments from @horenmar above and that should be it.

Bidski mentioned this pull request Jun 1, 2023

Make catch tags work with ctest labels #2696

Closed

horenmar reviewed Jun 15, 2023

View reviewed changes

extras/CatchAddTests.cmake Outdated Show resolved Hide resolved

horenmar reviewed Jun 15, 2023

View reviewed changes

extras/CatchAddTests.cmake Outdated Show resolved Hide resolved

horenmar reviewed Jun 15, 2023

View reviewed changes

extras/CatchAddTests.cmake Outdated Show resolved Hide resolved

uyha force-pushed the cmake-labels branch from 00ecff7 to 3b337ab Compare June 15, 2023 19:24

uyha mentioned this pull request Jun 18, 2023

Add JSON reporter #2706

Merged

uyha force-pushed the cmake-labels branch from 3b337ab to 6946b3a Compare October 27, 2023 12:14

horenmar reviewed Nov 14, 2023

View reviewed changes

extras/CatchAddTests.cmake Outdated Show resolved Hide resolved

uyha force-pushed the cmake-labels branch 2 times, most recently from 4d71b1e to 53968c9 Compare November 15, 2023 08:45

uyha added 4 commits November 15, 2023 20:27

Add test tags as cmake label

0645765

This allows people running `cmake -L 'tag'` to run tests matching the tags

Complete adding tags as labels to tests using the JSON reporter

c541a31

Remove custom escape logic

41ecdb6

Quote variable dereference properly

3cf405b

uyha force-pushed the cmake-labels branch from 53968c9 to 3cf405b Compare November 15, 2023 20:27

uyha requested a review from horenmar November 15, 2023 20:50

horenmar reviewed Nov 15, 2023

View reviewed changes

abeltrano mentioned this pull request Jan 25, 2024

Add CMake test preset for running non-root tests microsoft/netremote#130

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test tags as cmake label #2690

Add test tags as cmake label #2690

uyha commented May 21, 2023 •

edited

codecov bot commented May 21, 2023 •

edited

uyha commented May 23, 2023

horenmar commented May 23, 2023

horenmar Jun 15, 2023

horenmar commented Jun 15, 2023

uyha commented Jun 15, 2023

uyha commented Jun 15, 2023

LecrisUT commented Jun 15, 2023

uyha commented Jun 15, 2023

LecrisUT commented Jun 15, 2023

uyha commented Jun 15, 2023

LecrisUT commented Jun 15, 2023

nilsvu commented Jul 31, 2023

uyha commented Jul 31, 2023

uyha commented Oct 31, 2023

horenmar Nov 15, 2023

horenmar Nov 15, 2023

horenmar commented Nov 15, 2023

horenmar commented Nov 15, 2023

Bidski commented Mar 5, 2024

uyha commented Mar 6, 2024

LecrisUT commented Mar 6, 2024

uyha commented Mar 6, 2024

Add test tags as cmake label #2690

Are you sure you want to change the base?

Add test tags as cmake label #2690

Conversation

uyha commented May 21, 2023 • edited

Description

GitHub Issues

codecov bot commented May 21, 2023 • edited

Codecov Report

uyha commented May 23, 2023

horenmar commented May 23, 2023

horenmar Jun 15, 2023

Choose a reason for hiding this comment

horenmar commented Jun 15, 2023

uyha commented Jun 15, 2023

uyha commented Jun 15, 2023

LecrisUT commented Jun 15, 2023

uyha commented Jun 15, 2023

LecrisUT commented Jun 15, 2023

uyha commented Jun 15, 2023

LecrisUT commented Jun 15, 2023

nilsvu commented Jul 31, 2023

uyha commented Jul 31, 2023

uyha commented Oct 31, 2023

horenmar Nov 15, 2023

Choose a reason for hiding this comment

horenmar Nov 15, 2023

Choose a reason for hiding this comment

horenmar commented Nov 15, 2023

horenmar commented Nov 15, 2023

Bidski commented Mar 5, 2024

uyha commented Mar 6, 2024

LecrisUT commented Mar 6, 2024

uyha commented Mar 6, 2024

uyha commented May 21, 2023 •

edited

codecov bot commented May 21, 2023 •

edited