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.

Sign up for GitHub

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

Jump to bottom

cmake: Add new DISCOVERY_MODE option to catch_discover_tests #2670

Merged
merged 2 commits into from
Apr 19, 2023
Merged

cmake: Add new DISCOVERY_MODE option to catch_discover_tests #2670

merged 2 commits into from
Apr 19, 2023

Conversation

hkaelber
Copy link
Contributor

@hkaelber hkaelber commented Apr 11, 2023 •
edited

Description

Provide DISCOVERY_MODE option to catch_discover_tests to allow for PRE_TEST discovery mode.

Cf. details in commit msg-es.

This change follows closely the corresponding functionality / changes in the GoogleTest counterpart to maintain the convenient similarity to gtest_discover_tests() and friends.

GitHub Issues

Closes #2493

CatchAddTests.cmake: Refactor into callable method
cc273a5 
Move test discovery logic into new catch_discover_tests_impl method
and make CatchAddTests aware of whether it is being launched in
CMake's script mode.

When launched in script mode, catch_discover_tests_impl is called
passing arguments obtained from the definitions passed into the call
to cmake. This preserves the existing behavior assumed by Catch.cmake.

Looking ahead, it also allows CatchAddTests to be included in
generated files and call catch_discover_tests_impl to perform test
discovery at test runtime with the new PRE_TEST discovery mode
introduced later.
@hkaelber hkaelber changed the title Hk/pre test discovery Catch.cmake: Add new DISCOVERY_MODE option to catch_discover_tests Apr 11, 2023
@hkaelber hkaelber changed the title Catch.cmake: Add new DISCOVERY_MODE option to catch_discover_tests cmake: Add new DISCOVERY_MODE option to catch_discover_tests Apr 11, 2023
@codecov
Copy link

codecov bot commented Apr 11, 2023 •
edited

Codecov Report

Merging #2670 (3211fac) into devel (9a2a4ea) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 3211fac differs from pull request most recent head d03fafe. Consider uploading reports for the commit d03fafe to get more accurate results

@@            Coverage Diff             @@
##            devel    #2670      +/-   ##
==========================================
+ Coverage   91.24%   91.25%   +0.01%     
==========================================
  Files         192      192              
  Lines        7808     7808              
==========================================
+ Hits         7124     7125       +1     
+ Misses        684      683       -1

@hkaelber hkaelber marked this pull request as ready for review April 11, 2023 10:48
horenmar
horenmar reviewed Apr 13, 2023
View reviewed changes
docs/cmake-integration.md Outdated Show resolved Hide resolved
extras/Catch.cmake Outdated Show resolved Hide resolved
extras/Catch.cmake Outdated Show resolved Hide resolved
Catch.cmake: Add new DISCOVERY_MODE option to catch_discover_tests
d03fafe 
Introducing a new DISCOVERY_MODE mode option, which provides greater
control over when catch_discover_tests perforsm test discovery.

It has two supported modes:
* POST_BUILD: The default behavior, which adds a POST_BUILD command
  to perform test discovery after the test has been built as was
  always done so far.

* PRE_TEST: New mode, which delays test discovery until test execution.
  The generated include file generates the appropriate CTest files at
  runtime and regenerates the CTest files when the executable is
  updated.
  This mode can be used in build-environments that don't allow for
  executing the linked binaries at build-time (like in a
  cross-compilation environment).

DISCOVERY_MODE can be controlled in two ways:
1. Setting the DISCOVERY_MODE when calling catch_discover_tests.

2. Setting the global CMAKE_CATCH_DISCOVER_TESTS_DISCOVERY_MODE prior
   to calling gtest_discover_tests.

Closes catchorg#2493
@hkaelber
Copy link
Contributor Author

Sorry, forgot that you prefer fixup commits in your review process. But shoudl not be a big deal in this PR, pls cf. incremental diff here

@horenmar
Copy link
Member

LGTM, thanks.

@horenmar horenmar merged commit aad926b into catchorg:devel Apr 19, 2023
71 checks passed
@riandrake
Copy link

Hi, sorry if this is the wrong place. When trying this, I get an error in CMake:

CMake Error in CMakeLists.txt:
  Evaluation file to be written multiple times with different content.  This
  is generally caused by the content evaluating the configuration type,
  language, or location of object files:

Attached is my usage:

2023 08 22 @ 04 51 48PM@2x

I'm using CLion with Ninja Multi-Config.

Are there limitations I'm not aware of?

@hkaelber hkaelber mentioned this pull request Aug 27, 2023
Merged
@hkaelber
Copy link
Contributor Author

Hi, sorry if this is the wrong place. When trying this, I get an error in CMake:

CMake Error in CMakeLists.txt:
  Evaluation file to be written multiple times with different content.  This
  is generally caused by the content evaluating the configuration type,
  language, or location of object files:

Attached is my usage:

2023 08 22 @ 04 51 48PM@2x I'm using CLion with Ninja Multi-Config.

Are there limitations I'm not aware of?

Hi @riandrake, thx for spotting this.

Indeed currently multi-config generators are not supported with PRE_TEST discovery because I ignored them when porting this because -- of lazyness, as we don't have support for Ninja Multi-Config in our primay build-env and I could not easily test it. :-)

Could you maybe test #2739? Works for me so far, though I still miss testing all combinations. Thx

@riandrake
Copy link

@hkaelber this worked, thanks! I have a question though.

I noted that it didn't collect the tests until PRE_TEST time as expected, but this happened only once. My hope was that I could change the TEST_SPEC variable between runs and have it collect the tests again for targeted testing.

Am I misunderstanding how PRE_TEST works? If so, could I feature request this additional functionality somewhere?

@hkaelber
Copy link
Contributor Author

@hkaelber this worked, thanks! I have a question though.

I noted that it didn't collect the tests until PRE_TEST time as expected, but this happened only once. My hope was that I could change the TEST_SPEC variable between runs and have it collect the tests again for targeted testing.

Am I misunderstanding how PRE_TEST works? If so, could I feature request this additional functionality somewhere?

Not sure I understood your point, @riandrake.

I've never used multi-config so far, but to my understanding

  1. at configure/cmake time the config-specifc test includes are generated (containing the postponed discovery command) together with the main include file, that simply pulls in the config-specific test-include
  2. at every ctest invocation (per concrete config, Debug, Release, etc) the actual config-specific test file should be generated and executed

Correct? (Though I'm 100% sure how ctest is supposed to be executed in a multi-config env.)
That means that the TEST_SPEC is already fixed in 1. for all configs.

@riandrake
Copy link

riandrake commented Sep 10, 2023 via email

@bilderbuchi bilderbuchi mentioned this pull request Nov 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@horenmar horenmar horenmar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add DISCOVERY_MODE to catch_discover_tests
3 participants
@hkaelber @horenmar @riandrake