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

Add test tags as cmake label #2690

Open
wants to merge 4 commits into
base: devel
Choose a base branch
from
Open

Add test tags as cmake label #2690

wants to merge 4 commits into from

Conversation

uyha
Copy link
Contributor

@uyha uyha commented May 21, 2023

Description

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

GitHub Issues

#1590, #2613

@codecov
Copy link

codecov bot commented May 21, 2023

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
Copy link
Contributor Author

uyha commented May 23, 2023

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

@horenmar
Copy link
Member

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

add_command(set_tests_properties
"${prefix}${test}${suffix}"
PROPERTIES
ENVIRONMENT_MODIFICATION "${environment_modifications}")
LABELS "${tags}"
Copy link
Member

Choose a reason for hiding this comment

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

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

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

  1. 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.

  1. 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.

@uyha
Copy link
Contributor Author

uyha commented Jun 15, 2023

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
Copy link
Contributor Author

uyha commented Jun 15, 2023

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

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
Copy link
Contributor Author

uyha commented Jun 15, 2023

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

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
Copy link
Contributor Author

uyha commented Jun 15, 2023

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

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.

@uyha uyha mentioned this pull request Jun 18, 2023
@nilsvu
Copy link

nilsvu commented Jul 31, 2023

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

@uyha
Copy link
Contributor Author

uyha commented Jul 31, 2023

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

@uyha
Copy link
Contributor Author

uyha commented Oct 31, 2023

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

@uyha uyha force-pushed the cmake-labels branch 2 times, most recently from 4d71b1e to 53968c9 Compare November 15, 2023 08:45
@uyha uyha requested a review from horenmar November 15, 2023 20:50
Comment on lines +171 to +178
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()
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@horenmar
Copy link
Member

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

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

@horenmar
Copy link
Member

Two more general thoughts

  1. 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.
  2. 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
Copy link

Bidski commented Mar 5, 2024

Is there any progress on this?

@uyha
Copy link
Contributor Author

uyha commented Mar 6, 2024

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

LecrisUT commented Mar 6, 2024

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
Copy link
Contributor Author

uyha commented Mar 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants