Add code coverage presubmit test#11942
Conversation
|
/gemini review |
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.
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)
| # command. If a package drops below its listed threshold, the check | ||
| # will fail. | ||
| custom_coverage_minimums: | ||
| camera_android: 30.0 |
There was a problem hiding this comment.
omg this is so very low.
| .childDirectory('script') | ||
| .childDirectory('configs') | ||
| .childFile('custom_coverage_minimums.yaml'); | ||
| if (minimumsFile.existsSync()) { |
There was a problem hiding this comment.
Should we log that we are skipping coverage checks when the file does not exist? Or should we configure this command to fail if coverage checks are run and the file is missing then build in a yaml file that opts in camera_android and camera_android_camerax into this check?
Edit: I see below that you are using _customMinimums. My gut says that instead of building the list from where the files exist we should go with the pattern of having a yaml config file that opts our 2 packages in and then fails if the custom_coverage_minimums configs do not exist where we expect.
There was a problem hiding this comment.
This file is making me rethink if the pre push skills should also support camera_android. That said we can add that functionality later if we want.
|
|
||
| @override | ||
| final String description = | ||
| 'Checks that code coverage meets the specified minimums ' |
There was a problem hiding this comment.
Consider including where someone can find the specified minimums.
| } | ||
| final double requiredCoverage = _customMinimums[packageName]!; | ||
|
|
||
| // Delete generated lcov.info. |
There was a problem hiding this comment.
Move this code. See comment https://github.com/flutter/packages/pull/11942/changes#r3467934044
| return null; | ||
| } | ||
|
|
||
| return _calculateCoverage(lcovFile); |
There was a problem hiding this comment.
| return _calculateCoverage(lcovFile); | |
| calculatedCoverage = _calculateCoverage(lcovFile); | |
| // Delete generated lcov.info. | |
| final Directory coverageDir = package.directory.childDirectory('coverage'); | |
| if (coverageDir.existsSync()) { | |
| coverageDir.deleteSync(recursive: true); | |
| } | |
| return calculatedCoverage; |
| final io.ProcessResult result = await processRunner.run( | ||
| 'flutter', | ||
| <String>[ | ||
| 'test', |
There was a problem hiding this comment.
Non blocking nit. This means we are running all the tests twice. I dont want to block this pr on figuring out that efficiency unless you think we should but it should block adoption more broadly for the packages repo. If we did this efficiently the logic for opt-in package would live in the regular test command.
There was a problem hiding this comment.
How does the coverage command work with the dart code defined in camera_android_camerax/.agents/skills/check-readiness/
|
|
||
| /// Calculates code coverage for non-generated code files by finding | ||
| /// the percentage of covered lines of code over the total lines of code. | ||
| double _calculateCoverage(File lcovFile) { |
There was a problem hiding this comment.
I did not double check that this calculateCoverage function works as expected but you might consider making it visibleForTesting and adding unit tests just for the parsing.
| } | ||
|
|
||
| if (linesFound == 0) { | ||
| return 100.0; |
There was a problem hiding this comment.
maybe this is correct but it feels weird to me to have lines found give 100% when there were no lines.
| } | ||
| } | ||
|
|
||
| bool _isGeneratedFile(String fileName) { |
There was a problem hiding this comment.
This seems reasonable. FWIW the lcov command also has the ability to do exclusions.
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