Add progress notification + cancel button for multi-debug-session test runs #5842
Conversation
|
@codex review /gemini review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review
This pull request introduces a progress notification and cancellation support for multi-session test runs, which is a valuable improvement for user experience. The implementation correctly uses vs.window.withProgress and propagates the cancellation token. However, I've identified a critical bug in the logic for handling single-session test runs that would lead to a crash. My review includes a specific code suggestion to fix this issue. The rest of the changes for enabling cancellation appear correct.
There was a problem hiding this comment.
Pull request overview
This PR enhances the test execution experience for multi-debug-session test runs by adding a progress notification with a cancel button, making it clearer when tests span multiple sessions and allowing users to cancel the entire run instead of just individual debug sessions.
Key Changes:
- Added progress notification UI with cancellation support for multi-suite test runs
- Updated test command handlers to accept and propagate cancellation tokens
- Restructured test execution flow to distinguish between single and multi-suite scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/extension/test/vs_test_controller.ts | Refactored test execution to show progress notifications for multi-suite runs with cancellation support; split single vs. multiple suite execution paths |
| src/extension/commands/test.ts | Updated command registrations to accept cancellation token parameter and pass it through to test execution logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const totalItems = testsBySuite.size; | ||
|
|
||
| if (totalItems > 1) { | ||
| const title = debug ? "Debugging" : "Running"; |
There was a problem hiding this comment.
The progress notification title is too generic ("Debugging" or "Running"). Consider making it more descriptive to clarify what is being run, such as "Running Tests" or "Debugging Tests", to provide better context to users about what operation is in progress.
| const title = debug ? "Debugging" : "Running"; | |
| const title = debug ? "Debugging Tests" : "Running Tests"; |
This makes it clearer when a test run is spanning multiple sessions and makes it possible to cancel the entire run (because we can't detect cancelling a single debug session, so doing so will continue with the next). Fixes #5833
23b1612 to
d0d351e
Compare
These happen quite a lot, particularly during startup. We only need them in the log, we don't need them to show in _all_ logs (as warnings/errors do).
This makes it clearer when a test run is spanning multiple sessions and makes it possible to cancel the entire run (because we can't detect cancelling a single debug session, so doing so will continue with the next).
Fixes #5833