Rework CMake test discovery #1673
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thetic
commented
Nov 12, 2022
| @@ -4,6 +4,10 @@ set(_DISCOVER_SCRIPT "${CMAKE_CURRENT_LIST_DIR}/../Scripts/CppUTestBuildTimeDisc | |||
|
|
|||
| # Create target to discover tests | |||
| function (cpputest_buildtime_discover_tests tgt) | |||
There was a problem hiding this comment.
I chose to create a new module rather than refactoring the existing one. Their behavior is not the same. When using cpputest_buildtime_discover_tests with a multi-config project, ctest runs tests regardless of whether and which config it is passed. When using cpputest_discover_tests with such a project, ctest requires a config to be passed. This is the more "normal" behavior that mimics the native add_test command.
I don't know the workflow for breaking changes, but this should be deleted eventually, ideally prior to the next major release.
thetic
force-pushed
the
discovery-zone
branch
from
f4ed527
to
74df43e
Compare
thetic
commented
Nov 12, 2022
| @@ -1,10 +0,0 @@ | |||
| # Override output properties to put test executable at specificied location | |||
There was a problem hiding this comment.
This created collisions between configs by using the same output path for all of them:
cmake --preset MSVC
cmake --build cpputest_build --config Release
cmake --build cpputest_build --config Debug # Overwrites Release outputs
ctest --test-dir cpputest_build -C Release # Actually runs Debug tests
Merged
|
Uhm, the PR has a conflict |
This is broken for multi-configuration generators. Debug and release builds, for example, need to output to different paths and run tests against those paths. The current implementation would overwrite one config's build with another and test that one, regardless of the config the user is trying to test.
thetic
force-pushed
the
discovery-zone
branch
from
74df43e
to
c781ff8
Compare
thetic
force-pushed
the
discovery-zone
branch
from
c781ff8
to
73fa9fb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing implementation does not handle multi-config setups (generators that can build Debug, Release, etc. from the same project like Visual Studio).
Consider the following example. No tests should run because Debug hasn't been built. The current discovery automation, however, just runs whatever was built last.
These changes eliminate the need for the "path normalization" that prevents outputs from multiple configs from coexisting.