Conversation
|
@codex review /gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors how the package:test version is obtained, switching from spawning a dart test --version process to synchronously reading the version from the .dart_tool/package_graph.json file. This is an excellent performance improvement that avoids costly process creation, especially in large workspaces. The change has been propagated correctly, leading to a nice simplification of the codebase by converting several asynchronous functions and call sites to synchronous ones. The new implementation includes appropriate error handling for file I/O and JSON parsing. Overall, the changes are well-implemented and improve both performance and code clarity.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5983 +/- ##
==========================================
- Coverage 67.52% 67.51% -0.01%
==========================================
Files 169 169
Lines 12962 12948 -14
Branches 2565 2565
==========================================
- Hits 8752 8742 -10
+ Misses 3751 3748 -3
+ Partials 459 458 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…of running `dart test --version` We currently spawn `dart run test:test --version` for each package in the workspace to get the test capabilities during test discovery (to know if we can add profiles like Coverage to a given suite). This can be expensive (in terms of CPU and RAM) if the project is large. For example opening the Dart SDK root on my machine and then clicking the Test Explorer icon results in all RAM being consumed. We already filter out packages in the same Pub Workspace, but there are lots of non-Workspace packages in there. With this change, we just read the version from the `.dart_tool/package_graph.json` file instead which is significantly faster and uses little resources. This also resulted in quite a bit of async code becoming sync. Fixes #5936
7adbb63 to
503fed7
Compare
We currently spawn
dart run test:test --versionfor each package in the workspace to get the test capabilities during test discovery (to know if we can add profiles like Coverage to a given suite). This can be expensive (in terms of CPU and RAM) if the project is large.For example opening the Dart SDK root on my machine and then clicking the Test Explorer icon results in all RAM being consumed. We already filter out packages in the same Pub Workspace, but there are lots of non-Workspace packages in there.
With this change, we just read the version from the
.dart_tool/package_graph.jsonfile instead which is significantly faster and uses little resources.This also resulted in quite a bit of async code becoming sync.
Fixes #5936