Open
Conversation
…ting - BLOCKER: Replace path.relative() with executableTargets lookup to map test program paths to actual CMake target names. The previous approach produced file paths (e.g. 'tests/my_test') instead of target names (e.g. 'my_test'), causing cmake --build --target to fail. - BLOCKER: Fix accmulatedTestList.concat() no-op by using push(...) so tests are properly marked as errored on build failure. - MAJOR: Check return value of recursive getTestTargets() call so child failures propagate correctly to the caller. - MINOR: Add guard for empty testProgram() return to prevent nonsensical build targets from empty strings. - MINOR: Fix typo accmulatedTestList -> accumulatedTestList. - MINOR: Remove extra whitespace in for statement and redundant .toString() call. - NIT: Remove trailing semicolon after for...of block. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…or reporting" This reverts commit d548f90.
Contributor
There was a problem hiding this comment.
Pull request overview
Updates CTest Explorer “build before run” behavior so running/debugging selected tests builds their required executables instead of defaulting to the ALL target, improving usability for partial builds and excluded-from-all tests.
Changes:
- Add logic in
CTestDriverto collect test executables perCMakeProjectand invoke targeted builds. - Map test executable paths back to CMake target names using
project.executableTargets(with a filename-based fallback). - Add a CHANGELOG entry describing the new build-before-test behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
src/ctest.ts |
Adds target discovery and per-project targeted build step before running/debugging tests. |
CHANGELOG.md |
Documents the user-visible behavior change for build-before-run in the test workflow. |
Comment on lines
+1939
to
+1945
| const buildResult = await project.build(accumulatedTargets, false, false); | ||
| if (buildResult.exitCode !== 0) { | ||
| success = false; | ||
| } | ||
| } catch (e) { | ||
| success = false; | ||
| } |
There was a problem hiding this comment.
The catch (e) here suppresses the underlying build error details. Please log the error (or include relevant information such as the target list and exit code) so users can diagnose why build-before-test failed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…-cmake-tools into building-tests2
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This file contains hidden or 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
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.
Original PR: #4523
Addresses the following issue: #4515
This PR
buildTestTargets()to correctly compute target name