Skip to content
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,16 @@ targets:
{"dependency": "open_jdk", "version": "version:21"}
]

- name: Linux coverage_checks
recipe: packages/packages
bringup: true
timeout: 60
properties:
add_recipes_cq: "true"
target_file: coverage_checks.yaml
channel: master
version_file: flutter_master.version

- name: Linux dart_unit_test_shard_1 master
recipe: packages/packages
timeout: 60
Expand Down
7 changes: 7 additions & 0 deletions .ci/targets/coverage_checks.yaml
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"]
8 changes: 8 additions & 0 deletions script/configs/custom_coverage_minimums.yaml

Copy link
Copy Markdown
Contributor

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# 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: 30.0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg this is so very low.

camera_android_camerax: 18.3
151 changes: 151 additions & 0 deletions script/tool/lib/src/coverage_check_command.dart

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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,151 @@
// 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 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 '

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 _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.

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.
final double? currentCoverage = await _runCoverageAndParse(package);
if (currentCoverage == null) {
return PackageResult.fail(<String>['Failed to run tests or parse coverage']);
}
final double requiredCoverage = _customMinimums[packageName]!;

// Delete generated lcov.info.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final Directory coverageDir = package.directory.childDirectory('coverage');
if (coverageDir.existsSync()) {
coverageDir.deleteSync(recursive: true);
}
Comment thread
camsim99 marked this conversation as resolved.

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();
Comment on lines +80 to +91

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 io.ProcessResult result = await processRunner.run(
'flutter',
<String>[
'test',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'--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 = package.directory.childDirectory('coverage').childFile('lcov.info');
if (!lcovFile.existsSync()) {
print('Coverage file not found at ${lcovFile.path}.');
return null;
}

return _calculateCoverage(lcovFile);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;

}

/// 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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);
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) {
return 100.0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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;
}
}

bool _isGeneratedFile(String fileName) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable. FWIW the lcov command also has the ability to do exclusions.

return fileName.endsWith('.g.dart') ||
fileName.endsWith('.pb.dart') ||
fileName.endsWith('.mocks.dart');
}
Comment on lines +147 to +151

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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');
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer for addressing #11942 (comment)

2 changes: 2 additions & 0 deletions script/tool/lib/src/main.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import 'analyze_command.dart';
import 'branches_for_batch_release_command.dart';
import 'build_examples_command.dart';
import 'common/core.dart';
import 'coverage_check_command.dart';
import 'create_all_packages_app_command.dart';
import 'custom_test_command.dart';
import 'dart_test_command.dart';
Expand Down Expand Up @@ -57,6 +58,7 @@ void main(List<String> args) {
..addCommand(BranchesForBatchReleaseCommand(packagesDir))
..addCommand(BuildExamplesCommand(packagesDir))
..addCommand(CreateAllPackagesAppCommand(packagesDir))
..addCommand(CoverageCheckCommand(packagesDir))
..addCommand(CustomTestCommand(packagesDir))
..addCommand(DartTestCommand(packagesDir))
..addCommand(DriveExamplesCommand(packagesDir))
Expand Down
Loading
Loading