-
Notifications
You must be signed in to change notification settings - Fork 340
Add support for collecting coverage from dependencies that exist in the workspace #5631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…he workspace Fixes #5623
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for collecting code coverage from workspace dependencies when running Flutter tests. It modifies the test runner to include coverage data from packages that exist in the workspace, not just the current package being tested.
Key changes:
- Extends the test launch configuration to accept workspace package names for coverage collection
- Adds new test files demonstrating the dependency coverage functionality
- Updates the test controller to expose the coverage parser for accessing collected coverage data
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/test/test_projects/flutter_hello_world/lib/printer.dart |
Adds a simple print function to serve as a dependency for coverage testing |
src/test/test_projects/flutter_hello_world/example/test/printer_test.dart |
Test file that imports and tests the printer function from the parent package |
src/test/test_projects/flutter_hello_world/example/pubspec.yaml |
Configuration for the example package with dependency on the parent package |
src/test/test_projects/flutter_hello_world/example/lib/printer.dart |
Wrapper function that calls the parent package's printer function |
src/test/helpers.ts |
Adds file path constants for the new test project files |
src/test/flutter_test_debug/debug/flutter_test.test.ts |
Adds test coverage verification for dependency packages |
src/shared/vscode/interfaces.ts |
Exposes the coverage parser in the test controller API |
src/shared/utils/test.ts |
Extends launch configuration to include workspace package names for coverage |
src/extension/commands/test.ts |
Implements workspace package discovery for coverage collection |
| let workspacePackageNames: string[] | undefined; | ||
| if (includeCoverage && isFlutter) { | ||
| const workspacePackagePaths = await getAllProjectFolders(this.logger, getExcludedFolders, { requirePubspec: true, searchDepth: config.projectSearchDepth }); | ||
| workspacePackageNames = workspacePackagePaths.map((packagePath) => path.basename(packagePath)); |
Copilot
AI
Aug 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using path.basename() to extract package names from paths may not be reliable. Consider reading the package name from each package's pubspec.yaml file instead, as the directory name might not always match the actual package name declared in pubspec.yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for collecting test coverage from workspace dependencies in Flutter projects. The changes correctly identify workspace packages and pass them to the flutter test command using the --coverage-package flag. The implementation is accompanied by good test coverage, including a new test case that verifies coverage is collected from a local dependency.
My review includes a couple of suggestions to improve code conciseness and reduce duplication in the tests. Overall, the changes are solid and address the intended issue effectively.
| for (const packageName of workspacePackageNames) { | ||
| toolArgs.push("--coverage-package"); | ||
| toolArgs.push(packageName); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| describe("collects coverage", () => { | ||
| it("for a basic test", async () => { | ||
| // Discover tests. | ||
| await openFile(flutterTestMainFile); | ||
| await waitForResult(() => !!privateApi.fileTracker.getOutlineFor(flutterTestMainFile)); | ||
| const controller = privateApi.testController; | ||
|
|
||
| const suiteNode = controller.controller.items.get(`SUITE:${fsPath(flutterTestMainFile)}`)!; | ||
| const testRequest = new vs.TestRunRequest([suiteNode]); | ||
|
|
||
| const createTestRunOriginal = controller.controller.createTestRun; | ||
| const createTestRunStub = sb.stub(controller.controller, "createTestRun"); | ||
| let addCoverageStub: SinonStub | undefined; | ||
| createTestRunStub.callsFake((request) => { | ||
| const originalResult = createTestRunOriginal.call(controller.controller, request); | ||
| addCoverageStub = sb.stub(originalResult, "addCoverage").returns(null); | ||
| return originalResult; | ||
| }); | ||
| await controller.runTests(false, true, testRequest, fakeCancellationToken); | ||
|
|
||
| assert(addCoverageStub?.calledOnce); | ||
| const coverage = addCoverageStub.firstCall.args[0] as DartFileCoverage; | ||
| assert.equal(fsPath(coverage.uri), fsPath(flutterHelloWorldMainFile)); // App file, not test file. | ||
| assert.ok(coverage.statementCoverage.covered > 0); | ||
| assert.ok(coverage.statementCoverage.total > 0); | ||
| }); | ||
| await controller.runTests(false, true, testRequest, fakeCancellationToken); | ||
|
|
||
| assert(addCoverageStub?.calledOnce); | ||
| const coverage = addCoverageStub.firstCall.args[0] as DartFileCoverage; | ||
| assert.equal(fsPath(coverage.uri), fsPath(flutterHelloWorldMainFile)); // App file, not test file. | ||
| assert.ok(coverage.statementCoverage.covered > 0); | ||
| assert.ok(coverage.statementCoverage.total > 0); | ||
| it("and includes dependencies", async () => { | ||
| // Discover tests. | ||
| await openFile(flutterHelloWorldExampleTestFile); | ||
| await waitForResult(() => !!privateApi.fileTracker.getOutlineFor(flutterHelloWorldExampleTestFile)); | ||
|
|
||
| const controller = privateApi.testController; | ||
| const suiteNode = controller.controller.items.get(`SUITE:${fsPath(flutterHelloWorldExampleTestFile)}`)!; | ||
| const testRequest = new vs.TestRunRequest([suiteNode]); | ||
|
|
||
| const createTestRunOriginal = controller.controller.createTestRun; | ||
| const createTestRunStub = sb.stub(controller.controller, "createTestRun"); | ||
| let addCoverageStub: SinonStub | undefined; | ||
| createTestRunStub.callsFake((request) => { | ||
| const originalResult = createTestRunOriginal.call(controller.controller, request); | ||
| addCoverageStub = sb.stub(originalResult, "addCoverage").returns(null); | ||
| return originalResult; | ||
| }); | ||
| await controller.runTests(false, true, testRequest, fakeCancellationToken); | ||
|
|
||
| assert(addCoverageStub?.called); | ||
| const coverageFiles = addCoverageStub.getCalls().map((call) => fsPath((call.args[0] as DartFileCoverage).uri)); | ||
| assert.deepStrictEqual(coverageFiles, [fsPath(flutterHelloWorldExamplePrinterFile), fsPath(flutterHelloWorldPrinterFile)]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some duplicated test setup code in this describe block. You can extract the common logic for stubbing createTestRun and addCoverage into a beforeEach hook to reduce duplication and improve maintainability.
describe("collects coverage", () => {
let addCoverageStub: SinonStub | undefined;
beforeEach(() => {
const controller = privateApi.testController;
const createTestRunOriginal = controller.controller.createTestRun;
const createTestRunStub = sb.stub(controller.controller, "createTestRun");
createTestRunStub.callsFake((request) => {
const originalResult = createTestRunOriginal.call(controller.controller, request);
addCoverageStub = sb.stub(originalResult, "addCoverage").returns(null);
return originalResult;
});
});
it("for a basic test", async () => {
// Discover tests.
await openFile(flutterTestMainFile);
await waitForResult(() => !!privateApi.fileTracker.getOutlineFor(flutterTestMainFile));
const controller = privateApi.testController;
const suiteNode = controller.controller.items.get(`SUITE:${fsPath(flutterTestMainFile)}`)!;
const testRequest = new vs.TestRunRequest([suiteNode]);
await controller.runTests(false, true, testRequest, fakeCancellationToken);
assert(addCoverageStub?.calledOnce);
const coverage = addCoverageStub!.firstCall.args[0] as DartFileCoverage;
assert.equal(fsPath(coverage.uri), fsPath(flutterHelloWorldMainFile)); // App file, not test file.
assert.ok(coverage.statementCoverage.covered > 0);
assert.ok(coverage.statementCoverage.total > 0);
});
it("and includes dependencies", async () => {
// Discover tests.
await openFile(flutterHelloWorldExampleTestFile);
await waitForResult(() => !!privateApi.fileTracker.getOutlineFor(flutterHelloWorldExampleTestFile));
const controller = privateApi.testController;
const suiteNode = controller.controller.items.get(`SUITE:${fsPath(flutterHelloWorldExampleTestFile)}`)!;
const testRequest = new vs.TestRunRequest([suiteNode]);
await controller.runTests(false, true, testRequest, fakeCancellationToken);
assert(addCoverageStub?.called);
const coverageFiles = addCoverageStub!.getCalls().map((call) => fsPath((call.args[0] as DartFileCoverage).uri));
assert.deepStrictEqual(coverageFiles, [fsPath(flutterHelloWorldExamplePrinterFile), fsPath(flutterHelloWorldPrinterFile)]);
});
});
Fixes #5623