-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add code coverage presubmit test #11942
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
base: main
Are you sure you want to change the base?
Changes from all commits
c01885c
405bb67
6a60b80
c573a96
144f4f3
e77c789
146ab9b
56d8762
fcdfb2c
6cc606c
25e6ba9
2e29096
e79fbe2
087bb0c
f77987b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| tasks: | ||
| - name: prepare tool | ||
| script: .ci/scripts/prepare_tool.sh | ||
| infra_step: true | ||
| - name: code coverage check | ||
| script: .ci/scripts/tool_runner.sh | ||
| args: ["coverage-check", "--base-branch=origin/main"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # A list of packages and their minimum required code coverage. | ||
| # Only packages listed in this file will be checked for code coverage | ||
| # when running the script/tool/lib/src/coverage_check_command.dart | ||
| # command. If a package drops below its listed threshold, the check | ||
| # will fail. | ||
| custom_coverage_minimums: | ||
| camera_android_camerax: 18.3 |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does the coverage command work with the dart code defined in camera_android_camerax/.agents/skills/check-readiness/
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so how does it interact with https://github.com/flutter/packages/tree/main/packages/camera/camera_android_camerax/.agents/skills/check-readiness/test? longer term we probably want to be able to enforce coverage everywhere the agent is authoring new features.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah this doesn't test coverage of that as-written because it only runs
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An issue is fine thanks for your work here. Want I didnt want, was for us to not understand the interaction. Solving code coverage for dart skill packages can be out of scope for this pull request.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood! I'm glad you ask because I didn't consider it. Filed flutter/flutter#188880 |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,173 @@ | ||||||||||||||||||||||||
| // Copyright 2013 The Flutter Authors | ||||||||||||||||||||||||
| // Use of this source code is governed by a BSD-style license that can be | ||||||||||||||||||||||||
| // found in the LICENSE file. | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import 'dart:io' as io; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import 'package:file/file.dart'; | ||||||||||||||||||||||||
| import 'package:meta/meta.dart'; | ||||||||||||||||||||||||
| import 'package:yaml/yaml.dart'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import 'common/core.dart'; | ||||||||||||||||||||||||
| import 'common/output_utils.dart'; | ||||||||||||||||||||||||
| import 'common/package_looping_command.dart'; | ||||||||||||||||||||||||
| import 'common/repository_package.dart'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// A command to run code coverage checks on changed packages. | ||||||||||||||||||||||||
| class CoverageCheckCommand extends PackageLoopingCommand { | ||||||||||||||||||||||||
| /// Creates a coverage check command instance. | ||||||||||||||||||||||||
| CoverageCheckCommand(super.packagesDir, {super.processRunner, super.platform, super.gitDir}); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @override | ||||||||||||||||||||||||
| final String name = 'coverage-check'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @override | ||||||||||||||||||||||||
| final String description = | ||||||||||||||||||||||||
| 'Checks that code coverage meets the specified minimums ' | ||||||||||||||||||||||||
|
camsim99 marked this conversation as resolved.
|
||||||||||||||||||||||||
| 'for opted-in packages as specified in `script/configs/custom_coverage_minimums.yaml`.'; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // TODO(camsim99): Determine how to ensure test/ subdirectories are covered by this check: | ||||||||||||||||||||||||
| // https://github.com/flutter/flutter/issues/188880. | ||||||||||||||||||||||||
| @override | ||||||||||||||||||||||||
| PackageLoopingType get packageLoopingType => PackageLoopingType.includeAllSubpackages; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| final Map<String, double> _customMinimums = <String, double>{}; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @override | ||||||||||||||||||||||||
| Future<void> initializeRun() async { | ||||||||||||||||||||||||
| final File minimumsFile = packagesDir.parent | ||||||||||||||||||||||||
| .childDirectory('script') | ||||||||||||||||||||||||
| .childDirectory('configs') | ||||||||||||||||||||||||
| .childFile('custom_coverage_minimums.yaml'); | ||||||||||||||||||||||||
| if (!minimumsFile.existsSync()) { | ||||||||||||||||||||||||
| printError('The custom_coverage_minimums.yaml file is missing.'); | ||||||||||||||||||||||||
| throw ToolExit(1); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| final Object? yaml = loadYaml(minimumsFile.readAsStringSync()); | ||||||||||||||||||||||||
| if (yaml is YamlMap) { | ||||||||||||||||||||||||
| final Object? packageMap = yaml['custom_coverage_minimums']; | ||||||||||||||||||||||||
| if (packageMap is YamlMap) { | ||||||||||||||||||||||||
| for (final MapEntry<dynamic, dynamic> entry in packageMap.entries) { | ||||||||||||||||||||||||
| final Object? key = entry.key; | ||||||||||||||||||||||||
| final Object? value = entry.value; | ||||||||||||||||||||||||
| if (key is String && value is num) { | ||||||||||||||||||||||||
| _customMinimums[key] = value.toDouble(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @override | ||||||||||||||||||||||||
| Future<PackageResult> runForPackage(RepositoryPackage package) async { | ||||||||||||||||||||||||
| // Only run for first-party packages. | ||||||||||||||||||||||||
| final List<String> pathComponents = package.directory.fileSystem.path.split(package.path); | ||||||||||||||||||||||||
| if (pathComponents.contains('third_party')) { | ||||||||||||||||||||||||
| return PackageResult.skip('Not a first-party package.'); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!package.testDirectory.existsSync()) { | ||||||||||||||||||||||||
| return PackageResult.skip('No test/ directory.'); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| final String packageName = package.directory.basename; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!_customMinimums.containsKey(packageName)) { | ||||||||||||||||||||||||
| return PackageResult.skip('Package not opted into coverage checks.'); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Collect code coverage for the package. | ||||||||||||||||||||||||
| double? currentCoverage; | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| currentCoverage = await _runCoverageAndParse(package); | ||||||||||||||||||||||||
| } on NoLinesFoundException { | ||||||||||||||||||||||||
| return PackageResult.fail(<String>[ | ||||||||||||||||||||||||
| 'No instrumented Dart lines were found in the coverage report for $packageName.', | ||||||||||||||||||||||||
| 'If this package contains no code to cover, please remove it from ', | ||||||||||||||||||||||||
| 'script/configs/custom_coverage_minimums.yaml.', | ||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (currentCoverage == null) { | ||||||||||||||||||||||||
| return PackageResult.fail(<String>['Failed to run tests or parse coverage']); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| final double requiredCoverage = _customMinimums[packageName]!; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (currentCoverage < requiredCoverage) { | ||||||||||||||||||||||||
| return PackageResult.fail(<String>[ | ||||||||||||||||||||||||
| 'Code coverage for $packageName is ${currentCoverage.toStringAsFixed(1)}%, which is below the required ${requiredCoverage.toStringAsFixed(1)}%.', | ||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return PackageResult.success(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| Future<double?> _runCoverageAndParse(RepositoryPackage package) async { | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no change needed: It is kind of annoying that we have to write this parse code and there is not a built in way to enforce this on the cli. |
||||||||||||||||||||||||
| final Directory coverageDir = package.directory.childDirectory('coverage'); | ||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| final io.ProcessResult result = await processRunner.run('flutter', <String>[ | ||||||||||||||||||||||||
| 'test', | ||||||||||||||||||||||||
| '--coverage', | ||||||||||||||||||||||||
| ], workingDir: package.directory); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (result.exitCode != 0) { | ||||||||||||||||||||||||
| print('Test failed for ${package.directory.basename}:\n${result.stdout}\n${result.stderr}'); | ||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| final File lcovFile = coverageDir.childFile('lcov.info'); | ||||||||||||||||||||||||
| if (!lcovFile.existsSync()) { | ||||||||||||||||||||||||
| print('Coverage file not found at ${lcovFile.path}.'); | ||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return calculateCoverage(lcovFile); | ||||||||||||||||||||||||
| } finally { | ||||||||||||||||||||||||
| if (coverageDir.existsSync()) { | ||||||||||||||||||||||||
| coverageDir.deleteSync(recursive: true); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Calculates code coverage for non-generated code files by finding | ||||||||||||||||||||||||
| /// the percentage of covered lines of code over the total lines of code. | ||||||||||||||||||||||||
| @visibleForTesting | ||||||||||||||||||||||||
| double calculateCoverage(File lcovFile) { | ||||||||||||||||||||||||
| var linesHit = 0; | ||||||||||||||||||||||||
| var linesFound = 0; | ||||||||||||||||||||||||
| var skipCurrentFile = false; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| final List<String> lines = lcovFile.readAsLinesSync(); | ||||||||||||||||||||||||
| for (final line in lines) { | ||||||||||||||||||||||||
| if (line.startsWith('SF:')) { | ||||||||||||||||||||||||
| final String fileName = line.substring(3); | ||||||||||||||||||||||||
| skipCurrentFile = _isGeneratedFile(fileName); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| if (!skipCurrentFile) { | ||||||||||||||||||||||||
| if (line.startsWith('LH:')) { | ||||||||||||||||||||||||
| linesHit += int.parse(line.substring(3)); | ||||||||||||||||||||||||
| } else if (line.startsWith('LF:')) { | ||||||||||||||||||||||||
| linesFound += int.parse(line.substring(3)); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (linesFound == 0) { | ||||||||||||||||||||||||
| throw const NoLinesFoundException(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| return (linesHit / linesFound) * 100.0; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Exception thrown when no instrumented lines are found in the coverage report. | ||||||||||||||||||||||||
| class NoLinesFoundException implements Exception { | ||||||||||||||||||||||||
| /// Creates a new [NoLinesFoundException]. | ||||||||||||||||||||||||
| const NoLinesFoundException(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| bool _isGeneratedFile(String fileName) { | ||||||||||||||||||||||||
|
camsim99 marked this conversation as resolved.
|
||||||||||||||||||||||||
| return fileName.endsWith('.g.dart') || | ||||||||||||||||||||||||
| fileName.endsWith('.pb.dart') || | ||||||||||||||||||||||||
| fileName.endsWith('.mocks.dart'); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+169
to
+173
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same answer for addressing #11942 (comment) |
||||||||||||||||||||||||
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.
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.
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.
Removed camera_android for now since it's beyond the scope of our project anyway.