-
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 6 commits
c01885c
405bb67
6a60b80
c573a96
144f4f3
e77c789
146ab9b
56d8762
fcdfb2c
6cc606c
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,6 @@ | ||
| # A list of packages and their minimum required code coverage. | ||
| # Only packages listed in this file will be checked for code coverage. | ||
| # If a package drops below its listed threshold, the CI check will fail. | ||
| custom_coverage_minimums: | ||
| camera_android: 30.0 | ||
|
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. omg this is so very low. |
||
| 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/ |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,150 @@ | ||||||||||||||||||||||||||||||||||||||||||
| // 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:yaml/yaml.dart'; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import 'common/package_looping_command.dart'; | ||||||||||||||||||||||||||||||||||||||||||
| import 'common/repository_package.dart'; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| /// A command to run coverage checks on changed packages. | ||||||||||||||||||||||||||||||||||||||||||
| class CoverageCheckCommand extends PackageLoopingCommand { | ||||||||||||||||||||||||||||||||||||||||||
| /// Creates an instance of the coverage check command. | ||||||||||||||||||||||||||||||||||||||||||
| 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 ' | ||||||||||||||||||||||||||||||||||||||||||
|
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 including where someone can find the specified minimums. |
||||||||||||||||||||||||||||||||||||||||||
| 'for opted-in packages.'; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| @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()) { | ||||||||||||||||||||||||||||||||||||||||||
|
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. 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 |
||||||||||||||||||||||||||||||||||||||||||
| final minimumsConfig = loadYaml(minimumsFile.readAsStringSync()) as YamlMap; | ||||||||||||||||||||||||||||||||||||||||||
| final packageMap = minimumsConfig['custom_coverage_minimums'] as YamlMap?; | ||||||||||||||||||||||||||||||||||||||||||
| if (packageMap != null) { | ||||||||||||||||||||||||||||||||||||||||||
| for (final MapEntry<dynamic, dynamic> entry in packageMap.entries) { | ||||||||||||||||||||||||||||||||||||||||||
| _customMinimums[entry.key as String] = (entry.value as num).toDouble(); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| @override | ||||||||||||||||||||||||||||||||||||||||||
| Future<PackageResult> runForPackage(RepositoryPackage package) async { | ||||||||||||||||||||||||||||||||||||||||||
| // Only run for first-party packages. | ||||||||||||||||||||||||||||||||||||||||||
| if (!package.directory.path.contains( | ||||||||||||||||||||||||||||||||||||||||||
| '${packagesDir.fileSystem.path.separator}packages${packagesDir.fileSystem.path.separator}', | ||||||||||||||||||||||||||||||||||||||||||
| ) && | ||||||||||||||||||||||||||||||||||||||||||
| !package.directory.path.endsWith('${packagesDir.fileSystem.path.separator}packages')) { | ||||||||||||||||||||||||||||||||||||||||||
| 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.'); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Run tests on current branch. | ||||||||||||||||||||||||||||||||||||||||||
| final double? currentCoverage = await _runCoverageAndParse(package); | ||||||||||||||||||||||||||||||||||||||||||
| if (currentCoverage == null) { | ||||||||||||||||||||||||||||||||||||||||||
| return PackageResult.fail(<String>['Failed to run tests or parse coverage on HEAD']); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| final double requiredCoverage = _customMinimums[packageName]!; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| final errors = <String>[]; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| if (currentCoverage < requiredCoverage) { | ||||||||||||||||||||||||||||||||||||||||||
| errors.add( | ||||||||||||||||||||||||||||||||||||||||||
| 'Code coverage for $packageName is ${currentCoverage.toStringAsFixed(1)}%, ' | ||||||||||||||||||||||||||||||||||||||||||
| 'which is below the required ${requiredCoverage.toStringAsFixed(1)}%.', | ||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| final Directory coverageDir = package.directory.childDirectory('coverage'); | ||||||||||||||||||||||||||||||||||||||||||
| if (coverageDir.existsSync()) { | ||||||||||||||||||||||||||||||||||||||||||
| coverageDir.deleteSync(recursive: true); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
camsim99 marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| if (errors.isNotEmpty) { | ||||||||||||||||||||||||||||||||||||||||||
| return PackageResult.fail(errors); | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| return PackageResult.success(); | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+80
to
+91
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. Currently, the 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.
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 think the test is easily reproduceable locally so not sure we need this. |
||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| 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 args = <String>['test', '--coverage']; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| final io.ProcessResult result = await processRunner.run( | ||||||||||||||||||||||||||||||||||||||||||
| 'flutter', | ||||||||||||||||||||||||||||||||||||||||||
| args, | ||||||||||||||||||||||||||||||||||||||||||
| 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 = package.directory.childDirectory('coverage').childFile('lcov.info'); | ||||||||||||||||||||||||||||||||||||||||||
| if (!lcovFile.existsSync()) { | ||||||||||||||||||||||||||||||||||||||||||
| print('Coverage file not found at ${lcovFile.path}.'); | ||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| return _calculateCoverage(lcovFile); | ||||||||||||||||||||||||||||||||||||||||||
|
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.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| double _calculateCoverage(File lcovFile) { | ||||||||||||||||||||||||||||||||||||||||||
|
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. 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. |
||||||||||||||||||||||||||||||||||||||||||
| 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); | ||||||||||||||||||||||||||||||||||||||||||
| // Skip checking coverage of generated files. | ||||||||||||||||||||||||||||||||||||||||||
| skipCurrentFile = | ||||||||||||||||||||||||||||||||||||||||||
| fileName.endsWith('.g.dart') || | ||||||||||||||||||||||||||||||||||||||||||
| fileName.endsWith('.pb.dart') || | ||||||||||||||||||||||||||||||||||||||||||
| fileName.endsWith('.pigeon.dart') || | ||||||||||||||||||||||||||||||||||||||||||
| fileName.endsWith('.mocks.dart') || | ||||||||||||||||||||||||||||||||||||||||||
| fileName.endsWith('.freezed.dart'); | ||||||||||||||||||||||||||||||||||||||||||
| } else 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) { | ||||||||||||||||||||||||||||||||||||||||||
| return 100.0; | ||||||||||||||||||||||||||||||||||||||||||
|
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. maybe this is correct but it feels weird to me to have lines found give 100% when there were no lines. |
||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| return (linesHit / linesFound) * 100.0; | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
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.