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

Automatically add TEST_SUITE labels to discovered tests #464

Merged
merged 3 commits into from
Feb 18, 2021

Conversation

shivupa
Copy link
Contributor

@shivupa shivupa commented Jan 29, 2021

The PR is against dev, and I have read the CONTRIBUTING.md. I understand that usually a issue is opened for discussion, but I figured the draft PR would illustrate the changes better.

Description

What:
When using doctest_discover_tests, the LABELS property is set for the generated test based on the TEST_SUITE.

Why:
In a downstream project using CMake, I found it useful to group each TEST_CASE in a TEST_SUITE. I found myself wanting to selectively run tests based on what TEST_SUITE they belong to, e.g, ctest -L TEST_SUITE_NAME. The test discovery returns the list of test, but modifying each one would be tedious.

GitHub Issues

#302 is related, but the discussion doesn't seem to indicate that this is the desired solution.

@onqtam
Copy link
Member

onqtam commented Feb 2, 2021

Hi,

In a world where we had native XML parsing support in CMake it would have been better to switch to parsing the XML output in doctestAddTests.cmake instead of the console output - there you'll be able to get all test cases and their test suites (as an attribute to the test cases - see here) with a single execute_process call (as compared to the current approach - with one for each test case). But we don't live in that world...

My concern with the current approach is that if there are thousands of test cases there would be thousands of invocations of the binary just to discover the test suite of the test cases. Perhaps this labeling shouldn't be on by default and should be guarded in an if()...

I'll leave this PR open for now (hopefully will get more feedback from others as I'm not a user of these CMake scripts) but you should be free to use these changes locally for now.

@shivupa
Copy link
Contributor Author

shivupa commented Feb 12, 2021

Hi,

In a world where we had native XML parsing support in CMake it would have been better to switch to parsing the XML output in doctestAddTests.cmake instead of the console output - there you'll be able to get all test cases and their test suites (as an attribute to the test cases - see here) with a single execute_process call (as compared to the current approach - with one for each test case). But we don't live in that world...

My concern with the current approach is that if there are thousands of test cases there would be thousands of invocations of the binary just to discover the test suite of the test cases. Perhaps this labeling shouldn't be on by default and should be guarded in an if()...

Sorry for the delay I had some deadlines. I also was worried about this. In my use case there are only a few (<100) tests so there is no perceptible overhead, but this clearly could vary. I hope this method can serve as a stand in until there is a cleaner XML based method. I wrapped this in an if() so now doctest_discover_tests(main_test ADD_LABELS 1) will add the labels.

I'll leave this PR open for now (hopefully will get more feedback from others as I'm not a user of these CMake scripts) but you should be free to use these changes locally for now.

I'm fine with leaving this open for as long as you'd like. I am working from the branch for the time being so it doesn't hold me up!

@onqtam onqtam merged commit 36e0e04 into doctest:dev Feb 18, 2021
@onqtam
Copy link
Member

onqtam commented Feb 18, 2021

thanks for the contribution!

@shivupa shivupa deleted the cmaketestlabels branch February 18, 2021 18:10
onqtam pushed a commit that referenced this pull request Mar 22, 2021
@jecassis
Copy link

Seeing a failure in test discovery when upgrading to 2.4.6 due to this feature when labels are not explicitly set. Example:

doctest_discover_tests(MyTests)

This is the error:

 CMake Error at .../scripts/cmake/doctestAddTests.cmake:60 (if):
    if given arguments:
  
      "EQUAL" "1"
  
    Unknown arguments specified

To workaround, need to explicitly set ADD_LABELS to 0:

doctest_discover_tests(MyTests ADD_LABELS 0)

Please take care of the scenario where ADD_LABELS is an empty string so that the original and non-argumented test discovery directive maintains its original behavior.

@shivupa
Copy link
Contributor Author

shivupa commented Mar 30, 2021

doctest_discover_tests(MyTests)

Hi Thanks for reporting this I opened #489. I'll update there once the issue is fixed.

@shivupa
Copy link
Contributor Author

shivupa commented Mar 30, 2021

Seeing a failure in test discovery when upgrading to 2.4.6 due to this feature when labels are not explicitly set. Example:

doctest_discover_tests(MyTests)

This is the error:

 CMake Error at .../scripts/cmake/doctestAddTests.cmake:60 (if):
    if given arguments:
  
      "EQUAL" "1"
  
    Unknown arguments specified

To workaround, need to explicitly set ADD_LABELS to 0:

doctest_discover_tests(MyTests ADD_LABELS 0)

Please take care of the scenario where ADD_LABELS is an empty string so that the original and non-argumented test discovery directive maintains its original behavior.

#490 fixes this.

@jharmer95
Copy link
Contributor

This is still broken, it can be fixed by changing it to the CMake-preferred syntax if(${add_labels}) rather than compare a string to 1. See PR #527 (from issue #524) for the fix

AkiyukiOkayasu added a commit to AkiyukiOkayasu/ame that referenced this pull request Oct 11, 2021
doctest/doctest#464 の問題でdoctest_discover_tests()が壊れているので、その直前の2.4.5にまで更新
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.

4 participants