Skip to content

Catch.cmake: Support CMake multi-config with PRE_TEST discovery mode#2739

Merged
horenmar merged 2 commits intocatchorg:develfrom
hkaelber:hk/support-cmake-multi
Sep 25, 2023
Merged

Catch.cmake: Support CMake multi-config with PRE_TEST discovery mode#2739
horenmar merged 2 commits intocatchorg:develfrom
hkaelber:hk/support-cmake-multi

Conversation

@hkaelber
Copy link
Copy Markdown
Contributor

Description

Support CMake multi-config generators with PRE_TEST discovery mode.
Cf. #2670 (comment)

GitHub Issues

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 27, 2023

Codecov Report

Merging #2739 (9b930ec) into devel (85eb465) will increase coverage by 0.03%.
Report is 15 commits behind head on devel.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            devel    #2739      +/-   ##
==========================================
+ Coverage   91.32%   91.36%   +0.03%     
==========================================
  Files         192      190       -2     
  Lines        7850     7855       +5     
==========================================
+ Hits         7169     7176       +7     
+ Misses        681      679       -2     

@horenmar
Copy link
Copy Markdown
Member

How far along is this?

@horenmar horenmar added the Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration. label Sep 23, 2023
@hkaelber hkaelber changed the title [WIP]Catch.cmake: Support CMake multi-config with PRE_TEST discovery mode Catch.cmake: Support CMake multi-config with PRE_TEST discovery mode Sep 24, 2023
@hkaelber hkaelber marked this pull request as ready for review September 24, 2023 08:37
@hkaelber
Copy link
Copy Markdown
Contributor Author

How far along is this?

@horenmar, IMHO it's ready for review. I was hesitating, because of missing experience with the usage of multi-config targets. After re-reading the docs, made it finally work correctly. Could you pls review?

Copy link
Copy Markdown
Member

@horenmar horenmar left a comment

Choose a reason for hiding this comment

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

I tested this with MSVC and it seems to work.

@hkaelber hkaelber force-pushed the hk/support-cmake-multi branch from 630c7f0 to 9b930ec Compare September 25, 2023 14:32
Comment on lines +267 to +271
"if(NOT CTEST_CONFIGURATION_TYPE)" "\n"
" message(\"No configuration for testing specified, use '-C <cfg>'.\")" "\n"
"else()" "\n"
" include(\"${ctest_file_base}_include-\${CTEST_CONFIGURATION_TYPE}.cmake\")" "\n"
"endif()" "\n"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is fine to merge already, but I still have a question: is there a reason to do this like this (outside of consistency with the surrounding code), rather than either making the newlines part of the line-strings, or using multiline strings?

IIRC something like this would work as well and be simpler to read

set(ctest_include_multi_content [[
if(NOT CTEST_CONFIGURATION_TYPE)
  message("No configuration for testing specified, use '-C <cfg>'.")
else()
  include("C:/ubuntu/temp/discover-tests-tests/msvc-sln-1/tests-b12d07c_include-${CTEST_CONFIGURATION_TYPE}.cmake")
endif()
]])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, the issue is that we want to interpret some of the variables, while [[]] does not do any interpretation. nevermind then

@horenmar horenmar merged commit 7b79331 into catchorg:devel Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants