Don't perform test-suite-discovery when running specific tests#5891
Don't perform test-suite-discovery when running specific tests#5891
Conversation
DanTup
added
is bug fix
in testing
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5891 +/- ##
=======================================
Coverage 67.39% 67.40%
=======================================
Files 168 168
Lines 12907 12908 +1
Branches 2557 2558 +1
=======================================
+ Hits 8699 8700 +1
Misses 3754 3754
Partials 454 454 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@codex review /gemini review |
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly prevents test suite discovery when running specific tests, which improves performance. The changes are logical and well-tested. I've made one suggestion to improve the clarity of the condition for triggering test discovery. The associated refactoring and test additions are solid.
There was a problem hiding this comment.
Pull request overview
This PR fixes a performance issue where running specific tests would trigger slow discovery of all test suites. The fix ensures that full test-suite discovery only occurs when running all tests (i.e., when request.include is undefined), not when running specific tests.
Changes:
- Modified
runTeststo only perform suite discovery whenrequest.includeis undefined - Added tests to verify discovery behavior for both undefined and empty include arrays
- Updated interface to make
testDiscoverernon-optional and removed redundantdiscovererproperty fromtestController
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/extension/test/vs_test_controller.ts | Added conditional check to only run full suite discovery when request.include is undefined (Run All Tests scenario) |
| src/test/dart/test/test_discovery.test.ts | Added two new tests verifying discovery behavior and updated existing test calls to use testDiscoverer directly |
| src/shared/vscode/interfaces.ts | Made testDiscoverer non-optional and removed discoverer property from testController interface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba6a5eae45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
The change in c32340e to fix "Run All Tests" was also triggering full suite discovery for normal runs of tests. This could make the first test run have a fairly slow startup time.
Fixes #5879