Add code coverage presubmit test#11942
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new coverage-check command to the repository tooling, along with CI configuration and a YAML file to define minimum code coverage thresholds for opted-in packages. The command runs tests with coverage, parses the resulting lcov.info file while ignoring generated files, and verifies that the coverage meets the specified minimums. Feedback recommends implementing more defensive YAML parsing to prevent runtime crashes on malformed configuration files, and preserving the coverage directory on failure to assist developers with debugging.
| final Directory coverageDir = package.directory.childDirectory('coverage'); | ||
| if (coverageDir.existsSync()) { | ||
| coverageDir.deleteSync(recursive: true); | ||
| } | ||
|
|
||
| if (errors.isNotEmpty) { | ||
| return PackageResult.fail(errors); | ||
| } | ||
|
|
||
| return PackageResult.success(); |
There was a problem hiding this comment.
Currently, the coverage directory is deleted even when the coverage check fails. This prevents developers from inspecting the generated lcov.info file or generating an HTML report to see which lines/files missed coverage.
Swapping the deletion logic to only run on success improves the developer experience significantly by leaving the coverage artifacts intact for debugging when a check fails.
| final Directory coverageDir = package.directory.childDirectory('coverage'); | |
| if (coverageDir.existsSync()) { | |
| coverageDir.deleteSync(recursive: true); | |
| } | |
| if (errors.isNotEmpty) { | |
| return PackageResult.fail(errors); | |
| } | |
| return PackageResult.success(); | |
| if (errors.isNotEmpty) { | |
| return PackageResult.fail(errors); | |
| } | |
| final Directory coverageDir = package.directory.childDirectory('coverage'); | |
| if (coverageDir.existsSync()) { | |
| coverageDir.deleteSync(recursive: true); | |
| } | |
| return PackageResult.success(); |
There was a problem hiding this comment.
I think the test is easily reproduceable locally so not sure we need this.
camsim99
changed the title
There was a problem hiding this comment.
Code Review
This pull request introduces a code coverage check command (coverage-check) to the tooling, along with CI configuration and tests. The command runs tests with coverage, parses the resulting lcov.info file while ignoring certain generated files, and verifies that the coverage meets specified minimums for opted-in packages. Feedback recommends avoiding the deletion of the coverage directory to assist with local debugging, ignoring .freezed.dart files during coverage calculations, and adding a corresponding test case for this file type.
| bool _isGeneratedFile(String fileName) { | ||
| return fileName.endsWith('.g.dart') || | ||
| fileName.endsWith('.pb.dart') || | ||
| fileName.endsWith('.mocks.dart'); | ||
| } |
There was a problem hiding this comment.
Consider adding .freezed.dart to the list of ignored generated files, as Freezed is a very common code generation package in Flutter/Dart projects and its generated files should not be included in coverage calculations.
| bool _isGeneratedFile(String fileName) { | |
| return fileName.endsWith('.g.dart') || | |
| fileName.endsWith('.pb.dart') || | |
| fileName.endsWith('.mocks.dart'); | |
| } | |
| bool _isGeneratedFile(String fileName) { | |
| return fileName.endsWith('.g.dart') || | |
| fileName.endsWith('.pb.dart') || | |
| fileName.endsWith('.mocks.dart') || | |
| fileName.endsWith('.freezed.dart'); | |
| } |
There was a problem hiding this comment.
I see no usages of https://pub.dev/packages/freezed but if reviewers think I should protect against this, I can go ahead and add it.
There was a problem hiding this comment.
Same answer for addressing #11942 (comment)
1 participant
Adds a presubmit test that ensures that the Dart code coverage as evaluated by
flutter test --coveragedoes not fall below a predetermined minimum.Currently, implements minimums ONLY for
camera_androidandcamera_android_cameraxas their current code coverage. The reason I pursued this approach of hardcoding is because a common minimum across packages would be hard to enforce harshly depending on how the package is written. For example,camera_android_cameraxwraps native class with Dart, which I believe contributes to its much lower code coverage score. If reviewers are ok to this approach, open to including all packages as their current minimum.Intended to help agents ensure that while they are making changes, they maintain if not improve the code coverage of the packages they modify.
Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2