From 844fb699c790814f20304aa3a3762787145f2f33 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 15:40:24 +0200 Subject: [PATCH 01/14] refactor(cli): extract coverage threshold check into checkCoverage The upcoming `coverage merge` command must report coverage exactly like `very_good test`. Moving the metrics, threshold and uncovered-lines logic into a standalone function lets both commands share it. --- lib/src/cli/cli.dart | 1 + lib/src/cli/coverage_reporter.dart | 38 +++++++++ lib/src/cli/test_cli_runner.dart | 28 ++----- test/src/cli/coverage_reporter_test.dart | 99 ++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 23 deletions(-) create mode 100644 lib/src/cli/coverage_reporter.dart create mode 100644 test/src/cli/coverage_reporter_test.dart diff --git a/lib/src/cli/cli.dart b/lib/src/cli/cli.dart index 3e90a116f..5f31518bc 100644 --- a/lib/src/cli/cli.dart +++ b/lib/src/cli/cli.dart @@ -15,6 +15,7 @@ import 'package:very_good_test_runner/very_good_test_runner.dart'; export 'package:very_good_cli/src/test_optimizer/test_optimizer.dart'; +part 'coverage_reporter.dart'; part 'dart_cli.dart'; part 'flutter_cli.dart'; part 'git_cli.dart'; diff --git a/lib/src/cli/coverage_reporter.dart b/lib/src/cli/coverage_reporter.dart new file mode 100644 index 000000000..eff2fcaac --- /dev/null +++ b/lib/src/cli/coverage_reporter.dart @@ -0,0 +1,38 @@ +part of 'cli.dart'; + +/// Checks the coverage of [records] against [minCoverage]. +/// +/// Files matching [excludeFromCoverage] (space separated globs) are left out +/// of the measurement. +/// +/// Throws [MinCoverageNotMet] when the coverage is below [minCoverage], +/// carrying the uncovered lines when [showUncovered] is set. Otherwise, when +/// [showUncovered] is set and some lines are not covered, they are written to +/// [stdout] as informational output. +void checkCoverage( + List records, { + double? minCoverage, + bool showUncovered = false, + String? excludeFromCoverage, + void Function(String)? stdout, +}) { + final coverageMetrics = CoverageMetrics.fromLcovRecords( + records, + excludeFromCoverage: excludeFromCoverage, + ); + final coverage = coverageMetrics.percentage; + final uncoveredLines = + showUncovered && coverageMetrics.uncoveredLines.isNotEmpty + ? coverageMetrics.uncoveredLines + : null; + + if (minCoverage != null && coverage < minCoverage) { + throw MinCoverageNotMet(coverage, uncoveredLines: uncoveredLines); + } + + // When coverage passes but is below 100%, + // show uncovered lines as informational output. + if (uncoveredLines != null) { + stdout?.call('${TestCLIRunner.formatUncoveredLines(uncoveredLines)}\n'); + } +} diff --git a/lib/src/cli/test_cli_runner.dart b/lib/src/cli/test_cli_runner.dart index 2b3fba6cc..8c7084e34 100644 --- a/lib/src/cli/test_cli_runner.dart +++ b/lib/src/cli/test_cli_runner.dart @@ -302,31 +302,13 @@ class TestCLIRunner { } if (minCoverage != null || showUncovered) { - final records = await Parser.parse(lcovPath); - final coverageMetrics = CoverageMetrics.fromLcovRecords( - records, + checkCoverage( + await Parser.parse(lcovPath), + minCoverage: minCoverage, + showUncovered: showUncovered, excludeFromCoverage: excludeFromCoverage, + stdout: stdout, ); - final coverage = coverageMetrics.percentage; - final uncoveredLines = - showUncovered && coverageMetrics.uncoveredLines.isNotEmpty - ? coverageMetrics.uncoveredLines - : null; - - if (minCoverage != null && coverage < minCoverage) { - throw MinCoverageNotMet( - coverage, - uncoveredLines: uncoveredLines, - ); - } - - // When coverage passes but is below 100%, - // show uncovered lines as informational output. - if (showUncovered && - uncoveredLines != null && - uncoveredLines.isNotEmpty) { - stdout?.call('${formatUncoveredLines(uncoveredLines)}\n'); - } } }), ); diff --git a/test/src/cli/coverage_reporter_test.dart b/test/src/cli/coverage_reporter_test.dart new file mode 100644 index 000000000..f25581741 --- /dev/null +++ b/test/src/cli/coverage_reporter_test.dart @@ -0,0 +1,99 @@ +import 'package:lcov_parser/lcov_parser.dart'; +import 'package:test/test.dart'; +import 'package:very_good_cli/src/cli/cli.dart'; + +void main() { + group(checkCoverage, () { + late List stdoutLogs; + + final records = Parser.parseLines([ + 'SF:lib/a.dart', + 'DA:1,1', + 'DA:2,0', + 'LF:2', + 'LH:1', + 'end_of_record', + 'SF:lib/b.dart', + 'DA:1,1', + 'DA:2,1', + 'LF:2', + 'LH:2', + 'end_of_record', + ]); + + setUp(() { + stdoutLogs = []; + }); + + test('does nothing when no threshold is set', () { + checkCoverage(records, stdout: stdoutLogs.add); + + expect(stdoutLogs, isEmpty); + }); + + test('completes when the threshold is met', () { + checkCoverage(records, minCoverage: 75, stdout: stdoutLogs.add); + + expect(stdoutLogs, isEmpty); + }); + + test('throws $MinCoverageNotMet when the threshold is not met', () { + expect( + () => checkCoverage(records, minCoverage: 80), + throwsA( + isA() + .having((e) => e.coverage, 'coverage', equals(75)) + .having((e) => e.uncoveredLines, 'uncoveredLines', isNull), + ), + ); + }); + + test('throws with uncovered lines when show uncovered is set', () { + expect( + () => checkCoverage(records, minCoverage: 80, showUncovered: true), + throwsA( + isA().having( + (e) => e.uncoveredLines, + 'uncoveredLines', + equals({ + 'lib/a.dart': [2], + }), + ), + ), + ); + }); + + test('logs uncovered lines when the threshold is met', () { + checkCoverage( + records, + minCoverage: 75, + showUncovered: true, + stdout: stdoutLogs.add, + ); + + expect(stdoutLogs, equals(['Lines not covered:\n\t- lib/a.dart: 2\n'])); + }); + + test( + 'logs nothing when show uncovered is set and all lines are covered', + () { + checkCoverage( + records, + showUncovered: true, + excludeFromCoverage: 'lib/a.dart', + stdout: stdoutLogs.add, + ); + + expect(stdoutLogs, isEmpty); + }, + ); + + test('ignores files matching the exclude globs', () { + checkCoverage( + records, + minCoverage: 100, + excludeFromCoverage: 'lib/a.dart lib/c.dart', + ); + }); + }); +} From e4ccf4050d7344709f1434280e57de6400b4c115 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 15:43:25 +0200 Subject: [PATCH 02/14] feat(cli): add lcov parsing, merging and serialization Merging sharded reports needs to sum hits per line, function and branch and recompute the summaries. package:lcov_parser can't be used for this: it stops at the first blank line, keeps \r, splits SF paths on every ':' (breaking Windows absolute paths), throws on tags like VER or FNL, and doesn't export its detail models, so records can't be rebuilt from it. --- lib/src/cli/cli.dart | 2 + lib/src/cli/lcov_merger.dart | 182 ++++++++++++++++++++++ test/src/cli/lcov_merger_test.dart | 232 +++++++++++++++++++++++++++++ 3 files changed, 416 insertions(+) create mode 100644 lib/src/cli/lcov_merger.dart create mode 100644 test/src/cli/lcov_merger_test.dart diff --git a/lib/src/cli/cli.dart b/lib/src/cli/cli.dart index 5f31518bc..687350f4f 100644 --- a/lib/src/cli/cli.dart +++ b/lib/src/cli/cli.dart @@ -1,4 +1,5 @@ import 'dart:async'; +import 'dart:convert'; import 'dart:math'; import 'package:collection/collection.dart'; @@ -19,6 +20,7 @@ part 'coverage_reporter.dart'; part 'dart_cli.dart'; part 'flutter_cli.dart'; part 'git_cli.dart'; +part 'lcov_merger.dart'; part 'test_cli_runner.dart'; const R Function( diff --git a/lib/src/cli/lcov_merger.dart b/lib/src/cli/lcov_merger.dart new file mode 100644 index 000000000..31e86a45e --- /dev/null +++ b/lib/src/cli/lcov_merger.dart @@ -0,0 +1,182 @@ +part of 'cli.dart'; + +/// A branch of an lcov record, identified by its line, block and branch +/// number. +typedef LcovBranch = (int line, int block, int branch); + +/// {@template lcov_record} +/// The coverage of a single source file, as described by an lcov record. +/// +/// Only the details are kept: the `LF/LH/FNF/FNH/BRF/BRH` summaries are +/// derived from them when serializing, so they stay correct after merging. +/// {@endtemplate} +class LcovRecord { + /// {@macro lcov_record} + new( + this.file, { + Map? lines, + Map? functionLines, + Map? functionHits, + Map? branches, + }) : lines = lines ?? {}, + functionLines = functionLines ?? {}, + functionHits = functionHits ?? {}, + branches = branches ?? {}; + + /// The source file path (`SF:`). + final String file; + + /// Hits per line number (`DA:`). + final Map lines; + + /// Line number per function name (`FN:`). + final Map functionLines; + + /// Hits per function name (`FNDA:`). + final Map functionHits; + + /// Times taken per branch (`BRDA:`). + final Map branches; + + /// Adds the hits of [other] into this record. + void addAll(LcovRecord other) { + void sum(Map target, Map source) { + for (final MapEntry(:key, :value) in source.entries) { + target[key] = (target[key] ?? 0) + value; + } + } + + sum(lines, other.lines); + sum(functionHits, other.functionHits); + sum(branches, other.branches); + for (final MapEntry(:key, :value) in other.functionLines.entries) { + functionLines.putIfAbsent(key, () => value); + } + } + + /// Serializes this record to lcov, ending with `end_of_record`. + String toLcov() { + final buffer = StringBuffer()..writeln('SF:$file'); + + if (functionLines.isNotEmpty) { + final names = functionLines.keys.sortedBy((n) => functionLines[n]!); + for (final name in names) { + buffer.writeln('FN:${functionLines[name]},$name'); + } + for (final name in names) { + final hits = functionHits[name] ?? 0; + if (hits > 0) buffer.writeln('FNDA:$hits,$name'); + } + buffer + ..writeln('FNF:${names.length}') + ..writeln( + 'FNH:${names.where((n) => (functionHits[n] ?? 0) > 0).length}', + ); + } + + for (final line in lines.keys.sorted((a, b) => a - b)) { + buffer.writeln('DA:$line,${lines[line]}'); + } + buffer + ..writeln('LF:${lines.length}') + ..writeln('LH:${lines.values.where((hits) => hits > 0).length}'); + + if (branches.isNotEmpty) { + final keys = branches.keys.sorted( + (a, b) => [ + a.$1 - b.$1, + a.$2 - b.$2, + a.$3 - b.$3, + ].firstWhere((order) => order != 0, orElse: () => 0), + ); + for (final key in keys) { + buffer.writeln('BRDA:${key.$1},${key.$2},${key.$3},${branches[key]}'); + } + buffer + ..writeln('BRF:${branches.length}') + ..writeln('BRH:${branches.values.where((taken) => taken > 0).length}'); + } + + buffer.writeln('end_of_record'); + return buffer.toString(); + } +} + +/// Parses the lcov [content] into one [LcovRecord] per `end_of_record`. +/// +/// Unlike `package:lcov_parser`, this tolerates CRLF line endings, blank +/// lines, `:` and `,` in source paths and tags it doesn't know about, which +/// are ignored along with the `LF/LH/FNF/FNH/BRF/BRH` summaries. +/// +/// Throws a [FormatException] when a line is malformed. +List parseLcov(String content) { + final records = []; + LcovRecord? record; + + for (final rawLine in const LineSplitter().convert(content)) { + final line = rawLine.trim(); + if (line.isEmpty) continue; + + if (line == 'end_of_record') { + if (record != null) records.add(record); + record = null; + continue; + } + + final separator = line.indexOf(':'); + if (separator < 0) { + throw FormatException('Invalid lcov line "$line".'); + } + final tag = line.substring(0, separator); + final value = line.substring(separator + 1); + final fields = value.split(','); + + int number(int index) => + int.tryParse(fields.elementAtOrNull(index) ?? '') ?? + (throw FormatException('Invalid lcov line "$line".')); + + // Function names may contain commas, so they span the remaining fields. + String name() => fields.skip(1).join(','); + + switch ((tag, record)) { + case ('SF', _): + record = LcovRecord(value); + case ('DA' || 'FN' || 'FNDA' || 'BRDA', null): + throw FormatException('Found "$line" before any "SF:" line.'); + case ('DA', final LcovRecord current): + final lineNumber = number(0); + current.lines[lineNumber] = + (current.lines[lineNumber] ?? 0) + number(1); + case ('FN', final LcovRecord current): + current.functionLines[name()] = number(0); + case ('FNDA', final LcovRecord current): + current.functionHits[name()] = + (current.functionHits[name()] ?? 0) + number(0); + case ('BRDA', final LcovRecord current): + final branch = (number(0), number(1), number(2)); + // A `-` means the branch was never reached, which counts as not taken. + final taken = fields.elementAtOrNull(3) == '-' ? 0 : number(3); + current.branches[branch] = (current.branches[branch] ?? 0) + taken; + } + } + + if (record != null) records.add(record); + return records; +} + +/// Merges [records] that describe the same source file, summing their hits. +/// +/// The result keeps the order in which each file first appears. +List mergeLcovRecords(Iterable records) { + final merged = {}; + for (final record in records) { + merged + .putIfAbsent(record.file, () => LcovRecord(record.file)) + .addAll(record); + } + return merged.values.toList(); +} + +/// Serializes [records] to lcov. +String formatLcovRecords(Iterable records) => + records.map((record) => record.toLcov()).join(); diff --git a/test/src/cli/lcov_merger_test.dart b/test/src/cli/lcov_merger_test.dart new file mode 100644 index 000000000..f59605322 --- /dev/null +++ b/test/src/cli/lcov_merger_test.dart @@ -0,0 +1,232 @@ +import 'package:lcov_parser/lcov_parser.dart'; +import 'package:test/test.dart'; +import 'package:very_good_cli/src/cli/cli.dart'; + +import '../../fixtures/fixtures.dart'; + +/// The layout `package:coverage` writes for `dart test` and `flutter test`. +const _dartLcov = ''' +SF:lib/src/a.dart +FN:3,A.new +FN:7,A.call +FNDA:2,A.new +FNF:2 +FNH:1 +DA:3,2 +DA:7,0 +DA:8,0 +LF:3 +LH:1 +BRDA:7,0,0,0 +BRDA:7,0,1,1 +BRF:2 +BRH:1 +end_of_record +'''; + +void main() { + group(parseLcov, () { + test('parses lines, functions and branches', () { + final [record] = parseLcov(_dartLcov); + + expect(record.file, equals('lib/src/a.dart')); + expect(record.lines, equals({3: 2, 7: 0, 8: 0})); + expect(record.functionLines, equals({'A.new': 3, 'A.call': 7})); + expect(record.functionHits, equals({'A.new': 2})); + expect(record.branches, equals({(7, 0, 0): 0, (7, 0, 1): 1})); + }); + + test('returns no records for an empty report', () { + expect(parseLcov(''), isEmpty); + }); + + test('tolerates CRLF line endings and blank lines', () { + final records = parseLcov( + 'SF:lib/a.dart\r\nDA:1,1\r\nend_of_record\r\n' + '\r\n' + 'SF:lib/b.dart\r\nDA:1,0\r\nend_of_record\r\n', + ); + + expect(records.map((r) => r.file), equals(['lib/a.dart', 'lib/b.dart'])); + expect(records.last.lines, equals({1: 0})); + }); + + test('keeps colons and commas in source paths', () { + final [record] = parseLcov( + r'SF:C:\a,b\c.dart' + '\nend_of_record\n', + ); + + expect(record.file, equals(r'C:\a,b\c.dart')); + }); + + test('keeps commas in function names', () { + final [record] = parseLcov( + 'SF:a.dart\nFN:1,f\nFNDA:1,f\nend_of_record\n', + ); + + expect(record.functionLines, equals({'f': 1})); + expect(record.functionHits, equals({'f': 1})); + }); + + test('ignores unknown tags and summaries', () { + final [record] = parseLcov( + 'TN:\nVER:2\nSF:a.dart\nFNL:0,1,2\nDA:1,1\nLF:9\nLH:9\nend_of_record\n', + ); + + expect(record.lines, equals({1: 1})); + }); + + test('treats untaken "-" branches as not taken', () { + final [record] = parseLcov('SF:a.dart\nBRDA:1,0,0,-\nend_of_record\n'); + + expect(record.branches, equals({(1, 0, 0): 0})); + }); + + test('sums duplicated lines within a record', () { + final [record] = parseLcov('SF:a.dart\nDA:1,1\nDA:1,2\nend_of_record\n'); + + expect(record.lines, equals({1: 3})); + }); + + test('keeps a trailing record without end_of_record', () { + final [record] = parseLcov('SF:a.dart\nDA:1,1'); + + expect(record.lines, equals({1: 1})); + }); + + group('throws $FormatException', () { + for (final (description, content) in [ + ('for a line without a tag', 'SF:a.dart\nfoo\n'), + ('for details before any SF', 'DA:1,1\n'), + ('for a non numeric value', 'SF:a.dart\nDA:1,x\n'), + ('for a missing value', 'SF:a.dart\nBRDA:1,0\n'), + ]) { + test(description, () { + expect(() => parseLcov(content), throwsFormatException); + }); + } + }); + }); + + group(mergeLcovRecords, () { + test('sums hits of records for the same file', () { + final [record] = mergeLcovRecords([ + ...parseLcov(_dartLcov), + ...parseLcov( + 'SF:lib/src/a.dart\nFN:7,A.call\nFNDA:1,A.call\n' + 'DA:3,1\nDA:7,1\nBRDA:7,0,0,1\nend_of_record\n', + ), + ]); + + expect(record.lines, equals({3: 3, 7: 1, 8: 0})); + expect(record.functionHits, equals({'A.new': 2, 'A.call': 1})); + expect(record.functionLines, equals({'A.new': 3, 'A.call': 7})); + expect(record.branches, equals({(7, 0, 0): 1, (7, 0, 1): 1})); + }); + + test('keeps distinct files in first seen order', () { + final records = mergeLcovRecords([ + ...parseLcov('SF:b.dart\nDA:1,1\nend_of_record\n'), + ...parseLcov('SF:a.dart\nDA:1,1\nend_of_record\n'), + ...parseLcov('SF:b.dart\nDA:2,1\nend_of_record\n'), + ]); + + expect(records.map((r) => r.file), equals(['b.dart', 'a.dart'])); + expect(records.first.lines, equals({1: 1, 2: 1})); + }); + + test('covers a padded 0% record with hits from another report', () { + final [record] = mergeLcovRecords([ + ...parseLcov('SF:a.dart\nDA:1,0\nDA:2,0\nLF:2\nLH:0\nend_of_record\n'), + ...parseLcov('SF:a.dart\nDA:1,1\nDA:2,3\nLF:2\nLH:2\nend_of_record\n'), + ]); + + expect(record.toLcov(), contains('LH:2\n')); + }); + + test('accepts empty reports', () { + final records = mergeLcovRecords([ + ...parseLcov(''), + ...parseLcov('SF:a.dart\nDA:1,1\nend_of_record\n'), + ]); + + expect(records, hasLength(1)); + }); + + test('does not modify the given records', () { + final records = parseLcov('SF:a.dart\nDA:1,1\nend_of_record\n'); + + mergeLcovRecords([...records, ...records]); + + expect(records.single.lines, equals({1: 1})); + }); + }); + + group(formatLcovRecords, () { + test('recomputes summaries from the details', () { + final record = LcovRecord( + 'a.dart', + lines: {2: 0, 1: 4}, + functionLines: {'f': 1, 'g': 2}, + functionHits: {'f': 1, 'g': 0}, + branches: {(1, 0, 1): 0, (1, 0, 0): 2}, + ); + + expect( + formatLcovRecords([record]), + equals(''' +SF:a.dart +FN:1,f +FN:2,g +FNDA:1,f +FNF:2 +FNH:1 +DA:1,4 +DA:2,0 +LF:2 +LH:1 +BRDA:1,0,0,2 +BRDA:1,0,1,0 +BRF:2 +BRH:1 +end_of_record +'''), + ); + }); + + test('round-trips the package:coverage layout', () { + expect(formatLcovRecords(parseLcov(_dartLcov)), equals(_dartLcov)); + }); + + test('round-trips flutter reports', () { + for (final lcov in [lcov100, lcov95]) { + expect(formatLcovRecords(parseLcov(lcov)), equals(lcov)); + } + }); + + test('is readable by package:lcov_parser', () { + final merged = formatLcovRecords(parseLcov(lcov95)); + + final records = Parser.parseLines(merged.split('\n')); + final expected = Parser.parseLines(lcov95.split('\n')); + + expect( + CoverageMetrics.fromLcovRecords(records).percentage, + equals(CoverageMetrics.fromLcovRecords(expected).percentage), + ); + }); + + test('sorts branches by line, block and branch', () { + final record = LcovRecord( + 'a.dart', + branches: {(2, 0, 0): 1, (1, 1, 0): 1, (1, 0, 1): 1, (1, 0, 0): 1}, + ); + + expect( + RegExp('BRDA:(.*)').allMatches(record.toLcov()).map((m) => m[1]), + equals(['1,0,0,1', '1,0,1,1', '1,1,0,1', '2,0,0,1']), + ); + }); + }); +} From 09daec1f0d96632c35d90a1ca8ae895982572dce Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 15:44:51 +0200 Subject: [PATCH 03/14] feat(cli): normalize lcov source paths before merging Reports from different runners or packages must key the same file the same way: separators become '/', absolute paths under the current directory become relative, and relative paths can be rebased onto their package so lib/a.dart from two packages stay distinct. Absolute paths outside the current directory are kept and surfaced to the caller. --- lib/src/cli/lcov_merger.dart | 51 +++++++++++++++++ test/src/cli/lcov_merger_test.dart | 88 ++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+) diff --git a/lib/src/cli/lcov_merger.dart b/lib/src/cli/lcov_merger.dart index 31e86a45e..d576c8ce4 100644 --- a/lib/src/cli/lcov_merger.dart +++ b/lib/src/cli/lcov_merger.dart @@ -38,6 +38,15 @@ class LcovRecord { /// Times taken per branch (`BRDA:`). final Map branches; + /// A copy of this record for the source [file]. + LcovRecord withFile(String file) => LcovRecord( + file, + lines: {...lines}, + functionLines: {...functionLines}, + functionHits: {...functionHits}, + branches: {...branches}, + ); + /// Adds the hits of [other] into this record. void addAll(LcovRecord other) { void sum(Map target, Map source) { @@ -164,6 +173,48 @@ List parseLcov(String content) { return records; } +/// Normalizes the source path of [records] so that reports produced on +/// different runners, or for different packages, key the same file the same +/// way. +/// +/// * Separators become `/`. +/// * Relative paths are rebased onto [packagePath] (relative to the current +/// directory of [context]) when given, so that `lib/a.dart` from two +/// packages stay distinct. +/// * Absolute paths under the current directory of [context] become relative +/// to it. Others are kept, and reported through [onExternalPath]. +/// +/// [context] defaults to the platform's [p.context]. +List normalizeLcovRecords( + Iterable records, { + String? packagePath, + p.Context? context, + void Function(String path)? onExternalPath, +}) { + final ctx = context ?? p.context; + + String normalize(String file) { + final path = file.replaceAll(r'\', '/'); + + final String resolved; + if (ctx.isAbsolute(path) && ctx.isWithin(ctx.current, path)) { + resolved = ctx.relative(path); + } else if (ctx.isAbsolute(path) || p.windows.isAbsolute(path)) { + // Also checked as Windows, since a report from a Windows runner can be + // merged on any other platform. + onExternalPath?.call(path); + resolved = path; + } else { + resolved = packagePath == null ? path : ctx.join(packagePath, path); + } + return ctx.normalize(resolved).replaceAll(r'\', '/'); + } + + return [ + for (final record in records) record.withFile(normalize(record.file)), + ]; +} + /// Merges [records] that describe the same source file, summing their hits. /// /// The result keeps the order in which each file first appears. diff --git a/test/src/cli/lcov_merger_test.dart b/test/src/cli/lcov_merger_test.dart index f59605322..fad5b5bc5 100644 --- a/test/src/cli/lcov_merger_test.dart +++ b/test/src/cli/lcov_merger_test.dart @@ -1,4 +1,5 @@ import 'package:lcov_parser/lcov_parser.dart'; +import 'package:path/path.dart' as p; import 'package:test/test.dart'; import 'package:very_good_cli/src/cli/cli.dart'; @@ -229,4 +230,91 @@ end_of_record ); }); }); + + group(normalizeLcovRecords, () { + final posix = p.Context(style: p.Style.posix, current: '/repo'); + final windows = p.Context(style: p.Style.windows, current: r'C:\repo'); + + List normalize( + List files, { + String? packagePath, + p.Context? context, + void Function(String)? onExternalPath, + }) => normalizeLcovRecords( + files.map(LcovRecord.new), + packagePath: packagePath, + context: context ?? posix, + onExternalPath: onExternalPath, + ).map((record) => record.file).toList(); + + test('keeps relative paths without a package path', () { + expect(normalize(['lib/a.dart', './lib/b.dart']), [ + 'lib/a.dart', + 'lib/b.dart', + ]); + }); + + test('rebases relative paths onto the package path', () { + expect( + normalize(['lib/a.dart'], packagePath: 'packages/foo'), + equals(['packages/foo/lib/a.dart']), + ); + expect(normalize(['lib/a.dart'], packagePath: '.'), ['lib/a.dart']); + }); + + test('converts Windows separators', () { + expect( + normalize([r'lib\src\a.dart'], packagePath: 'packages/foo'), + equals(['packages/foo/lib/src/a.dart']), + ); + expect( + normalize( + [r'lib\src\a.dart'], + packagePath: r'packages\foo', + context: windows, + ), + equals(['packages/foo/lib/src/a.dart']), + ); + }); + + test('makes absolute paths under the current directory relative', () { + expect( + normalize([ + '/repo/packages/foo/lib/a.dart', + ], packagePath: 'packages/foo'), + equals(['packages/foo/lib/a.dart']), + ); + expect( + normalize([r'c:\repo\lib\a.dart'], context: windows), + equals(['lib/a.dart']), + ); + }); + + test('keeps and reports absolute paths outside the current directory', () { + final external = []; + + final files = normalize( + ['/other/lib/a.dart', r'D:\runner\lib\a.dart', 'lib/b.dart'], + packagePath: 'packages/foo', + onExternalPath: external.add, + ); + + expect(files, [ + '/other/lib/a.dart', + 'D:/runner/lib/a.dart', + 'packages/foo/lib/b.dart', + ]); + expect(external, ['/other/lib/a.dart', 'D:/runner/lib/a.dart']); + }); + + test('does not modify the given records', () { + final record = LcovRecord('lib/a.dart', lines: {1: 1}); + + final [normalized] = normalizeLcovRecords([record], packagePath: 'foo'); + normalized.lines[1] = 2; + + expect(record.file, equals('lib/a.dart')); + expect(record.lines, equals({1: 1})); + }); + }); } From 186a072d56837ffe5589ec84fc309ab2adb39858 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 15:55:01 +0200 Subject: [PATCH 04/14] feat: add coverage merge command Sharded and recursive runs leave one lcov report per shard or package, with no way to enforce --min-coverage on the whole suite. 'very_good coverage merge' unions the given reports (files or globs, expanded by the CLI so quoted patterns work on every shell), writes the result and enforces the threshold with the same output as 'very_good test'. --min-coverage, --exclude-coverage and --show-uncovered fall back to the existing test.* and then dart.test.* keys in very_good.yaml. The 'coverage/' rule in .gitignore, meant for generated reports, also matched the new command and test directories, so they are re-included. --- .gitignore | 3 + lib/src/command_runner.dart | 1 + lib/src/commands/commands.dart | 1 + .../commands/coverage/commands/commands.dart | 1 + lib/src/commands/coverage/commands/merge.dart | 212 ++++++++ lib/src/commands/coverage/coverage.dart | 21 + test/src/command_runner_test.dart | 1 + .../coverage/commands/merge_test.dart | 492 ++++++++++++++++++ test/src/commands/coverage/coverage_test.dart | 33 ++ 9 files changed, 765 insertions(+) create mode 100644 lib/src/commands/coverage/commands/commands.dart create mode 100644 lib/src/commands/coverage/commands/merge.dart create mode 100644 lib/src/commands/coverage/coverage.dart create mode 100644 test/src/commands/coverage/commands/merge_test.dart create mode 100644 test/src/commands/coverage/coverage_test.dart diff --git a/.gitignore b/.gitignore index 4ea56d1fb..cbcb8791c 100644 --- a/.gitignore +++ b/.gitignore @@ -15,6 +15,9 @@ doc/api/ # Files generated during tests .test_coverage.dart coverage/ +!lib/src/commands/coverage/ +!test/src/commands/coverage/ +!e2e/test/commands/coverage/ .test_optimizer.dart !bricks/test_optimizer/__brick__/test/.test_optimizer.dart *.vm.json diff --git a/lib/src/command_runner.dart b/lib/src/command_runner.dart index 85f8cf1e2..288d0f06f 100644 --- a/lib/src/command_runner.dart +++ b/lib/src/command_runner.dart @@ -33,6 +33,7 @@ class VeryGoodCommandRunner extends CompletionCommandRunner { 'verbose', help: 'Noisy logging, including all shell commands executed.', ); + addCommand(CoverageCommand(logger: _logger)); addCommand(CreateCommand(logger: _logger)); addCommand(PackagesCommand(logger: _logger)); addCommand(TestCommand(logger: _logger)); diff --git a/lib/src/commands/commands.dart b/lib/src/commands/commands.dart index 86e5367a2..635e7a98b 100644 --- a/lib/src/commands/commands.dart +++ b/lib/src/commands/commands.dart @@ -1,3 +1,4 @@ +export 'coverage/coverage.dart'; export 'create/commands/commands.dart'; export 'create/create.dart'; export 'dart/dart.dart'; diff --git a/lib/src/commands/coverage/commands/commands.dart b/lib/src/commands/coverage/commands/commands.dart new file mode 100644 index 000000000..9b5208a44 --- /dev/null +++ b/lib/src/commands/coverage/commands/commands.dart @@ -0,0 +1 @@ +export 'merge.dart'; diff --git a/lib/src/commands/coverage/commands/merge.dart b/lib/src/commands/coverage/commands/merge.dart new file mode 100644 index 000000000..e26ac18f5 --- /dev/null +++ b/lib/src/commands/coverage/commands/merge.dart @@ -0,0 +1,212 @@ +import 'package:args/command_runner.dart'; +import 'package:collection/collection.dart'; +import 'package:glob/glob.dart'; +import 'package:glob/list_local_fs.dart'; +import 'package:lcov_parser/lcov_parser.dart'; +import 'package:mason/mason.dart'; +import 'package:path/path.dart' as p; +import 'package:universal_io/io.dart'; +import 'package:very_good_cli/src/cli/cli.dart'; +import 'package:very_good_cli/src/very_good_config/very_good_config.dart'; + +/// {@template coverage_merge_command} +/// `very_good coverage merge` command for merging lcov reports. +/// +/// Combines the reports of sharded (`--shard-index`/`--total-shards`) or +/// recursive (`--recursive`) test runs into a single report, then enforces +/// `--min-coverage` on it the same way `very_good test` does. +/// {@endtemplate} +class CoverageMergeCommand extends Command { + /// {@macro coverage_merge_command} + new({required this._logger}) { + argParser + ..addOption( + 'output', + abbr: 'o', + defaultsTo: 'coverage/lcov.info', + help: 'The path to write the merged lcov report to.', + valueHelp: 'path', + ) + ..addOption( + 'min-coverage', + help: 'Whether to enforce a minimum coverage percentage.', + ) + ..addOption( + 'exclude-coverage', + help: + 'A glob which will be used to exclude files that match from the ' + "coverage (e.g. '**/*.g.dart').", + ) + ..addFlag( + 'show-uncovered', + help: 'Whether to show uncovered lines when coverage is below 100%.', + negatable: false, + ); + } + + final Logger _logger; + + @override + String get description => + 'Merge lcov reports, from sharded or recursive test runs, and check the ' + 'merged coverage.'; + + @override + String get name => 'merge'; + + @override + String get invocation => + 'very_good coverage merge [lcov files or globs] [arguments]'; + + @override + Future run() async { + final argResults = this.argResults!; + final cwd = p.normalize(Directory.current.absolute.path); + + final config = VeryGoodConfig.load(Directory(cwd), logger: _logger); + if (config == null) return ExitCode.config.code; + final testConfig = config.test; + final dartTestConfig = config.dart.test; + + final rawMinCoverage = argResults.resolve( + 'min-coverage', + testConfig.minCoverage ?? dartTestConfig.minCoverage, + ); + final minCoverage = double.tryParse(rawMinCoverage ?? ''); + final excludeFromCoverage = argResults.resolve( + 'exclude-coverage', + testConfig.excludeCoverage ?? dartTestConfig.excludeCoverage, + ); + final showUncovered = argResults.resolve( + 'show-uncovered', + testConfig.showUncovered ?? dartTestConfig.showUncovered, + ); + final output = argResults['output'] as String; + + try { + if (rawMinCoverage != null && minCoverage == null) { + throw _MergeError( + '--min-coverage must be a number, but got "$rawMinCoverage".', + ExitCode.usage.code, + ); + } + + final inputs = _resolveInputs(argResults.rest, cwd: cwd); + final externalPaths = {}; + final records = [ + for (final input in inputs) + ...normalizeLcovRecords( + _parse(input.path), + packagePath: input.packagePath, + onExternalPath: externalPaths.add, + ), + ]; + + if (externalPaths.isNotEmpty) { + _logger.warn( + 'These source paths are outside of $cwd and were kept as is, so ' + 'they will not match the same files from other reports:\n' + '${externalPaths.map((path) => ' - $path').join('\n')}', + ); + } + + final outputFile = File(p.join(cwd, output)); + await outputFile.create(recursive: true); + await outputFile.writeAsString( + formatLcovRecords(mergeLcovRecords(records)), + ); + _logger.info('Merged ${inputs.length} lcov report(s) into $output'); + + if (minCoverage != null || showUncovered) { + checkCoverage( + await Parser.parse(outputFile.path), + minCoverage: minCoverage, + showUncovered: showUncovered, + excludeFromCoverage: excludeFromCoverage, + stdout: _logger.write, + ); + } + } on _MergeError catch (error) { + _logger.err(error.message); + return error.exitCode; + } on MinCoverageNotMet catch (error) { + return TestCLIRunner.handleMinCoverageNotMet( + error, + logger: _logger, + minCoverage: minCoverage, + ); + } + + return ExitCode.success.code; + } + + /// The lcov reports to merge, and the package directory their relative + /// source paths should be rebased onto. + /// + /// Each of [args] is a path to an lcov file or, when no such file exists, a + /// glob relative to [cwd]. Expanding globs here, rather than relying on the + /// shell, makes quoted patterns behave the same on every platform. + List<({String path, String? packagePath})> _resolveInputs( + List args, { + required String cwd, + }) { + if (args.isEmpty) { + throw _MergeError( + 'No lcov reports to merge. Pass the lcov files or globs to merge.', + ExitCode.usage.code, + ); + } + + final paths = {}; + for (final arg in args) { + if (File(p.join(cwd, arg)).existsSync()) { + paths.add(p.normalize(arg)); + continue; + } + + final List matches; + try { + matches = Glob(arg) + .listSync(root: cwd) + .whereType() + .map((file) => p.relative(file.path, from: cwd)) + .sorted(); + } on FormatException catch (error) { + throw _MergeError( + 'Invalid glob "$arg": ${error.message}', + ExitCode.usage.code, + ); + } + + if (matches.isEmpty) { + throw _MergeError( + 'No lcov report found at "$arg".', + ExitCode.noInput.code, + ); + } + paths.addAll(matches); + } + + return [for (final path in paths) (path: path, packagePath: null)]; + } + + List _parse(String path) { + try { + return parseLcov(File(path).readAsStringSync()); + } on FormatException catch (error) { + throw _MergeError( + 'Could not parse the lcov report "$path": ${error.message}', + ExitCode.data.code, + ); + } + } +} + +/// A failure that stops the merge, reported with [message] and [exitCode]. +class _MergeError implements Exception { + const new(this.message, this.exitCode); + + final String message; + + final int exitCode; +} diff --git a/lib/src/commands/coverage/coverage.dart b/lib/src/commands/coverage/coverage.dart new file mode 100644 index 000000000..5b48d29c0 --- /dev/null +++ b/lib/src/commands/coverage/coverage.dart @@ -0,0 +1,21 @@ +import 'package:args/command_runner.dart'; +import 'package:mason/mason.dart'; +import 'package:very_good_cli/src/commands/coverage/commands/commands.dart'; + +export 'commands/commands.dart'; + +/// {@template coverage_command} +/// `very_good coverage` command for working with coverage reports. +/// {@endtemplate} +class CoverageCommand extends Command { + /// {@macro coverage_command} + new({required Logger logger}) { + addSubcommand(CoverageMergeCommand(logger: logger)); + } + + @override + String get description => 'Command for working with coverage reports.'; + + @override + String get name => 'coverage'; +} diff --git a/test/src/command_runner_test.dart b/test/src/command_runner_test.dart index df0124aed..d26cc2ed2 100644 --- a/test/src/command_runner_test.dart +++ b/test/src/command_runner_test.dart @@ -35,6 +35,7 @@ const expectedUsage = [ ''' --[no-]verbose Noisy logging, including all shell commands executed.\n''', '\n', 'Available commands:\n', + ' coverage Command for working with coverage reports.\n', ' create very_good create [arguments]\n', ''' Creates a new very good project in the specified directory.\n''', ' dart Command for running dart related commands.\n', diff --git a/test/src/commands/coverage/commands/merge_test.dart b/test/src/commands/coverage/commands/merge_test.dart new file mode 100644 index 000000000..a85be45a5 --- /dev/null +++ b/test/src/commands/coverage/commands/merge_test.dart @@ -0,0 +1,492 @@ +// Expected usage of the plugin will need to be adjacent strings due to format +// and also be longer than 80 chars. +// ignore_for_file: no_adjacent_strings_in_list, lines_longer_than_80_chars + +import 'dart:io'; + +import 'package:mason/mason.dart'; +import 'package:mocktail/mocktail.dart'; +import 'package:path/path.dart' as p; +import 'package:test/test.dart'; + +import '../../../../helpers/helpers.dart'; + +const _expectedMergeUsage = [ + 'Merge lcov reports, from sharded or recursive test runs, and check the merged coverage.\n' + '\n' + 'Usage: very_good coverage merge [lcov files or globs] [arguments]\n' + '-h, --help Print this usage information.\n' + '-o, --output= The path to write the merged lcov report to.\n' + ' (defaults to "coverage/lcov.info")\n' + ' --min-coverage Whether to enforce a minimum coverage percentage.\n' + " --exclude-coverage A glob which will be used to exclude files that match from the coverage (e.g. '**/*.g.dart').\n" + ' --show-uncovered Whether to show uncovered lines when coverage is below 100%.\n' + '\n' + 'Run "very_good help" to see global options.', +]; + +/// The first shard covers line 1 of `a.dart`, the second one line 2, and +/// neither covers `b.dart`. +const _shard1 = ''' +SF:lib/a.dart +DA:1,1 +DA:2,0 +LF:2 +LH:1 +end_of_record +SF:lib/b.dart +DA:1,0 +LF:1 +LH:0 +end_of_record +'''; + +const _shard2 = ''' +SF:lib/a.dart +DA:1,0 +DA:2,2 +LF:2 +LH:1 +end_of_record +'''; + +const _merged = ''' +SF:lib/a.dart +DA:1,1 +DA:2,2 +LF:2 +LH:2 +end_of_record +SF:lib/b.dart +DA:1,0 +LF:1 +LH:0 +end_of_record +'''; + +/// Makes a fresh temporary directory the current one for the test. +Directory _enterTempDirectory() { + final previous = Directory.current; + final directory = Directory.systemTemp.createTempSync(); + Directory.current = directory; + addTearDown(() { + Directory.current = previous; + directory.deleteSync(recursive: true); + }); + return Directory.current; +} + +void _writeFile(String path, String content) { + File(path) + ..createSync(recursive: true) + ..writeAsStringSync(content); +} + +void _writeShards() { + _writeFile(p.join('shards', '1', 'lcov.info'), _shard1); + _writeFile(p.join('shards', '2', 'lcov.info'), _shard2); +} + +String _readOutput([String path = 'coverage/lcov.info']) => + File(path).readAsStringSync(); + +void main() { + group('coverage merge', () { + test( + 'help', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + final result = await commandRunner.run(['coverage', 'merge', '--help']); + + expect(printLogs, equals(_expectedMergeUsage)); + expect(result, equals(ExitCode.success.code)); + }), + ); + + test( + 'merges the given lcov files into coverage/lcov.info', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/1/lcov.info', + 'shards/2/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), equals(_merged)); + verify( + () => logger.info('Merged 2 lcov report(s) into coverage/lcov.info'), + ).called(1); + }), + ); + + test( + 'writes to --output, creating its directory', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/1/lcov.info', + 'shards/2/lcov.info', + '-o', + 'out/merged.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput('out/merged.info'), equals(_merged)); + }), + ); + + test( + 'can overwrite one of its inputs', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/1/lcov.info', + 'shards/2/lcov.info', + '-o', + 'shards/1/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput('shards/1/lcov.info'), equals(_merged)); + }), + ); + + test( + 'expands globs, merging each matched file once', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + 'shards/1/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), equals(_merged)); + verify( + () => logger.info('Merged 2 lcov report(s) into coverage/lcov.info'), + ).called(1); + }), + ); + + test( + 'makes absolute source paths under the current directory relative', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + final cwd = _enterTempDirectory(); + _writeFile( + 'shard.info', + 'SF:${p.join(cwd.path, 'lib', 'a.dart')}\nDA:1,1\nend_of_record\n', + ); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shard.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), startsWith('SF:lib/a.dart\n')); + verifyNever(() => logger.warn(any())); + }), + ); + + test( + 'warns about absolute source paths outside the current directory', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + final cwd = _enterTempDirectory(); + _writeFile( + 'shard.info', + 'SF:/elsewhere/a.dart\nDA:1,1\nend_of_record\n', + ); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shard.info', + ]); + + expect(result, equals(ExitCode.success.code)); + verify( + () => logger.warn( + 'These source paths are outside of ${cwd.path} and were kept as ' + 'is, so they will not match the same files from other reports:\n' + ' - /elsewhere/a.dart', + ), + ).called(1); + }), + ); + + group('fails', () { + test( + 'when no lcov file is given', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + + final result = await commandRunner.run(['coverage', 'merge']); + + expect(result, equals(ExitCode.usage.code)); + verify( + () => logger.err( + 'No lcov reports to merge. Pass the lcov files or globs to merge.', + ), + ).called(1); + }), + ); + + test( + 'when a file or glob matches nothing', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/1/lcov.info', + 'missing/*.info', + ]); + + expect(result, equals(ExitCode.noInput.code)); + verify(() => logger.err('No lcov report found at "missing/*.info".')) + .called(1); + expect(File('coverage/lcov.info').existsSync(), isFalse); + }), + ); + + test( + 'when a glob is invalid', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + + final result = await commandRunner.run(['coverage', 'merge', '[']); + + expect(result, equals(ExitCode.usage.code)); + verify(() => logger.err(any(that: startsWith('Invalid glob "[": ')))) + .called(1); + }), + ); + + test( + 'when an lcov file cannot be parsed', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeFile('shard.info', 'not lcov\n'); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shard.info', + ]); + + expect(result, equals(ExitCode.data.code)); + verify( + () => logger.err( + 'Could not parse the lcov report "shard.info": ' + 'Invalid lcov line "not lcov".', + ), + ).called(1); + }), + ); + + test( + 'when --min-coverage is not a number', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/1/lcov.info', + '--min-coverage', + 'abc', + ]); + + expect(result, equals(ExitCode.usage.code)); + verify( + () => logger.err('--min-coverage must be a number, but got "abc".'), + ).called(1); + }), + ); + + test( + 'when very_good.yaml is invalid', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeFile('very_good.yaml', 'test:\n min_coverage: abc\n'); + + final result = await commandRunner.run(['coverage', 'merge']); + + expect(result, equals(ExitCode.config.code)); + }), + ); + }); + + group('--min-coverage', () { + test( + 'succeeds when the merged coverage meets it', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + '--min-coverage', + '66', + ]); + + expect(result, equals(ExitCode.success.code)); + verifyNever(() => logger.err(any())); + }), + ); + + test( + 'fails like very_good test when the merged coverage is below it', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + '--min-coverage', + '100', + '--show-uncovered', + ]); + + expect(result, equals(ExitCode.software.code)); + verify( + () => logger.err( + 'Expected coverage >= 100.00% but actual is 66.67%.', + ), + ).called(1); + verify(() => logger.err('Lines not covered:\n\t- lib/b.dart: 1')) + .called(1); + expect(_readOutput(), equals(_merged)); + }), + ); + + test( + 'ignores files matching --exclude-coverage', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + '--min-coverage', + '100', + '--exclude-coverage', + 'lib/b.dart', + ]); + + expect(result, equals(ExitCode.success.code)); + }), + ); + }); + + test( + '--show-uncovered logs uncovered lines when there is no threshold', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + '--show-uncovered', + ]); + + expect(result, equals(ExitCode.success.code)); + verify(() => logger.write('Lines not covered:\n\t- lib/b.dart: 1\n')) + .called(1); + }), + ); + + group('very_good.yaml', () { + test( + 'uses the test section', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + _writeFile( + 'very_good.yaml', + 'test:\n min_coverage: 100\n show_uncovered: true\n' + 'dart:\n test:\n min_coverage: 50\n', + ); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + ]); + + expect(result, equals(ExitCode.software.code)); + verify(() => logger.err('Lines not covered:\n\t- lib/b.dart: 1')) + .called(1); + }), + ); + + test( + 'falls back to the dart test section', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + _writeFile( + 'very_good.yaml', + 'dart:\n test:\n min_coverage: 100\n' + ' exclude_coverage: lib/b.dart\n', + ); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + }), + ); + + test( + 'is overridden by command line arguments', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + _writeFile('very_good.yaml', 'test:\n min_coverage: 100\n'); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + '--min-coverage', + '50', + ]); + + expect(result, equals(ExitCode.success.code)); + }), + ); + }); + }); +} diff --git a/test/src/commands/coverage/coverage_test.dart b/test/src/commands/coverage/coverage_test.dart new file mode 100644 index 000000000..74ee5db92 --- /dev/null +++ b/test/src/commands/coverage/coverage_test.dart @@ -0,0 +1,33 @@ +// Expected usage of the plugin will need to be adjacent strings due to format +// and also be longer than 80 chars. +// ignore_for_file: no_adjacent_strings_in_list, lines_longer_than_80_chars + +import 'package:mason/mason.dart'; +import 'package:test/test.dart'; + +import '../../../helpers/helpers.dart'; + +const _expectedCoverageUsage = [ + 'Command for working with coverage reports.\n' + '\n' + 'Usage: very_good coverage [arguments]\n' + '-h, --help Print this usage information.\n' + '\n' + 'Available subcommands:\n' + ' merge Merge lcov reports, from sharded or recursive test runs, and check the merged coverage.\n' + '\n' + 'Run "very_good help" to see global options.', +]; + +void main() { + group('coverage', () { + test( + 'help', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + final result = await commandRunner.run(['coverage', '--help']); + expect(printLogs, equals(_expectedCoverageUsage)); + expect(result, equals(ExitCode.success.code)); + }), + ); + }); +} From 56de5d1f7ee677a0c5a622135e6726ea06deae76 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 15:56:06 +0200 Subject: [PATCH 05/14] feat: discover lcov reports when coverage merge gets no files After 'very_good test --recursive --coverage' every package has its own coverage/lcov.info. With no arguments, 'coverage merge' now finds them the same way --recursive finds packages and rebases their relative source paths onto their package, so lib/a.dart from two packages stay distinct. The --output report is skipped, with a warning, so merging again doesn't count the previous merge. --- lib/src/cli/lcov_merger.dart | 17 +++ lib/src/commands/coverage/commands/merge.dart | 46 ++++++-- .../coverage/commands/merge_test.dart | 106 +++++++++++++++++- 3 files changed, 157 insertions(+), 12 deletions(-) diff --git a/lib/src/cli/lcov_merger.dart b/lib/src/cli/lcov_merger.dart index d576c8ce4..11a1e7cf0 100644 --- a/lib/src/cli/lcov_merger.dart +++ b/lib/src/cli/lcov_merger.dart @@ -215,6 +215,23 @@ List normalizeLcovRecords( ]; } +/// The directories of the packages under [cwd], relative to it, that have a +/// `coverage/lcov.info` report, as left behind by +/// `very_good test --recursive --coverage`. +/// +/// Packages are found the same way as with `--recursive`, so platform, build +/// and tool directories are skipped. +List discoverLcovPackages(String cwd) => Directory(cwd) + .listSync(recursive: true) + .where(_isPubspec) + .map((pubspec) => p.relative(pubspec.parent.path, from: cwd)) + .where((package) => !p.split(package).any(_ignoredDirectories.contains)) + .where( + (package) => + File(p.join(cwd, package, 'coverage', 'lcov.info')).existsSync(), + ) + .sorted(); + /// Merges [records] that describe the same source file, summing their hits. /// /// The result keeps the order in which each file first appears. diff --git a/lib/src/commands/coverage/commands/merge.dart b/lib/src/commands/coverage/commands/merge.dart index e26ac18f5..79b401b83 100644 --- a/lib/src/commands/coverage/commands/merge.dart +++ b/lib/src/commands/coverage/commands/merge.dart @@ -91,7 +91,9 @@ class CoverageMergeCommand extends Command { ); } - final inputs = _resolveInputs(argResults.rest, cwd: cwd); + final inputs = argResults.rest.isEmpty + ? _discoverInputs(cwd: cwd, output: output) + : _resolveInputs(argResults.rest, cwd: cwd); final externalPaths = {}; final records = [ for (final input in inputs) @@ -150,13 +152,6 @@ class CoverageMergeCommand extends Command { List args, { required String cwd, }) { - if (args.isEmpty) { - throw _MergeError( - 'No lcov reports to merge. Pass the lcov files or globs to merge.', - ExitCode.usage.code, - ); - } - final paths = {}; for (final arg in args) { if (File(p.join(cwd, arg)).existsSync()) { @@ -190,6 +185,41 @@ class CoverageMergeCommand extends Command { return [for (final path in paths) (path: path, packagePath: null)]; } + /// The `coverage/lcov.info` report of every package under [cwd], with their + /// relative source paths rebased onto their package, so the same + /// `lib/a.dart` from two packages stays distinct. + /// + /// The [output] report is left out, so that merging again doesn't count the + /// previous merge. + List<({String path, String? packagePath})> _discoverInputs({ + required String cwd, + required String output, + }) { + final outputPath = p.join(cwd, output); + final inputs = <({String path, String? packagePath})>[]; + for (final package in discoverLcovPackages(cwd)) { + final path = p.normalize(p.join(package, 'coverage', 'lcov.info')); + if (p.equals(p.join(cwd, path), outputPath)) { + _logger.warn( + 'Skipping $path, since it is the --output report. Pass a different ' + '--output to merge it too.', + ); + continue; + } + inputs.add((path: path, packagePath: package)); + } + + if (inputs.isEmpty) { + throw _MergeError( + 'No lcov reports found in $cwd. Run ' + '"very_good test --recursive --coverage" first, or pass the lcov files ' + 'or globs to merge.', + ExitCode.noInput.code, + ); + } + return inputs; + } + List _parse(String path) { try { return parseLcov(File(path).readAsStringSync()); diff --git a/test/src/commands/coverage/commands/merge_test.dart b/test/src/commands/coverage/commands/merge_test.dart index a85be45a5..6c931f9dd 100644 --- a/test/src/commands/coverage/commands/merge_test.dart +++ b/test/src/commands/coverage/commands/merge_test.dart @@ -231,18 +231,116 @@ void main() { }), ); - group('fails', () { + group('without lcov files', () { + void writePackage(String path, String lcov) { + _writeFile(p.join(path, 'pubspec.yaml'), 'name: ${p.basename(path)}'); + _writeFile(p.join(path, 'coverage', 'lcov.info'), lcov); + } + test( - 'when no lcov file is given', + 'merges the report of every package, keeping their files distinct', withRunner((commandRunner, logger, pubUpdater, printLogs) async { _enterTempDirectory(); + writePackage(p.join('packages', 'foo'), _shard1); + writePackage(p.join('packages', 'bar'), _shard2); + for (final ignored in ['build', '.dart_tool', '.fvm', 'ios']) { + writePackage(p.join('packages', 'foo', ignored, 'pkg'), _shard1); + } + // A package without tests leaves no report behind. + _writeFile(p.join('packages', 'baz', 'pubspec.yaml'), 'name: baz'); final result = await commandRunner.run(['coverage', 'merge']); - expect(result, equals(ExitCode.usage.code)); + expect(result, equals(ExitCode.success.code)); + expect( + _readOutput(), + equals(''' +SF:packages/bar/lib/a.dart +DA:1,0 +DA:2,2 +LF:2 +LH:1 +end_of_record +SF:packages/foo/lib/a.dart +DA:1,1 +DA:2,0 +LF:2 +LH:1 +end_of_record +SF:packages/foo/lib/b.dart +DA:1,0 +LF:1 +LH:0 +end_of_record +'''), + ); + verify( + () => + logger.info('Merged 2 lcov report(s) into coverage/lcov.info'), + ).called(1); + }), + ); + + test( + 'skips the --output report', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + writePackage('.', 'SF:stale.dart\nDA:1,1\nend_of_record\n'); + writePackage('foo', _shard2); + + final result = await commandRunner.run(['coverage', 'merge']); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), startsWith('SF:foo/lib/a.dart\n')); + expect(_readOutput(), isNot(contains('stale.dart'))); + verify( + () => logger.warn( + 'Skipping ${p.join('coverage', 'lcov.info')}, since it is the ' + '--output report. Pass a different --output to merge it too.', + ), + ).called(1); + }), + ); + + test( + 'merges the root report into a different --output', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + writePackage('.', _shard1); + writePackage('foo', _shard2); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + '-o', + 'merged.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect( + _readOutput('merged.info'), + allOf(contains('SF:lib/a.dart\n'), contains('SF:foo/lib/a.dart\n')), + ); + }), + ); + }); + + group('fails', () { + test( + 'when no lcov file is given and none is found', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + final cwd = _enterTempDirectory(); + // A package without a report. + _writeFile('pubspec.yaml', 'name: root'); + + final result = await commandRunner.run(['coverage', 'merge']); + + expect(result, equals(ExitCode.noInput.code)); verify( () => logger.err( - 'No lcov reports to merge. Pass the lcov files or globs to merge.', + 'No lcov reports found in ${cwd.path}. Run ' + '"very_good test --recursive --coverage" first, or pass the lcov ' + 'files or globs to merge.', ), ).called(1); }), From a922467bd5efcdb15d67bd2da81463143cb3d8c9 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 15:57:02 +0200 Subject: [PATCH 06/14] feat(test): point sharded coverage users at coverage merge The --min-coverage + sharding error now names 'very_good coverage merge' instead of the vague 'merge the lcov reports'. A min_coverage inherited from very_good.yaml is still skipped while sharding, but no longer silently: 'test' and 'dart test' warn once per run, without changing the exit code. --- lib/src/cli/test_cli_runner.dart | 12 +++++- .../dart/commands/dart_test_command.dart | 4 ++ lib/src/commands/test/test.dart | 4 ++ .../dart/commands/dart_test_test.dart | 38 ++++++++++++++++++- test/src/commands/test/test_test.dart | 38 ++++++++++++++++++- 5 files changed, 90 insertions(+), 6 deletions(-) diff --git a/lib/src/cli/test_cli_runner.dart b/lib/src/cli/test_cli_runner.dart index 8c7084e34..cf0a5c8ef 100644 --- a/lib/src/cli/test_cli_runner.dart +++ b/lib/src/cli/test_cli_runner.dart @@ -135,13 +135,21 @@ class TestCLIRunner { // shard and enforce the threshold in a separate job instead. if (rawMinCoverage != null) { return '--min-coverage cannot be combined with sharding. Collect ' - 'coverage per shard with --coverage, merge the lcov reports, then ' - 'check the threshold in a separate job.'; + 'coverage per shard with --coverage, then enforce the threshold on ' + 'the merged reports with "very_good coverage merge".'; } return null; } + /// The warning shown when a sharded run doesn't enforce the [minCoverage] + /// set in `very_good.yaml`, since each shard only covers part of the suite. + static String shardedMinCoverageWarning(String minCoverage) => + 'min_coverage ($minCoverage%) from very_good.yaml is not enforced while ' + 'sharding, since each shard only covers part of the suite.\n' + 'Enforce it on the merged report instead: ' + 'very_good coverage merge '; + /// Run tests (`flutter test`). /// Returns a list of exit codes for each test process. static Future> test({ diff --git a/lib/src/commands/dart/commands/dart_test_command.dart b/lib/src/commands/dart/commands/dart_test_command.dart index ebcfbf509..5302806c0 100644 --- a/lib/src/commands/dart/commands/dart_test_command.dart +++ b/lib/src/commands/dart/commands/dart_test_command.dart @@ -473,6 +473,10 @@ This command should be run from the root of your Dart project.'''); final minCoverage = options.totalShards == null ? options.minCoverage : null; + final configMinCoverage = config.dart.test.minCoverage; + if (options.totalShards != null && configMinCoverage != null) { + _logger.warn(TestCLIRunner.shardedMinCoverageWarning(configMinCoverage)); + } try { final results = await _dartTest( diff --git a/lib/src/commands/test/test.dart b/lib/src/commands/test/test.dart index 8cbe2ea1f..b059d979b 100644 --- a/lib/src/commands/test/test.dart +++ b/lib/src/commands/test/test.dart @@ -552,6 +552,10 @@ This command should be run from the root of your Flutter project.'''); final minCoverage = options.totalShards == null ? options.minCoverage : null; + final configMinCoverage = config.test.minCoverage; + if (options.totalShards != null && configMinCoverage != null) { + _logger.warn(TestCLIRunner.shardedMinCoverageWarning(configMinCoverage)); + } try { final results = await _flutterTest( diff --git a/test/src/commands/dart/commands/dart_test_test.dart b/test/src/commands/dart/commands/dart_test_test.dart index 974fc0213..00e103604 100644 --- a/test/src/commands/dart/commands/dart_test_test.dart +++ b/test/src/commands/dart/commands/dart_test_test.dart @@ -1042,9 +1042,43 @@ void main() { stderr: logger.err, ), ).called(1); + verify( + () => logger.warn(TestCLIRunner.shardedMinCoverageWarning('90')), + ).called(1); }, ); + test( + 'does not warn about min_coverage when sharding without one', + () async { + withShards('1', '3'); + + final result = await testCommand.run(); + + expect(result, equals(ExitCode.success.code)); + verifyNever(() => logger.warn(any())); + }, + ); + + test('does not warn about min_coverage from very_good.yaml when not ' + 'sharding', () async { + final tempDirectory = Directory.systemTemp.createTempSync(); + addTearDown(() { + Directory.current = cwd; + tempDirectory.deleteSync(recursive: true); + }); + Directory.current = tempDirectory.path; + File(path.join(tempDirectory.path, 'pubspec.yaml')).createSync(); + File(path.join(tempDirectory.path, 'very_good.yaml')) + .writeAsStringSync('dart:\n test:\n min_coverage: 90\n'); + when(() => argResults.wasParsed(any())).thenReturn(false); + + final result = await testCommand.run(); + + expect(result, equals(ExitCode.success.code)); + verifyNever(() => logger.warn(any())); + }); + test('fails when sharding is combined with --min-coverage', () async { withShards('1', '3'); when(() => argResults['min-coverage']).thenReturn('100'); @@ -1055,8 +1089,8 @@ void main() { verify( () => logger.err( '--min-coverage cannot be combined with sharding. Collect ' - 'coverage per shard with --coverage, merge the lcov reports, ' - 'then check the threshold in a separate job.', + 'coverage per shard with --coverage, then enforce the threshold ' + 'on the merged reports with "very_good coverage merge".', ), ).called(1); }); diff --git a/test/src/commands/test/test_test.dart b/test/src/commands/test/test_test.dart index 4233e6a40..b01a47494 100644 --- a/test/src/commands/test/test_test.dart +++ b/test/src/commands/test/test_test.dart @@ -1211,9 +1211,43 @@ void main() { stderr: logger.err, ), ).called(1); + verify( + () => logger.warn(TestCLIRunner.shardedMinCoverageWarning('90')), + ).called(1); }, ); + test( + 'does not warn about min_coverage when sharding without one', + () async { + withShards('1', '3'); + + final result = await testCommand.run(); + + expect(result, equals(ExitCode.success.code)); + verifyNever(() => logger.warn(any())); + }, + ); + + test('does not warn about min_coverage from very_good.yaml when not ' + 'sharding', () async { + final tempDirectory = Directory.systemTemp.createTempSync(); + addTearDown(() { + Directory.current = cwd; + tempDirectory.deleteSync(recursive: true); + }); + Directory.current = tempDirectory.path; + File(path.join(tempDirectory.path, 'pubspec.yaml')).createSync(); + File(path.join(tempDirectory.path, 'very_good.yaml')) + .writeAsStringSync('test:\n min_coverage: 90\n'); + when(() => argResults.wasParsed(any())).thenReturn(false); + + final result = await testCommand.run(); + + expect(result, equals(ExitCode.success.code)); + verifyNever(() => logger.warn(any())); + }); + test('fails when sharding is combined with --min-coverage', () async { withShards('1', '3'); when(() => argResults['min-coverage']).thenReturn('100'); @@ -1224,8 +1258,8 @@ void main() { verify( () => logger.err( '--min-coverage cannot be combined with sharding. Collect ' - 'coverage per shard with --coverage, merge the lcov reports, ' - 'then check the threshold in a separate job.', + 'coverage per shard with --coverage, then enforce the threshold ' + 'on the merged reports with "very_good coverage merge".', ), ).called(1); }); From e26fa142b8bebba17c1af4b74e0b3b61e8facae8 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 16:00:58 +0200 Subject: [PATCH 07/14] docs: document coverage merge and the sharded coverage workflow The sharding caution told users to merge reports with lcov and pass --ignore-errors empty. It now points at 'very_good coverage merge', documents the very_good.yaml min_coverage warning, and shows the full matrix, upload, download and merge workflow. A new Coverage page covers the command, its merge semantics and the config keys it reads. --- README.md | 12 +++ site/docs/commands/coverage.md | 131 +++++++++++++++++++++++++++++++++ site/docs/commands/test.md | 54 ++++++++++++-- site/docs/configuration.md | 8 ++ 4 files changed, 197 insertions(+), 8 deletions(-) create mode 100644 site/docs/commands/coverage.md diff --git a/README.md b/README.md index 2006eb3d7..20a1bcb4b 100644 --- a/README.md +++ b/README.md @@ -142,6 +142,18 @@ very_good test -r very_good test --platform chrome ``` +### [`very_good coverage merge`](https://cli.vgv.dev/docs/commands/coverage) + +Merge the lcov reports of sharded or recursive test runs, and enforce a minimum coverage on the result. + +```sh +# Merge the reports of every shard and enforce 100% coverage +very_good coverage merge 'shards/*/lcov.info' --min-coverage 100 + +# Merge the reports of every package, after `very_good test -r --coverage` +very_good coverage merge +``` + ### [`very_good packages get`](https://cli.vgv.dev/docs/commands/get_pkgs) Get packages in a Dart or Flutter project. diff --git a/site/docs/commands/coverage.md b/site/docs/commands/coverage.md new file mode 100644 index 000000000..2d6c531ed --- /dev/null +++ b/site/docs/commands/coverage.md @@ -0,0 +1,131 @@ +--- +sidebar_position: 1.5 +--- + +# Coverage 📊 + +Merge the lcov reports of sharded or recursive test runs, and enforce a minimum +coverage on the result with `very_good coverage merge`. + +## Usage + +```sh +very_good coverage merge [lcov files or globs] [arguments] +-h, --help Print this usage information. +-o, --output= The path to write the merged lcov report to. + (defaults to "coverage/lcov.info") + --min-coverage Whether to enforce a minimum coverage percentage. + --exclude-coverage A glob which will be used to exclude files that match from the coverage (e.g. '**/*.g.dart'). + --show-uncovered Whether to show uncovered lines when coverage is below 100%. + +Run "very_good help" to see global options. +``` + +## Merging reports + +When you [shard your tests](test.md#sharding-tests-across-ci-runners) or run +them with `--recursive`, every shard or package writes its own +`coverage/lcov.info`. None of them reflects the whole suite, so +`very_good coverage merge` combines them into a single report and checks it the +same way `very_good test --min-coverage` does, with the same output and exit +code. + +```sh +# Merge the reports downloaded from each shard +very_good coverage merge 'shards/*/lcov.info' --min-coverage 100 + +# Merge the reports of every package, after `very_good test -r --coverage` +very_good coverage merge + +# Write the merged report somewhere else +very_good coverage merge 'shards/*/lcov.info' --output coverage/merged.info +``` + +Each argument is either the path to an lcov file or a glob, relative to the +current directory. The CLI expands globs itself, so quote them to get the same +result in bash, zsh, PowerShell, and `cmd`. Use `/` as the separator in globs, +on every platform. A glob that matches no file is an error. + +Without arguments, the command looks for the `coverage/lcov.info` of every +package under the current directory, skipping the same directories as +`--recursive` (such as `build`, `.dart_tool`, and the platform folders). The +source paths of each report are prefixed with the path of its package, so +`lib/main.dart` from two packages counts as two files. The `--output` report is +never merged into itself; pass a different `--output` when the root package has +its own tests. + +### How reports are merged + +Reports are merged by source file: + +- Hits are summed per line, per function, and per branch. +- The `LF`, `LH`, `FNF`, `FNH`, `BRF`, and `BRH` totals are recomputed from + the merged hits. +- Separators in source paths become `/`, and absolute paths under the current + directory become relative, so reports from Linux, macOS, and Windows runners + merge together. Absolute paths outside of the current directory are kept as + they are, with a warning. +- Files padded with 0% coverage by `--collect-coverage-from all` are covered by + the hits of the other reports. +- Empty reports, such as the one a shard without tests writes, are accepted. + +## Configuration + +The command has no section of its own in +[`very_good.yaml`](../configuration.md). When a flag isn't passed, it reads the +`min_coverage`, `exclude_coverage`, and `show_uncovered` values from the +`test` section, and then from the `dart.test` section. This lets the threshold +you already enforce in un-sharded runs apply to the merged report. + +## Example CI workflow + +The following GitHub Actions workflow runs the tests on 3 runners, then +enforces 100% coverage on the merged report: + +```yaml +jobs: + test: + runs-on: ubuntu-latest + strategy: + matrix: + shard: [1, 2, 3] + steps: + - uses: actions/checkout@v4 + - uses: subosito/flutter-action@v2 + - run: dart pub global activate very_good_cli + - run: very_good test --coverage --shard-index ${{ matrix.shard }} --total-shards 3 + - uses: actions/upload-artifact@v4 + with: + name: coverage-${{ matrix.shard }} + path: coverage/lcov.info + + coverage: + needs: test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: dart-lang/setup-dart@v1 + - run: dart pub global activate very_good_cli + - uses: actions/download-artifact@v4 + with: + pattern: coverage-* + path: shards + - run: very_good coverage merge 'shards/*/lcov.info' --min-coverage 100 +``` + +When the shards also run with `--recursive`, each shard leaves one report per +package. Merge them in the shard job first, so the uploaded report has its +source paths relative to the root, and enforce the threshold in the final job +only. Pass `--min-coverage 0` so that a `min_coverage` from `very_good.yaml` +isn't enforced on the partial report of a single shard: + +```yaml +- run: very_good test -r --coverage --shard-index ${{ matrix.shard }} --total-shards 3 +- run: very_good coverage merge --output shard.info --min-coverage 0 +- uses: actions/upload-artifact@v4 + with: + name: coverage-${{ matrix.shard }} + path: shard.info +``` + +The final job then runs `very_good coverage merge 'shards/*/shard.info'`. diff --git a/site/docs/commands/test.md b/site/docs/commands/test.md index 6c9447fff..165917aea 100644 --- a/site/docs/commands/test.md +++ b/site/docs/commands/test.md @@ -116,16 +116,54 @@ given runner executes a slice of every package. Balance therefore degrades when a workspace contains many packages with few tests each. :::caution -Sharding cannot be combined with `--min-coverage`, and a `min_coverage` set in -`very_good.yaml` is ignored while sharding. Each shard only exercises a subset -of the codebase, so its coverage is not representative of the whole suite. -Collect coverage per shard with `--coverage`, merge the resulting lcov reports -once every shard has finished, and enforce the threshold on the merged report -in a separate job. A shard without tests still writes an empty -`coverage/lcov.info`; pass `--ignore-errors empty` to `lcov` when merging so it -is accepted. +Sharding cannot be combined with `--min-coverage`. Each shard only exercises a +subset of the codebase, so its coverage is not representative of the whole +suite. Collect coverage per shard with `--coverage`, and enforce the threshold +on the merged reports with +[`very_good coverage merge`](coverage.md) once every shard has finished. ::: +A `min_coverage` set in `very_good.yaml` is not enforced while sharding either. +Instead of failing, the command warns once per run, and the exit code only +reflects the test results: + +``` +[WARN] min_coverage (100%) from very_good.yaml is not enforced while sharding, since each shard only covers part of the suite. +Enforce it on the merged report instead: very_good coverage merge +``` + +`very_good coverage merge` reads the same `min_coverage`, so the merge job +enforces it without repeating the threshold. The full workflow, where a matrix +of shards uploads its reports and a final job downloads and merges them, looks +like this: + +```yaml +jobs: + test: + strategy: + matrix: + shard: [1, 2, 3] + steps: + - run: very_good test --coverage --shard-index ${{ matrix.shard }} --total-shards 3 + - uses: actions/upload-artifact@v4 + with: + name: coverage-${{ matrix.shard }} + path: coverage/lcov.info + + coverage: + needs: test + steps: + - uses: actions/download-artifact@v4 + with: + pattern: coverage-* + path: shards + - run: very_good coverage merge 'shards/*/lcov.info' +``` + +A shard without tests still writes an empty `coverage/lcov.info`, which +`very_good coverage merge` accepts. See [Coverage](coverage.md) for the +complete workflow, including `--recursive` runs. + :::info Sharding requires the test optimizer, so it is rejected whenever the optimizer is off: with `--no-optimization`, `--platform`, `--update-goldens`, or when diff --git a/site/docs/configuration.md b/site/docs/configuration.md index 9105e5167..850271ff1 100644 --- a/site/docs/configuration.md +++ b/site/docs/configuration.md @@ -213,6 +213,14 @@ dart: | `check_ignore` | `bool` | Whether to respect coverage ignore comments (e.g. `// coverage:ignore-line`). | | `file_reporter` | `string` | Additional file reporter as `:` (e.g. `json:reports/tests.json`). | +### `coverage merge` + +[`very_good coverage merge`](commands/coverage.md) has no section of its own. +When a flag isn't passed, it reads `min_coverage`, `exclude_coverage`, and +`show_uncovered` from the `test` section, and then from the `dart.test` +section, so the threshold you enforce in un-sharded runs also applies to the +merged report. + ### `packages.get` Defaults for [`very_good packages get`](commands/get_pkgs.md). From 0cb48888a87c34cac05104fbb7c26d2fb247e615 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 16:03:26 +0200 Subject: [PATCH 08/14] test(e2e): cover merging sharded coverage reports Runs a Flutter fixture unsharded and in 2 shards, then checks that 'coverage merge' rebuilds the unsharded report and fails a 100% threshold on it like 'very_good test' would. --- .github/workflows/e2e.yaml | 3 + e2e/analysis_options.yaml | 1 + .../coverage/merge/fixture/lib/src/add.dart | 9 ++ .../merge/fixture/lib/src/multiply.dart | 9 ++ .../merge/fixture/lib/src/subtract.dart | 9 ++ .../coverage/merge/fixture/pubspec.yaml | 15 +++ .../coverage/merge/fixture/test/add_test.dart | 8 ++ .../merge/fixture/test/multiply_test.dart | 8 ++ .../merge/fixture/test/subtract_test.dart | 8 ++ .../commands/coverage/merge/merge_test.dart | 96 +++++++++++++++++++ 10 files changed, 166 insertions(+) create mode 100644 e2e/test/commands/coverage/merge/fixture/lib/src/add.dart create mode 100644 e2e/test/commands/coverage/merge/fixture/lib/src/multiply.dart create mode 100644 e2e/test/commands/coverage/merge/fixture/lib/src/subtract.dart create mode 100644 e2e/test/commands/coverage/merge/fixture/pubspec.yaml create mode 100644 e2e/test/commands/coverage/merge/fixture/test/add_test.dart create mode 100644 e2e/test/commands/coverage/merge/fixture/test/multiply_test.dart create mode 100644 e2e/test/commands/coverage/merge/fixture/test/subtract_test.dart create mode 100644 e2e/test/commands/coverage/merge/merge_test.dart diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 908549ea4..7487d64c8 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -37,6 +37,9 @@ jobs: - test/commands/test/spaced_golden_file_name/spaced_golden_file_name_test.dart - test/commands/test/very_good_config/very_good_config_test.dart + # E2E tests for the coverage command + - test/commands/coverage/merge/merge_test.dart + # E2E tests for the create command - test/commands/create/flutter_app/core_test.dart - test/commands/create/dart_cli/dart_cli_test.dart diff --git a/e2e/analysis_options.yaml b/e2e/analysis_options.yaml index 262b2764e..b62dfa579 100644 --- a/e2e/analysis_options.yaml +++ b/e2e/analysis_options.yaml @@ -1,6 +1,7 @@ include: package:very_good_analysis/analysis_options.yaml analyzer: exclude: + - test/commands/coverage/merge/fixture/** - test/commands/test/** - build/** - android/** diff --git a/e2e/test/commands/coverage/merge/fixture/lib/src/add.dart b/e2e/test/commands/coverage/merge/fixture/lib/src/add.dart new file mode 100644 index 000000000..ee71a259a --- /dev/null +++ b/e2e/test/commands/coverage/merge/fixture/lib/src/add.dart @@ -0,0 +1,9 @@ +/// Returns [a] + [b]. +int add(int a, int b) { + return a + b; +} + +/// Applies [add] to every value of [values], never called by the tests. +int addAll(List values) { + return values.reduce(add); +} diff --git a/e2e/test/commands/coverage/merge/fixture/lib/src/multiply.dart b/e2e/test/commands/coverage/merge/fixture/lib/src/multiply.dart new file mode 100644 index 000000000..bb04b58b8 --- /dev/null +++ b/e2e/test/commands/coverage/merge/fixture/lib/src/multiply.dart @@ -0,0 +1,9 @@ +/// Returns [a] * [b]. +int multiply(int a, int b) { + return a * b; +} + +/// Applies [multiply] to every value of [values], never called by the tests. +int multiplyAll(List values) { + return values.reduce(multiply); +} diff --git a/e2e/test/commands/coverage/merge/fixture/lib/src/subtract.dart b/e2e/test/commands/coverage/merge/fixture/lib/src/subtract.dart new file mode 100644 index 000000000..2a3c4fc48 --- /dev/null +++ b/e2e/test/commands/coverage/merge/fixture/lib/src/subtract.dart @@ -0,0 +1,9 @@ +/// Returns [a] - [b]. +int subtract(int a, int b) { + return a - b; +} + +/// Applies [subtract] to every value of [values], never called by the tests. +int subtractAll(List values) { + return values.reduce(subtract); +} diff --git a/e2e/test/commands/coverage/merge/fixture/pubspec.yaml b/e2e/test/commands/coverage/merge/fixture/pubspec.yaml new file mode 100644 index 000000000..d0187ba6f --- /dev/null +++ b/e2e/test/commands/coverage/merge/fixture/pubspec.yaml @@ -0,0 +1,15 @@ +name: coverage_merge_fixture +description: Fixture for testing the merge of sharded coverage reports. +version: 0.1.0+1 +publish_to: none + +environment: + sdk: ^3.13.0 + +dependencies: + flutter: + sdk: flutter + +dev_dependencies: + flutter_test: + sdk: flutter diff --git a/e2e/test/commands/coverage/merge/fixture/test/add_test.dart b/e2e/test/commands/coverage/merge/fixture/test/add_test.dart new file mode 100644 index 000000000..2e0c3237a --- /dev/null +++ b/e2e/test/commands/coverage/merge/fixture/test/add_test.dart @@ -0,0 +1,8 @@ +import 'package:coverage_merge_fixture/src/add.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + test('add', () { + expect(add(4, 2), equals(4 + 2)); + }); +} diff --git a/e2e/test/commands/coverage/merge/fixture/test/multiply_test.dart b/e2e/test/commands/coverage/merge/fixture/test/multiply_test.dart new file mode 100644 index 000000000..b0a66621d --- /dev/null +++ b/e2e/test/commands/coverage/merge/fixture/test/multiply_test.dart @@ -0,0 +1,8 @@ +import 'package:coverage_merge_fixture/src/multiply.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + test('multiply', () { + expect(multiply(4, 2), equals(4 * 2)); + }); +} diff --git a/e2e/test/commands/coverage/merge/fixture/test/subtract_test.dart b/e2e/test/commands/coverage/merge/fixture/test/subtract_test.dart new file mode 100644 index 000000000..042e1179e --- /dev/null +++ b/e2e/test/commands/coverage/merge/fixture/test/subtract_test.dart @@ -0,0 +1,8 @@ +import 'package:coverage_merge_fixture/src/subtract.dart'; +import 'package:flutter_test/flutter_test.dart'; + +void main() { + test('subtract', () { + expect(subtract(4, 2), equals(4 - 2)); + }); +} diff --git a/e2e/test/commands/coverage/merge/merge_test.dart b/e2e/test/commands/coverage/merge/merge_test.dart new file mode 100644 index 000000000..befdc76cf --- /dev/null +++ b/e2e/test/commands/coverage/merge/merge_test.dart @@ -0,0 +1,96 @@ +import 'package:mason/mason.dart'; +import 'package:mocktail/mocktail.dart'; +import 'package:path/path.dart' as path; +import 'package:test/test.dart'; +import 'package:universal_io/io.dart'; + +import '../../../../helpers/helpers.dart'; + +/// The lines of the lcov report at [filePath], sorted so reports listing the +/// same files in a different order compare equal. +List _sortedLines(String filePath) => + File(filePath).readAsLinesSync()..sort(); + +void main() { + test( + 'merges sharded coverage into the coverage of an unsharded run', + timeout: const Timeout(Duration(minutes: 5)), + withRunner((commandRunner, logger, updater, logs, progressLogs) async { + final tempDirectory = Directory.systemTemp.createTempSync('merge'); + addTearDown(() => tempDirectory.deleteSync(recursive: true)); + + await copyDirectory( + Directory( + path.join( + Directory.current.path, + 'test/commands/coverage/merge/fixture', + ), + ), + tempDirectory, + ); + await expectSuccessfulProcessResult('flutter', [ + 'pub', + 'get', + ], workingDirectory: tempDirectory.path); + + final cwd = Directory.current; + Directory.current = tempDirectory; + addTearDown(() => Directory.current = cwd); + + final lcovPath = path.join('coverage', 'lcov.info'); + + await expectLater( + commandRunner.run(['test', '--coverage']), + completion(equals(ExitCode.success.code)), + ); + final unshardedPath = File(lcovPath).copySync('unsharded.info').path; + + for (final shard in ['1', '2']) { + await expectLater( + commandRunner.run([ + 'test', + '--coverage', + '--shard-index', + shard, + '--total-shards', + '2', + ]), + completion(equals(ExitCode.success.code)), + ); + final shardPath = path.join('shards', shard, 'lcov.info'); + Directory(path.dirname(shardPath)).createSync(recursive: true); + File(lcovPath).copySync(shardPath); + } + + await expectLater( + commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + '--output', + 'merged.info', + ]), + completion(equals(ExitCode.success.code)), + ); + expect(_sortedLines('merged.info'), equals(_sortedLines(unshardedPath))); + + await expectLater( + commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + '--output', + 'merged.info', + '--min-coverage', + '100', + ]), + completion(equals(ExitCode.software.code)), + ); + verify( + () => logger.err( + any(that: startsWith('Expected coverage >= 100.00% but actual is ')), + ), + ).called(1); + }), + ); +} From 4224f34ebd08f9c4d320a2f2b19e6c45c0f02f22 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 18:32:42 +0200 Subject: [PATCH 09/14] refactor(cli): check coverage from CoverageMetrics checkCoverage now takes the computed CoverageMetrics instead of lcov_parser records, so callers choose how the metrics are built. CoverageMetrics.fromLcovRecords folds per-file summaries through a private _fromFiles, ready to be shared by other record sources. Also renames coverage_reporter.dart to coverage_check.dart to match its only function, and replaces the list-based branch comparator in LcovRecord.toLcov with a plain _compareBranches helper. --- lib/src/cli/cli.dart | 2 +- ...rage_reporter.dart => coverage_check.dart} | 19 ++--- lib/src/cli/flutter_cli.dart | 80 +++++++++---------- lib/src/cli/lcov_merger.dart | 17 ++-- lib/src/cli/test_cli_runner.dart | 6 +- lib/src/commands/coverage/commands/merge.dart | 6 +- ...ter_test.dart => coverage_check_test.dart} | 51 +++++------- 7 files changed, 85 insertions(+), 96 deletions(-) rename lib/src/cli/{coverage_reporter.dart => coverage_check.dart} (59%) rename test/src/cli/{coverage_reporter_test.dart => coverage_check_test.dart} (67%) diff --git a/lib/src/cli/cli.dart b/lib/src/cli/cli.dart index 687350f4f..a28931b84 100644 --- a/lib/src/cli/cli.dart +++ b/lib/src/cli/cli.dart @@ -16,7 +16,7 @@ import 'package:very_good_test_runner/very_good_test_runner.dart'; export 'package:very_good_cli/src/test_optimizer/test_optimizer.dart'; -part 'coverage_reporter.dart'; +part 'coverage_check.dart'; part 'dart_cli.dart'; part 'flutter_cli.dart'; part 'git_cli.dart'; diff --git a/lib/src/cli/coverage_reporter.dart b/lib/src/cli/coverage_check.dart similarity index 59% rename from lib/src/cli/coverage_reporter.dart rename to lib/src/cli/coverage_check.dart index eff2fcaac..be2de9006 100644 --- a/lib/src/cli/coverage_reporter.dart +++ b/lib/src/cli/coverage_check.dart @@ -1,29 +1,20 @@ part of 'cli.dart'; -/// Checks the coverage of [records] against [minCoverage]. -/// -/// Files matching [excludeFromCoverage] (space separated globs) are left out -/// of the measurement. +/// Checks the coverage [metrics] against [minCoverage]. /// /// Throws [MinCoverageNotMet] when the coverage is below [minCoverage], /// carrying the uncovered lines when [showUncovered] is set. Otherwise, when /// [showUncovered] is set and some lines are not covered, they are written to /// [stdout] as informational output. void checkCoverage( - List records, { + CoverageMetrics metrics, { double? minCoverage, bool showUncovered = false, - String? excludeFromCoverage, void Function(String)? stdout, }) { - final coverageMetrics = CoverageMetrics.fromLcovRecords( - records, - excludeFromCoverage: excludeFromCoverage, - ); - final coverage = coverageMetrics.percentage; - final uncoveredLines = - showUncovered && coverageMetrics.uncoveredLines.isNotEmpty - ? coverageMetrics.uncoveredLines + final coverage = metrics.percentage; + final uncoveredLines = showUncovered && metrics.uncoveredLines.isNotEmpty + ? metrics.uncoveredLines : null; if (minCoverage != null && coverage < minCoverage) { diff --git a/lib/src/cli/flutter_cli.dart b/lib/src/cli/flutter_cli.dart index 2a922c8f9..eb7e8c859 100644 --- a/lib/src/cli/flutter_cli.dart +++ b/lib/src/cli/flutter_cli.dart @@ -69,51 +69,49 @@ class CoverageMetrics { }); /// Generate coverage metrics from a list of lcov records. - factory fromLcovRecords(List records, {String? excludeFromCoverage}) { - final globs = []; + factory fromLcovRecords( + List records, { + String? excludeFromCoverage, + }) => ._fromFiles([ + for (final record in records) + ( + file: record.file, + found: record.lines?.found ?? 0, + hit: record.lines?.hit ?? 0, + uncovered: [ + for (final line in [...?record.lines?.details]) + if ((line.hit ?? 1) == 0 && line.line != null) line.line!, + ], + ), + ], excludeFromCoverage: excludeFromCoverage); - if (excludeFromCoverage != null && excludeFromCoverage.isNotEmpty) { - for (final glob in excludeFromCoverage.trim().split(' ')) { - if (glob.isNotEmpty) globs.add(Glob(glob)); + factory _fromFiles( + List<({String? file, int found, int hit, List uncovered})> files, { + String? excludeFromCoverage, + }) { + final globs = [ + for (final glob in (excludeFromCoverage ?? '').trim().split(' ')) + if (glob.isNotEmpty) Glob(glob), + ]; + + var totalFound = 0; + var totalHits = 0; + final uncoveredLines = >{}; + for (final (:file, :found, :hit, :uncovered) in files) { + if (file != null && globs.any((glob) => glob.matches(file))) continue; + + totalFound += found; + totalHits += hit; + if (file != null && uncovered.isNotEmpty) { + (uncoveredLines[file] ??= []).addAll(uncovered); } } - return records.fold(const CoverageMetrics(), ( - current, - record, - ) { - final found = record.lines?.found ?? 0; - final hit = record.lines?.hit ?? 0; - if (globs.isNotEmpty && record.file != null) { - for (final glob in globs) { - if (glob.matches(record.file!)) return current; - } - } - - final file = record.file; - final details = record.lines?.details; - final uncoveredLines = Map>.from( - current.uncoveredLines, - ); - - if (file != null && details != null) { - for (final line in details) { - if ((line.hit ?? 1) == 0 && line.line != null) { - uncoveredLines.update( - file, - (lines) => [...lines, line.line!], - ifAbsent: () => [line.line!], - ); - } - } - } - - return CoverageMetrics( - totalFound: current.totalFound + found, - totalHits: current.totalHits + hit, - uncoveredLines: uncoveredLines, - ); - }); + return CoverageMetrics( + totalFound: totalFound, + totalHits: totalHits, + uncoveredLines: uncoveredLines, + ); } /// Total number of lines hit (covered) across all included files. diff --git a/lib/src/cli/lcov_merger.dart b/lib/src/cli/lcov_merger.dart index 11a1e7cf0..939f2ae3f 100644 --- a/lib/src/cli/lcov_merger.dart +++ b/lib/src/cli/lcov_merger.dart @@ -91,13 +91,7 @@ class LcovRecord { ..writeln('LH:${lines.values.where((hits) => hits > 0).length}'); if (branches.isNotEmpty) { - final keys = branches.keys.sorted( - (a, b) => [ - a.$1 - b.$1, - a.$2 - b.$2, - a.$3 - b.$3, - ].firstWhere((order) => order != 0, orElse: () => 0), - ); + final keys = branches.keys.sorted(_compareBranches); for (final key in keys) { buffer.writeln('BRDA:${key.$1},${key.$2},${key.$3},${branches[key]}'); } @@ -111,6 +105,15 @@ class LcovRecord { } } +/// Orders branches by line, then block, then branch number. +int _compareBranches(LcovBranch a, LcovBranch b) { + final (aLine, aBlock, aBranch) = a; + final (bLine, bBlock, bBranch) = b; + if (aLine != bLine) return aLine.compareTo(bLine); + if (aBlock != bBlock) return aBlock.compareTo(bBlock); + return aBranch.compareTo(bBranch); +} + /// Parses the lcov [content] into one [LcovRecord] per `end_of_record`. /// /// Unlike `package:lcov_parser`, this tolerates CRLF line endings, blank diff --git a/lib/src/cli/test_cli_runner.dart b/lib/src/cli/test_cli_runner.dart index cf0a5c8ef..400070ba3 100644 --- a/lib/src/cli/test_cli_runner.dart +++ b/lib/src/cli/test_cli_runner.dart @@ -311,10 +311,12 @@ class TestCLIRunner { if (minCoverage != null || showUncovered) { checkCoverage( - await Parser.parse(lcovPath), + CoverageMetrics.fromLcovRecords( + await Parser.parse(lcovPath), + excludeFromCoverage: excludeFromCoverage, + ), minCoverage: minCoverage, showUncovered: showUncovered, - excludeFromCoverage: excludeFromCoverage, stdout: stdout, ); } diff --git a/lib/src/commands/coverage/commands/merge.dart b/lib/src/commands/coverage/commands/merge.dart index 79b401b83..cae6ddf8e 100644 --- a/lib/src/commands/coverage/commands/merge.dart +++ b/lib/src/commands/coverage/commands/merge.dart @@ -121,10 +121,12 @@ class CoverageMergeCommand extends Command { if (minCoverage != null || showUncovered) { checkCoverage( - await Parser.parse(outputFile.path), + CoverageMetrics.fromLcovRecords( + await Parser.parse(outputFile.path), + excludeFromCoverage: excludeFromCoverage, + ), minCoverage: minCoverage, showUncovered: showUncovered, - excludeFromCoverage: excludeFromCoverage, stdout: _logger.write, ); } diff --git a/test/src/cli/coverage_reporter_test.dart b/test/src/cli/coverage_check_test.dart similarity index 67% rename from test/src/cli/coverage_reporter_test.dart rename to test/src/cli/coverage_check_test.dart index f25581741..fa8239f06 100644 --- a/test/src/cli/coverage_reporter_test.dart +++ b/test/src/cli/coverage_check_test.dart @@ -6,40 +6,42 @@ void main() { group(checkCoverage, () { late List stdoutLogs; - final records = Parser.parseLines([ - 'SF:lib/a.dart', - 'DA:1,1', - 'DA:2,0', - 'LF:2', - 'LH:1', - 'end_of_record', - 'SF:lib/b.dart', - 'DA:1,1', - 'DA:2,1', - 'LF:2', - 'LH:2', - 'end_of_record', - ]); + final metrics = CoverageMetrics.fromLcovRecords( + Parser.parseLines([ + 'SF:lib/a.dart', + 'DA:1,1', + 'DA:2,0', + 'LF:2', + 'LH:1', + 'end_of_record', + 'SF:lib/b.dart', + 'DA:1,1', + 'DA:2,1', + 'LF:2', + 'LH:2', + 'end_of_record', + ]), + ); setUp(() { stdoutLogs = []; }); test('does nothing when no threshold is set', () { - checkCoverage(records, stdout: stdoutLogs.add); + checkCoverage(metrics, stdout: stdoutLogs.add); expect(stdoutLogs, isEmpty); }); test('completes when the threshold is met', () { - checkCoverage(records, minCoverage: 75, stdout: stdoutLogs.add); + checkCoverage(metrics, minCoverage: 75, stdout: stdoutLogs.add); expect(stdoutLogs, isEmpty); }); test('throws $MinCoverageNotMet when the threshold is not met', () { expect( - () => checkCoverage(records, minCoverage: 80), + () => checkCoverage(metrics, minCoverage: 80), throwsA( isA() .having((e) => e.coverage, 'coverage', equals(75)) @@ -50,7 +52,7 @@ void main() { test('throws with uncovered lines when show uncovered is set', () { expect( - () => checkCoverage(records, minCoverage: 80, showUncovered: true), + () => checkCoverage(metrics, minCoverage: 80, showUncovered: true), throwsA( isA().having( (e) => e.uncoveredLines, @@ -65,7 +67,7 @@ void main() { test('logs uncovered lines when the threshold is met', () { checkCoverage( - records, + metrics, minCoverage: 75, showUncovered: true, stdout: stdoutLogs.add, @@ -78,22 +80,13 @@ void main() { 'logs nothing when show uncovered is set and all lines are covered', () { checkCoverage( - records, + const CoverageMetrics(totalHits: 2, totalFound: 2), showUncovered: true, - excludeFromCoverage: 'lib/a.dart', stdout: stdoutLogs.add, ); expect(stdoutLogs, isEmpty); }, ); - - test('ignores files matching the exclude globs', () { - checkCoverage( - records, - minCoverage: 100, - excludeFromCoverage: 'lib/a.dart lib/c.dart', - ); - }); }); } From 5ed28a652498cb439692897d06f4c056f2806e56 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 18:33:08 +0200 Subject: [PATCH 10/14] fix(coverage): check merged coverage without re-parsing the report coverage merge wrote the merged report and then re-read it with package:lcov_parser to enforce --min-coverage. That parser splits lines on ":" and keeps the second field, so the external Windows source paths that merge deliberately keeps (e.g. "C:/runner/lib/a.g.dart") were read back as "C": --exclude-coverage globs never matched them and --show-uncovered grouped them all under "C". The threshold is now checked on the merged records in memory, through a new CoverageMetrics.fromLcov, so the merge only goes through parseLcov. --- lib/src/cli/flutter_cli.dart | 19 +++++++++ lib/src/commands/coverage/commands/merge.dart | 13 +++--- test/src/cli/flutter_cli_test.dart | 40 +++++++++++++++++++ .../coverage/commands/merge_test.dart | 25 ++++++++++++ 4 files changed, 89 insertions(+), 8 deletions(-) diff --git a/lib/src/cli/flutter_cli.dart b/lib/src/cli/flutter_cli.dart index eb7e8c859..d6a5fe9fe 100644 --- a/lib/src/cli/flutter_cli.dart +++ b/lib/src/cli/flutter_cli.dart @@ -85,6 +85,25 @@ class CoverageMetrics { ), ], excludeFromCoverage: excludeFromCoverage); + /// Generate coverage metrics from a list of [LcovRecord]s, as returned by + /// [parseLcov]. + factory fromLcov( + Iterable records, { + String? excludeFromCoverage, + }) => ._fromFiles([ + for (final record in records) + ( + file: record.file, + found: record.lines.length, + hit: record.lines.values.where((hits) => hits > 0).length, + uncovered: [ + for (final MapEntry(key: line, value: hits) + in record.lines.entries.sortedBy((entry) => entry.key)) + if (hits == 0) line, + ], + ), + ], excludeFromCoverage: excludeFromCoverage); + factory _fromFiles( List<({String? file, int found, int hit, List uncovered})> files, { String? excludeFromCoverage, diff --git a/lib/src/commands/coverage/commands/merge.dart b/lib/src/commands/coverage/commands/merge.dart index cae6ddf8e..d8d58021d 100644 --- a/lib/src/commands/coverage/commands/merge.dart +++ b/lib/src/commands/coverage/commands/merge.dart @@ -2,7 +2,6 @@ import 'package:args/command_runner.dart'; import 'package:collection/collection.dart'; import 'package:glob/glob.dart'; import 'package:glob/list_local_fs.dart'; -import 'package:lcov_parser/lcov_parser.dart'; import 'package:mason/mason.dart'; import 'package:path/path.dart' as p; import 'package:universal_io/io.dart'; @@ -95,14 +94,14 @@ class CoverageMergeCommand extends Command { ? _discoverInputs(cwd: cwd, output: output) : _resolveInputs(argResults.rest, cwd: cwd); final externalPaths = {}; - final records = [ + final records = mergeLcovRecords([ for (final input in inputs) ...normalizeLcovRecords( _parse(input.path), packagePath: input.packagePath, onExternalPath: externalPaths.add, ), - ]; + ]); if (externalPaths.isNotEmpty) { _logger.warn( @@ -114,15 +113,13 @@ class CoverageMergeCommand extends Command { final outputFile = File(p.join(cwd, output)); await outputFile.create(recursive: true); - await outputFile.writeAsString( - formatLcovRecords(mergeLcovRecords(records)), - ); + await outputFile.writeAsString(formatLcovRecords(records)); _logger.info('Merged ${inputs.length} lcov report(s) into $output'); if (minCoverage != null || showUncovered) { checkCoverage( - CoverageMetrics.fromLcovRecords( - await Parser.parse(outputFile.path), + CoverageMetrics.fromLcov( + records, excludeFromCoverage: excludeFromCoverage, ), minCoverage: minCoverage, diff --git a/test/src/cli/flutter_cli_test.dart b/test/src/cli/flutter_cli_test.dart index 9c675e8a1..7366805c4 100644 --- a/test/src/cli/flutter_cli_test.dart +++ b/test/src/cli/flutter_cli_test.dart @@ -536,5 +536,45 @@ void main() { }); }); }); + + group('fromLcov', () { + test('derives the totals and uncovered lines from the line hits', () { + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('lib/a.dart', lines: {3: 0, 1: 2, 2: 0}), + LcovRecord('lib/b.dart', lines: {1: 1}), + ]); + + expect(metrics.totalFound, equals(4)); + expect(metrics.totalHits, equals(2)); + expect( + metrics.uncoveredLines, + equals({ + 'lib/a.dart': [2, 3], + }), + ); + }); + + test('keeps source paths that contain a colon', () { + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('C:/runner/lib/a.dart', lines: {1: 0}), + LcovRecord('D:/runner/lib/b.dart', lines: {1: 0}), + ]); + + expect( + metrics.uncoveredLines.keys, + equals(['C:/runner/lib/a.dart', 'D:/runner/lib/b.dart']), + ); + }); + + test('excludes source paths that contain a colon', () { + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('lib/a.dart', lines: {1: 1}), + LcovRecord('C:/runner/lib/b.g.dart', lines: {1: 0}), + ], excludeFromCoverage: '**/*.g.dart'); + + expect(metrics.totalFound, equals(1)); + expect(metrics.totalHits, equals(1)); + }); + }); }); } diff --git a/test/src/commands/coverage/commands/merge_test.dart b/test/src/commands/coverage/commands/merge_test.dart index 6c931f9dd..edb723848 100644 --- a/test/src/commands/coverage/commands/merge_test.dart +++ b/test/src/commands/coverage/commands/merge_test.dart @@ -501,6 +501,31 @@ end_of_record expect(result, equals(ExitCode.success.code)); }), ); + + test( + 'ignores external Windows source paths matching --exclude-coverage', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeFile( + 'shard.info', + 'SF:lib/a.dart\nDA:1,1\nend_of_record\n' + 'SF:C:\\runner\\lib\\a.g.dart\nDA:1,0\nend_of_record\n', + ); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shard.info', + '--min-coverage', + '100', + '--exclude-coverage', + '**/*.g.dart', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), contains('SF:C:/runner/lib/a.g.dart\n')); + }), + ); }); test( From 9777c3827b7998cbe70464637fff15f29ab5283a Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 18:33:28 +0200 Subject: [PATCH 11/14] fix(coverage): skip the merged report when matched by a glob Discovery already left the --output report out, but glob inputs did not: running `very_good coverage merge '**/lcov.info'` twice merged the previous coverage/lcov.info into the new one, doubling its hit counts. Glob matches now skip the --output report too, with the same warning, and fail like an empty glob when it was their only match. A report passed by its exact path is still merged, so it can be overwritten on purpose. Explicit paths are also kept relative to the current directory, like glob matches, so the same report given as an absolute path and through a glob is merged once. --- lib/src/commands/coverage/commands/merge.dart | 73 ++++++++++++------- .../coverage/commands/merge_test.dart | 66 +++++++++++++++++ 2 files changed, 114 insertions(+), 25 deletions(-) diff --git a/lib/src/commands/coverage/commands/merge.dart b/lib/src/commands/coverage/commands/merge.dart index d8d58021d..1d3b7efff 100644 --- a/lib/src/commands/coverage/commands/merge.dart +++ b/lib/src/commands/coverage/commands/merge.dart @@ -80,7 +80,7 @@ class CoverageMergeCommand extends Command { 'show-uncovered', testConfig.showUncovered ?? dartTestConfig.showUncovered, ); - final output = argResults['output'] as String; + final output = p.normalize(argResults['output'] as String); try { if (rawMinCoverage != null && minCoverage == null) { @@ -90,9 +90,10 @@ class CoverageMergeCommand extends Command { ); } + final outputPath = p.join(cwd, output); final inputs = argResults.rest.isEmpty - ? _discoverInputs(cwd: cwd, output: output) - : _resolveInputs(argResults.rest, cwd: cwd); + ? _discoverInputs(cwd: cwd, outputPath: outputPath) + : _resolveInputs(argResults.rest, cwd: cwd, outputPath: outputPath); final externalPaths = {}; final records = mergeLcovRecords([ for (final input in inputs) @@ -141,20 +142,25 @@ class CoverageMergeCommand extends Command { return ExitCode.success.code; } - /// The lcov reports to merge, and the package directory their relative - /// source paths should be rebased onto. + /// The lcov reports to merge, as paths relative to [cwd], and the package + /// directory their relative source paths should be rebased onto. /// /// Each of [args] is a path to an lcov file or, when no such file exists, a /// glob relative to [cwd]. Expanding globs here, rather than relying on the /// shell, makes quoted patterns behave the same on every platform. - List<({String path, String? packagePath})> _resolveInputs( + /// + /// Glob matches skip the report at [outputPath], while an explicit path to + /// it is merged, so that it can be overwritten on purpose. + List<_MergeInput> _resolveInputs( List args, { required String cwd, + required String outputPath, }) { final paths = {}; for (final arg in args) { - if (File(p.join(cwd, arg)).existsSync()) { - paths.add(p.normalize(arg)); + final file = File(p.join(cwd, arg)); + if (file.existsSync()) { + paths.add(p.relative(file.path, from: cwd)); continue; } @@ -164,6 +170,9 @@ class CoverageMergeCommand extends Command { .listSync(root: cwd) .whereType() .map((file) => p.relative(file.path, from: cwd)) + .whereNot( + (path) => _isOutput(path, cwd: cwd, outputPath: outputPath), + ) .sorted(); } on FormatException catch (error) { throw _MergeError( @@ -188,25 +197,18 @@ class CoverageMergeCommand extends Command { /// relative source paths rebased onto their package, so the same /// `lib/a.dart` from two packages stays distinct. /// - /// The [output] report is left out, so that merging again doesn't count the - /// previous merge. - List<({String path, String? packagePath})> _discoverInputs({ + /// The report at [outputPath] is skipped. + List<_MergeInput> _discoverInputs({ required String cwd, - required String output, + required String outputPath, }) { - final outputPath = p.join(cwd, output); - final inputs = <({String path, String? packagePath})>[]; - for (final package in discoverLcovPackages(cwd)) { - final path = p.normalize(p.join(package, 'coverage', 'lcov.info')); - if (p.equals(p.join(cwd, path), outputPath)) { - _logger.warn( - 'Skipping $path, since it is the --output report. Pass a different ' - '--output to merge it too.', - ); - continue; - } - inputs.add((path: path, packagePath: package)); - } + final inputs = <_MergeInput>[ + for (final package in discoverLcovPackages(cwd)) + if (p.normalize(p.join(package, 'coverage', 'lcov.info')) + case final path + when !_isOutput(path, cwd: cwd, outputPath: outputPath)) + (path: path, packagePath: package), + ]; if (inputs.isEmpty) { throw _MergeError( @@ -219,6 +221,23 @@ class CoverageMergeCommand extends Command { return inputs; } + /// Whether [path], relative to [cwd], is the report at [outputPath]. + /// + /// Such a report is likely a previous merge, so it is reported as skipped, + /// since merging it again would count its hits twice. + bool _isOutput( + String path, { + required String cwd, + required String outputPath, + }) { + if (!p.equals(p.join(cwd, path), outputPath)) return false; + _logger.warn( + 'Skipping $path, since it is the --output report. Pass a different ' + '--output to merge it too.', + ); + return true; + } + List _parse(String path) { try { return parseLcov(File(path).readAsStringSync()); @@ -231,6 +250,10 @@ class CoverageMergeCommand extends Command { } } +/// An lcov report to merge, and the package directory its relative source +/// paths should be rebased onto, if any. +typedef _MergeInput = ({String path, String? packagePath}); + /// A failure that stops the merge, reported with [message] and [exitCode]. class _MergeError implements Exception { const new(this.message, this.exitCode); diff --git a/test/src/commands/coverage/commands/merge_test.dart b/test/src/commands/coverage/commands/merge_test.dart index edb723848..04a2c2c9e 100644 --- a/test/src/commands/coverage/commands/merge_test.dart +++ b/test/src/commands/coverage/commands/merge_test.dart @@ -184,6 +184,54 @@ void main() { }), ); + test( + 'merges a file once when given as an absolute path and by a glob', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + final cwd = _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + p.join(cwd.path, 'shards', '1', 'lcov.info'), + 'shards/*/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), equals(_merged)); + verify( + () => logger.info('Merged 2 lcov report(s) into coverage/lcov.info'), + ).called(1); + }), + ); + + test( + 'skips the --output report when matched by a glob', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + _writeFile(p.join('coverage', 'lcov.info'), _merged); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + '**/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), equals(_merged)); + verify( + () => logger.warn( + 'Skipping ${p.join('coverage', 'lcov.info')}, since it is the ' + '--output report. Pass a different --output to merge it too.', + ), + ).called(1); + verify( + () => logger.info('Merged 2 lcov report(s) into coverage/lcov.info'), + ).called(1); + }), + ); + test( 'makes absolute source paths under the current directory relative', withRunner((commandRunner, logger, pubUpdater, printLogs) async { @@ -401,6 +449,24 @@ end_of_record }), ); + test( + 'when a glob only matches the --output report', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeFile(p.join('coverage', 'lcov.info'), _merged); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'coverage/*.info', + ]); + + expect(result, equals(ExitCode.noInput.code)); + verify(() => logger.err('No lcov report found at "coverage/*.info".')) + .called(1); + }), + ); + test( 'when --min-coverage is not a number', withRunner((commandRunner, logger, pubUpdater, printLogs) async { From 69c21915ed7e417585d533d6865815d05eb9f11e Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Thu, 1 Oct 2026 18:53:31 +0200 Subject: [PATCH 12/14] refactor(coverage): reuse the resolved output path when writing the merged report --- lib/src/commands/coverage/commands/merge.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/commands/coverage/commands/merge.dart b/lib/src/commands/coverage/commands/merge.dart index 1d3b7efff..0db79d2ee 100644 --- a/lib/src/commands/coverage/commands/merge.dart +++ b/lib/src/commands/coverage/commands/merge.dart @@ -112,7 +112,7 @@ class CoverageMergeCommand extends Command { ); } - final outputFile = File(p.join(cwd, output)); + final outputFile = File(outputPath); await outputFile.create(recursive: true); await outputFile.writeAsString(formatLcovRecords(records)); _logger.info('Merged ${inputs.length} lcov report(s) into $output'); From 650f3e8799abfb95df77c3e68290de2d7d590a19 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Fri, 2 Oct 2026 15:59:57 +0300 Subject: [PATCH 13/14] fix(coverage): harden coverage merge inputs and unify lcov parsing - Report file system errors from coverage merge instead of crashing. - Keep an lcov record left without end_of_record before the next SF. - Rebase every package coverage/lcov.info report onto its package and skip glob matches in platform, build and tool directories. - Match exclude_coverage globs against package-relative paths. - Share the --recursive pubspec filter with lcov package discovery. - Parse lcov with parseLcov in the test runner and drop CoverageMetrics.fromLcovRecords. - Move the coverage check helpers into coverage_check.dart. Addresses FINDING-01 to FINDING-08 from review. Co-Authored-By: Claude Opus 5.5 (1M context) --- lib/src/cli/cli.dart | 14 +- lib/src/cli/coverage_check.dart | 152 ++++++++- lib/src/cli/flutter_cli.dart | 97 +----- lib/src/cli/lcov_merger.dart | 6 +- lib/src/cli/test_cli_runner.dart | 81 +---- lib/src/commands/coverage/commands/merge.dart | 75 +++-- .../dart/commands/dart_test_command.dart | 2 +- lib/src/commands/test/test.dart | 2 +- site/docs/commands/coverage.md | 20 +- test/src/cli/coverage_check_test.dart | 21 +- test/src/cli/flutter_cli_test.dart | 289 ++++++------------ test/src/cli/lcov_merger_test.dart | 80 ++++- .../coverage/commands/merge_test.dart | 191 +++++++++++- 13 files changed, 586 insertions(+), 444 deletions(-) diff --git a/lib/src/cli/cli.dart b/lib/src/cli/cli.dart index a28931b84..f4c1bea51 100644 --- a/lib/src/cli/cli.dart +++ b/lib/src/cli/cli.dart @@ -5,7 +5,6 @@ import 'dart:math'; import 'package:collection/collection.dart'; import 'package:coverage/coverage.dart' as coverage; import 'package:glob/glob.dart'; -import 'package:lcov_parser/lcov_parser.dart'; import 'package:mason/mason.dart'; import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; @@ -190,11 +189,24 @@ const _ignoredDirectories = { '.fvm', }; +/// Whether the relative [path] goes through a platform, build or tool +/// directory, which recursive commands skip. +bool isInIgnoredDirectory(String path) => + p.split(path).any(_ignoredDirectories.contains); + bool _isPubspec(FileSystemEntity entity) { if (entity is! File) return false; return p.basename(entity.path) == 'pubspec.yaml'; } +/// Whether [entity] is the `pubspec.yaml` of a package that recursive commands +/// run on, skipping platform, build and tool directories and the [ignore]d +/// ones. +bool _isPackagePubspec( + FileSystemEntity entity, { + Set ignore = const {}, +}) => _isPubspec(entity) && !ignore.excludes(entity); + extension on Set { bool excludes(FileSystemEntity entity) { final segments = p.split(entity.path).toSet(); diff --git a/lib/src/cli/coverage_check.dart b/lib/src/cli/coverage_check.dart index be2de9006..a1f832dd6 100644 --- a/lib/src/cli/coverage_check.dart +++ b/lib/src/cli/coverage_check.dart @@ -24,6 +24,156 @@ void checkCoverage( // When coverage passes but is below 100%, // show uncovered lines as informational output. if (uncoveredLines != null) { - stdout?.call('${TestCLIRunner.formatUncoveredLines(uncoveredLines)}\n'); + stdout?.call('${formatUncoveredLines(uncoveredLines)}\n'); } } + +/// {@template coverage_not_met} +/// Thrown when `flutter test ---coverage --min-coverage` +/// does not meet the provided minimum coverage threshold. +/// {@endtemplate} +class MinCoverageNotMet implements Exception { + /// {@macro coverage_not_met} + const new(this.coverage, {this.uncoveredLines}); + + /// The measured coverage percentage (total hits / total found * 100). + final double coverage; + + /// Lines not covered, keyed by file path, values are line numbers. + /// + /// Only populated when `--show-uncovered` is set. + final Map>? uncoveredLines; +} + +/// {@template coverage_metrics} +/// Aggregated coverage metrics computed from a list of LCOV records. +/// {@endtemplate} +class CoverageMetrics { + /// {@macro coverage_metrics} + @visibleForTesting + const new({ + this.totalHits = 0, + this.totalFound = 0, + this.uncoveredLines = const {}, + }); + + /// Generate coverage metrics from a list of [LcovRecord]s, as returned by + /// [parseLcov]. + /// + /// Files matching any of the space separated [excludeFromCoverage] globs are + /// left out. The globs are also matched against the paths relative to each + /// of [packagePaths], so that a glob written for a single package, like + /// `lib/src/gen/**`, still applies once its report is rebased onto the + /// package by [normalizeLcovRecords]. + factory fromLcov( + Iterable records, { + String? excludeFromCoverage, + Iterable packagePaths = const [], + }) { + final globs = [ + for (final glob in (excludeFromCoverage ?? '').trim().split(' ')) + if (glob.isNotEmpty) Glob(glob), + ]; + + bool isExcluded(String file) => [ + file, + for (final package in packagePaths) + if (p.posix.isWithin(package, file)) + p.posix.relative(file, from: package), + ].any((path) => globs.any((glob) => glob.matches(path))); + + var totalFound = 0; + var totalHits = 0; + final uncoveredLines = >{}; + for (final LcovRecord(:file, :lines) in records) { + if (isExcluded(file)) continue; + + totalFound += lines.length; + totalHits += lines.values.where((hits) => hits > 0).length; + final uncovered = [ + for (final MapEntry(key: line, value: hits) + in lines.entries.sortedBy((entry) => entry.key)) + if (hits == 0) line, + ]; + if (uncovered.isNotEmpty) { + (uncoveredLines[file] ??= []).addAll(uncovered); + } + } + + return CoverageMetrics( + totalFound: totalFound, + totalHits: totalHits, + uncoveredLines: uncoveredLines, + ); + } + + /// Total number of lines hit (covered) across all included files. + final int totalHits; + + /// Total number of instrumented lines found across all included files. + final int totalFound; + + /// Lines not covered. + /// Keyed by file path, values are sorted line numbers. + final Map> uncoveredLines; + + /// Coverage percentage: [totalHits] / [totalFound] * 100. + /// + /// Returns `0` when [totalFound] is less than 1. + double get percentage { + return totalFound < 1 ? 0 : (totalHits / totalFound * 100); + } +} + +/// Logs [error], along with its uncovered lines when it carries any, and +/// returns the exit code an unmet coverage threshold reports. +int handleMinCoverageNotMet( + MinCoverageNotMet error, { + required Logger logger, + double? minCoverage, +}) { + var decimalPlaces = 2; + + double round(double x) { + final b = pow(10, decimalPlaces); + return (x * b).roundToDouble() / b; + } + + if (error.coverage < minCoverage!) { + var rounded = round(error.coverage); + while (rounded == minCoverage) { + decimalPlaces++; + rounded = round(error.coverage); + } + } + + logger.err( + '''Expected coverage >= ${minCoverage.toStringAsFixed(decimalPlaces)}% but actual is ${error.coverage.toStringAsFixed(decimalPlaces)}%.''', + ); + + final uncoveredLines = error.uncoveredLines; + if (uncoveredLines != null && uncoveredLines.isNotEmpty) { + logger.err(formatUncoveredLines(uncoveredLines)); + } + + return ExitCode.software.code; +} + +/// Formats a map of uncovered lines into a human-readable string. +/// +/// The [uncoveredLines] map is keyed by file path, with values being lists +/// of uncovered line numbers. +/// +/// Example output: +/// ```dart +/// Lines not covered: +/// - lib/src/foo.dart: 10, 20, 30 +/// - lib/src/bar.dart: 5 +/// ``` +String formatUncoveredLines(Map> uncoveredLines) { + final lines = uncoveredLines.entries.map((entry) { + final sortedLines = [...entry.value]..sort(); + return '\t- ${entry.key}: ${sortedLines.join(', ')}'; + }); + return 'Lines not covered:\n${lines.join('\n')}'; +} diff --git a/lib/src/cli/flutter_cli.dart b/lib/src/cli/flutter_cli.dart index d6a5fe9fe..65b16646f 100644 --- a/lib/src/cli/flutter_cli.dart +++ b/lib/src/cli/flutter_cli.dart @@ -56,101 +56,6 @@ class _ProcessSignalOverridesScope extends ProcessSignalOverrides { /// Thrown when `flutter pub get` is executed without a `pubspec.yaml`. class PubspecNotFound implements Exception; -/// {@template coverage_metrics} -/// Aggregated coverage metrics computed from a list of LCOV records. -/// {@endtemplate} -class CoverageMetrics { - /// {@macro coverage_metrics} - @visibleForTesting - const new({ - this.totalHits = 0, - this.totalFound = 0, - this.uncoveredLines = const {}, - }); - - /// Generate coverage metrics from a list of lcov records. - factory fromLcovRecords( - List records, { - String? excludeFromCoverage, - }) => ._fromFiles([ - for (final record in records) - ( - file: record.file, - found: record.lines?.found ?? 0, - hit: record.lines?.hit ?? 0, - uncovered: [ - for (final line in [...?record.lines?.details]) - if ((line.hit ?? 1) == 0 && line.line != null) line.line!, - ], - ), - ], excludeFromCoverage: excludeFromCoverage); - - /// Generate coverage metrics from a list of [LcovRecord]s, as returned by - /// [parseLcov]. - factory fromLcov( - Iterable records, { - String? excludeFromCoverage, - }) => ._fromFiles([ - for (final record in records) - ( - file: record.file, - found: record.lines.length, - hit: record.lines.values.where((hits) => hits > 0).length, - uncovered: [ - for (final MapEntry(key: line, value: hits) - in record.lines.entries.sortedBy((entry) => entry.key)) - if (hits == 0) line, - ], - ), - ], excludeFromCoverage: excludeFromCoverage); - - factory _fromFiles( - List<({String? file, int found, int hit, List uncovered})> files, { - String? excludeFromCoverage, - }) { - final globs = [ - for (final glob in (excludeFromCoverage ?? '').trim().split(' ')) - if (glob.isNotEmpty) Glob(glob), - ]; - - var totalFound = 0; - var totalHits = 0; - final uncoveredLines = >{}; - for (final (:file, :found, :hit, :uncovered) in files) { - if (file != null && globs.any((glob) => glob.matches(file))) continue; - - totalFound += found; - totalHits += hit; - if (file != null && uncovered.isNotEmpty) { - (uncoveredLines[file] ??= []).addAll(uncovered); - } - } - - return CoverageMetrics( - totalFound: totalFound, - totalHits: totalHits, - uncoveredLines: uncoveredLines, - ); - } - - /// Total number of lines hit (covered) across all included files. - final int totalHits; - - /// Total number of instrumented lines found across all included files. - final int totalFound; - - /// Lines not covered. - /// Keyed by file path, values are sorted line numbers. - final Map> uncoveredLines; - - /// Coverage percentage: [totalHits] / [totalFound] * 100. - /// - /// Returns `0` when [totalFound] is less than 1. - double get percentage { - return totalFound < 1 ? 0 : (totalHits / totalFound * 100); - } -} - /// Flutter CLI class Flutter { /// Determine whether flutter is installed. @@ -312,7 +217,7 @@ Future> _runCommand({ final processes = _Cmd.runWhere( run: (entity) => cmd(entity.parent.path), - where: (entity) => !ignore.excludes(entity) && _isPubspec(entity), + where: (entity) => _isPackagePubspec(entity, ignore: ignore), cwd: cwd, ); diff --git a/lib/src/cli/lcov_merger.dart b/lib/src/cli/lcov_merger.dart index 939f2ae3f..28fb08d22 100644 --- a/lib/src/cli/lcov_merger.dart +++ b/lib/src/cli/lcov_merger.dart @@ -152,6 +152,9 @@ List parseLcov(String content) { switch ((tag, record)) { case ('SF', _): + // A record left without `end_of_record` is kept, as at the end of the + // report, rather than dropped. + if (record != null) records.add(record); record = LcovRecord(value); case ('DA' || 'FN' || 'FNDA' || 'BRDA', null): throw FormatException('Found "$line" before any "SF:" line.'); @@ -226,9 +229,8 @@ List normalizeLcovRecords( /// and tool directories are skipped. List discoverLcovPackages(String cwd) => Directory(cwd) .listSync(recursive: true) - .where(_isPubspec) + .where(_isPackagePubspec) .map((pubspec) => p.relative(pubspec.parent.path, from: cwd)) - .where((package) => !p.split(package).any(_ignoredDirectories.contains)) .where( (package) => File(p.join(cwd, package, 'coverage', 'lcov.info')).existsSync(), diff --git a/lib/src/cli/test_cli_runner.dart b/lib/src/cli/test_cli_runner.dart index 400070ba3..b9d3221b8 100644 --- a/lib/src/cli/test_cli_runner.dart +++ b/lib/src/cli/test_cli_runner.dart @@ -35,23 +35,6 @@ enum CoverageCollectionMode { } } -/// {@template coverage_not_met} -/// Thrown when `flutter test ---coverage --min-coverage` -/// does not meet the provided minimum coverage threshold. -/// {@endtemplate} -class MinCoverageNotMet implements Exception { - /// {@macro coverage_not_met} - const new(this.coverage, {this.uncoveredLines}); - - /// The measured coverage percentage (total hits / total found * 100). - final double coverage; - - /// Lines not covered, keyed by file path, values are line numbers. - /// - /// Only populated when `--show-uncovered` is set. - final Map>? uncoveredLines; -} - /// A class to run test command from a CLI command, like `flutter` or `dart`. /// /// It abstracts common functionalities like the test optimization, coverage @@ -311,8 +294,8 @@ class TestCLIRunner { if (minCoverage != null || showUncovered) { checkCoverage( - CoverageMetrics.fromLcovRecords( - await Parser.parse(lcovPath), + CoverageMetrics.fromLcov( + parseLcov(await lcovFile.readAsString()), excludeFromCoverage: excludeFromCoverage, ), minCoverage: minCoverage, @@ -334,59 +317,6 @@ class TestCLIRunner { ? body.call() : overrideAnsiOutput(enableAnsiOutput, body); - /// Logs [error], along with its uncovered lines when it carries any, and - /// returns the exit code an unmet coverage threshold reports. - static int handleMinCoverageNotMet( - MinCoverageNotMet error, { - required Logger logger, - double? minCoverage, - }) { - var decimalPlaces = 2; - - double round(double x) { - final b = pow(10, decimalPlaces); - return (x * b).roundToDouble() / b; - } - - if (error.coverage < minCoverage!) { - var rounded = round(error.coverage); - while (rounded == minCoverage) { - decimalPlaces++; - rounded = round(error.coverage); - } - } - - logger.err( - '''Expected coverage >= ${minCoverage.toStringAsFixed(decimalPlaces)}% but actual is ${error.coverage.toStringAsFixed(decimalPlaces)}%.''', - ); - - final uncoveredLines = error.uncoveredLines; - if (uncoveredLines != null && uncoveredLines.isNotEmpty) { - logger.err(formatUncoveredLines(uncoveredLines)); - } - - return ExitCode.software.code; - } - - /// Formats a map of uncovered lines into a human-readable string. - /// - /// The [uncoveredLines] map is keyed by file path, with values being lists - /// of uncovered line numbers. - /// - /// Example output: - /// ```dart - /// Lines not covered: - /// - lib/src/foo.dart: 10, 20, 30 - /// - lib/src/bar.dart: 5 - /// ``` - static String formatUncoveredLines(Map> uncoveredLines) { - final lines = uncoveredLines.entries.map((entry) { - final sortedLines = [...entry.value]..sort(); - return '\t- ${entry.key}: ${sortedLines.join(', ')}'; - }); - return 'Lines not covered:\n${lines.join('\n')}'; - } - /// Discovers all Dart files in the specified directories for coverage. static List _discoverDartFilesForCoverage({ required String cwd, @@ -426,11 +356,8 @@ class TestCLIRunner { ); // Parse existing lcov to find covered files - final existingRecords = await Parser.parse(lcovPath); - final coveredFiles = existingRecords - .where((r) => r.file != null) - .map((r) => r.file!) - .toSet(); + final existingRecords = parseLcov(await lcovFile.readAsString()); + final coveredFiles = existingRecords.map((r) => r.file).toSet(); // Find uncovered files final uncoveredFiles = allDartFiles.where((file) { diff --git a/lib/src/commands/coverage/commands/merge.dart b/lib/src/commands/coverage/commands/merge.dart index 0db79d2ee..c9a1bff52 100644 --- a/lib/src/commands/coverage/commands/merge.dart +++ b/lib/src/commands/coverage/commands/merge.dart @@ -91,15 +91,18 @@ class CoverageMergeCommand extends Command { } final outputPath = p.join(cwd, output); - final inputs = argResults.rest.isEmpty + final paths = argResults.rest.isEmpty ? _discoverInputs(cwd: cwd, outputPath: outputPath) : _resolveInputs(argResults.rest, cwd: cwd, outputPath: outputPath); + final inputs = { + for (final path in paths) path: _packageOf(path, cwd: cwd), + }; final externalPaths = {}; final records = mergeLcovRecords([ - for (final input in inputs) + for (final MapEntry(key: path, value: packagePath) in inputs.entries) ...normalizeLcovRecords( - _parse(input.path), - packagePath: input.packagePath, + _parse(path), + packagePath: packagePath, onExternalPath: externalPaths.add, ), ]); @@ -122,6 +125,10 @@ class CoverageMergeCommand extends Command { CoverageMetrics.fromLcov( records, excludeFromCoverage: excludeFromCoverage, + packagePaths: [ + for (final package in inputs.values.nonNulls) + package.replaceAll(r'\', '/'), + ], ), minCoverage: minCoverage, showUncovered: showUncovered, @@ -132,26 +139,29 @@ class CoverageMergeCommand extends Command { _logger.err(error.message); return error.exitCode; } on MinCoverageNotMet catch (error) { - return TestCLIRunner.handleMinCoverageNotMet( + return handleMinCoverageNotMet( error, logger: _logger, minCoverage: minCoverage, ); + } on FileSystemException catch (error) { + _logger.err('$error'); + return ExitCode.ioError.code; } return ExitCode.success.code; } - /// The lcov reports to merge, as paths relative to [cwd], and the package - /// directory their relative source paths should be rebased onto. + /// The lcov reports to merge, as paths relative to [cwd]. /// /// Each of [args] is a path to an lcov file or, when no such file exists, a /// glob relative to [cwd]. Expanding globs here, rather than relying on the /// shell, makes quoted patterns behave the same on every platform. /// - /// Glob matches skip the report at [outputPath], while an explicit path to - /// it is merged, so that it can be overwritten on purpose. - List<_MergeInput> _resolveInputs( + /// Glob matches skip the report at [outputPath] and the reports in platform, + /// build and tool directories, while an explicit path to them is merged, so + /// that they can be merged on purpose. + List _resolveInputs( List args, { required String cwd, required String outputPath, @@ -181,33 +191,42 @@ class CoverageMergeCommand extends Command { ); } - if (matches.isEmpty) { + final skipped = matches.where(isInIgnoredDirectory).toList(); + if (skipped.isNotEmpty) { + _logger.warn( + 'Skipping these reports matched by "$arg", since they are in ' + 'platform, build or tool directories. Pass their paths to merge ' + 'them too:\n' + '${skipped.map((path) => ' - $path').join('\n')}', + ); + } + + final kept = matches.whereNot(isInIgnoredDirectory).toList(); + if (kept.isEmpty) { throw _MergeError( 'No lcov report found at "$arg".', ExitCode.noInput.code, ); } - paths.addAll(matches); + paths.addAll(kept); } - return [for (final path in paths) (path: path, packagePath: null)]; + return paths.toList(); } - /// The `coverage/lcov.info` report of every package under [cwd], with their - /// relative source paths rebased onto their package, so the same - /// `lib/a.dart` from two packages stays distinct. + /// The `coverage/lcov.info` report of every package under [cwd]. /// /// The report at [outputPath] is skipped. - List<_MergeInput> _discoverInputs({ + List _discoverInputs({ required String cwd, required String outputPath, }) { - final inputs = <_MergeInput>[ + final inputs = [ for (final package in discoverLcovPackages(cwd)) if (p.normalize(p.join(package, 'coverage', 'lcov.info')) case final path when !_isOutput(path, cwd: cwd, outputPath: outputPath)) - (path: path, packagePath: package), + path, ]; if (inputs.isEmpty) { @@ -238,6 +257,20 @@ class CoverageMergeCommand extends Command { return true; } + /// The package directory, relative to [cwd], that the relative source paths + /// of the report at [path] are rebased onto, so that the same `lib/a.dart` + /// from two packages stays distinct. + /// + /// Only a package's `coverage/lcov.info` report, as left behind by + /// `very_good test --coverage`, is rebased. + String? _packageOf(String path, {required String cwd}) { + final package = p.dirname(p.dirname(path)); + final isPackageReport = + p.equals(path, p.join(package, 'coverage', 'lcov.info')) && + File(p.join(cwd, package, 'pubspec.yaml')).existsSync(); + return isPackageReport ? package : null; + } + List _parse(String path) { try { return parseLcov(File(path).readAsStringSync()); @@ -250,10 +283,6 @@ class CoverageMergeCommand extends Command { } } -/// An lcov report to merge, and the package directory its relative source -/// paths should be rebased onto, if any. -typedef _MergeInput = ({String path, String? packagePath}); - /// A failure that stops the merge, reported with [message] and [exitCode]. class _MergeError implements Exception { const new(this.message, this.exitCode); diff --git a/lib/src/commands/dart/commands/dart_test_command.dart b/lib/src/commands/dart/commands/dart_test_command.dart index 5302806c0..0525cd802 100644 --- a/lib/src/commands/dart/commands/dart_test_command.dart +++ b/lib/src/commands/dart/commands/dart_test_command.dart @@ -517,7 +517,7 @@ This command should be run from the root of your Dart project.'''); return ExitCode.software.code; } } on MinCoverageNotMet catch (error) { - return TestCLIRunner.handleMinCoverageNotMet( + return handleMinCoverageNotMet( error, logger: _logger, minCoverage: minCoverage, diff --git a/lib/src/commands/test/test.dart b/lib/src/commands/test/test.dart index b059d979b..a7130bd26 100644 --- a/lib/src/commands/test/test.dart +++ b/lib/src/commands/test/test.dart @@ -605,7 +605,7 @@ This command should be run from the root of your Flutter project.'''); return ExitCode.software.code; } } on MinCoverageNotMet catch (error) { - return TestCLIRunner.handleMinCoverageNotMet( + return handleMinCoverageNotMet( error, logger: _logger, minCoverage: minCoverage, diff --git a/site/docs/commands/coverage.md b/site/docs/commands/coverage.md index 2d6c531ed..933222136 100644 --- a/site/docs/commands/coverage.md +++ b/site/docs/commands/coverage.md @@ -44,15 +44,21 @@ very_good coverage merge 'shards/*/lcov.info' --output coverage/merged.info Each argument is either the path to an lcov file or a glob, relative to the current directory. The CLI expands globs itself, so quote them to get the same result in bash, zsh, PowerShell, and `cmd`. Use `/` as the separator in globs, -on every platform. A glob that matches no file is an error. +on every platform. A glob that matches no file is an error. Globs skip the +same directories as `--recursive` (such as `build`, `.dart_tool`, and the +platform folders), with a warning; pass a report's path to merge it anyway. Without arguments, the command looks for the `coverage/lcov.info` of every -package under the current directory, skipping the same directories as -`--recursive` (such as `build`, `.dart_tool`, and the platform folders). The -source paths of each report are prefixed with the path of its package, so -`lib/main.dart` from two packages counts as two files. The `--output` report is -never merged into itself; pass a different `--output` when the root package has -its own tests. +package under the current directory, skipping those same directories. + +Whether it was found or passed as an argument, a package's +`coverage/lcov.info` report has its source paths prefixed with the path of its +package, so `lib/main.dart` from two packages counts as two files. Other +reports, such as shards downloaded to `shards/1/lcov.info`, are merged as they +are. `exclude_coverage` globs match both the prefixed paths and the paths +within each package, so `lib/src/generated/**` still excludes those files in +every package. The `--output` report is never merged into itself; pass a +different `--output` when the root package has its own tests. ### How reports are merged diff --git a/test/src/cli/coverage_check_test.dart b/test/src/cli/coverage_check_test.dart index fa8239f06..6f3f29778 100644 --- a/test/src/cli/coverage_check_test.dart +++ b/test/src/cli/coverage_check_test.dart @@ -1,4 +1,3 @@ -import 'package:lcov_parser/lcov_parser.dart'; import 'package:test/test.dart'; import 'package:very_good_cli/src/cli/cli.dart'; @@ -6,22 +5,10 @@ void main() { group(checkCoverage, () { late List stdoutLogs; - final metrics = CoverageMetrics.fromLcovRecords( - Parser.parseLines([ - 'SF:lib/a.dart', - 'DA:1,1', - 'DA:2,0', - 'LF:2', - 'LH:1', - 'end_of_record', - 'SF:lib/b.dart', - 'DA:1,1', - 'DA:2,1', - 'LF:2', - 'LH:2', - 'end_of_record', - ]), - ); + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('lib/a.dart', lines: {1: 1, 2: 0}), + LcovRecord('lib/b.dart', lines: {1: 1, 2: 1}), + ]); setUp(() { stdoutLogs = []; diff --git a/test/src/cli/flutter_cli_test.dart b/test/src/cli/flutter_cli_test.dart index 7366805c4..695c8068c 100644 --- a/test/src/cli/flutter_cli_test.dart +++ b/test/src/cli/flutter_cli_test.dart @@ -2,7 +2,6 @@ import 'dart:async'; -import 'package:lcov_parser/lcov_parser.dart'; import 'package:mason/mason.dart'; import 'package:mocktail/mocktail.dart'; import 'package:path/path.dart' as p; @@ -319,48 +318,23 @@ void main() { }); group(CoverageMetrics, () { - List parseRecords(List lines) => Parser.parseLines(lines); - - group('.fromLcovRecords', () { + group('fromLcov', () { test('returns empty metrics for an empty record list', () { - final metrics = CoverageMetrics.fromLcovRecords([]); + final metrics = CoverageMetrics.fromLcov([]); expect(metrics.totalHits, equals(0)); expect(metrics.totalFound, equals(0)); expect(metrics.uncoveredLines, isEmpty); }); - test('aggregates hits and found across records', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'LF:10', - 'LH:8', - 'end_of_record', - 'SF:lib/b.dart', - 'LF:5', - 'LH:5', - 'end_of_record', - ]); - - final metrics = CoverageMetrics.fromLcovRecords(records); - - expect(metrics.totalFound, equals(15)); - expect(metrics.totalHits, equals(13)); - }); - - test('collects uncovered lines per file', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'DA:1,1', - 'DA:2,0', - 'DA:3,0', - 'LF:3', - 'LH:1', - 'end_of_record', + test('derives the totals and uncovered lines from the line hits', () { + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('lib/a.dart', lines: {3: 0, 1: 2, 2: 0}), + LcovRecord('lib/b.dart', lines: {1: 1}), ]); - final metrics = CoverageMetrics.fromLcovRecords(records); - + expect(metrics.totalFound, equals(4)); + expect(metrics.totalHits, equals(2)); expect( metrics.uncoveredLines, equals({ @@ -370,23 +344,11 @@ void main() { }); test('accumulates uncovered lines across multiple records', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'DA:10,0', - 'DA:20,1', - 'LF:2', - 'LH:1', - 'end_of_record', - 'SF:lib/b.dart', - 'DA:5,0', - 'DA:6,0', - 'LF:2', - 'LH:0', - 'end_of_record', + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('lib/a.dart', lines: {10: 0, 20: 1}), + LcovRecord('lib/b.dart', lines: {5: 0, 6: 0}), ]); - final metrics = CoverageMetrics.fromLcovRecords(records); - expect( metrics.uncoveredLines, equals({ @@ -397,184 +359,107 @@ void main() { }); test('handles records with no DA entries', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'LF:0', - 'LH:0', - 'end_of_record', - 'SF:lib/b.dart', - 'LF:4', - 'LH:4', - 'end_of_record', + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('lib/a.dart'), + LcovRecord('lib/b.dart', lines: {1: 1, 2: 1}), ]); - final metrics = CoverageMetrics.fromLcovRecords(records); - - expect(metrics.totalFound, equals(4)); - expect(metrics.totalHits, equals(4)); + expect(metrics.totalFound, equals(2)); + expect(metrics.totalHits, equals(2)); expect(metrics.uncoveredLines, isEmpty); }); - group('excludeFromCoverage', () { - test('handles null', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'LF:3', - 'LH:3', - 'end_of_record', - ]); - - final metrics = CoverageMetrics.fromLcovRecords(records); - - expect(metrics.totalFound, equals(3)); - expect(metrics.totalHits, equals(3)); - }); + test('keeps source paths that contain a colon', () { + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('C:/runner/lib/a.dart', lines: {1: 0}), + LcovRecord('D:/runner/lib/b.dart', lines: {1: 0}), + ]); - test('handles empty string', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'LF:3', - 'LH:3', - 'end_of_record', - ]); + expect( + metrics.uncoveredLines.keys, + equals(['C:/runner/lib/a.dart', 'D:/runner/lib/b.dart']), + ); + }); - final metrics = CoverageMetrics.fromLcovRecords( - records, - excludeFromCoverage: '', - ); + group('excludeFromCoverage', () { + final records = [ + LcovRecord('lib/a.dart', lines: {1: 1, 2: 0}), + LcovRecord('lib/generated/b.g.dart', lines: {1: 0}), + LcovRecord('lib/mocks/mock_c.dart', lines: {1: 0}), + ]; + + for (final (description, excludeFromCoverage) in [ + ('handles null', null), + ('handles empty string', ''), + ('does not exclude files when no glob matches', 'lib/other/**'), + ]) { + test(description, () { + final metrics = CoverageMetrics.fromLcov( + records, + excludeFromCoverage: excludeFromCoverage, + ); - expect(metrics.totalFound, equals(3)); - expect(metrics.totalHits, equals(3)); - }); + expect(metrics.totalFound, equals(4)); + expect(metrics.totalHits, equals(1)); + }); + } test('excludes a single glob-matched file', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'LF:10', - 'LH:8', - 'end_of_record', - 'SF:lib/generated/b.g.dart', - 'LF:5', - 'LH:5', - 'end_of_record', - ]); - - final metrics = CoverageMetrics.fromLcovRecords( + final metrics = CoverageMetrics.fromLcov( records, excludeFromCoverage: 'lib/generated/**', ); - expect(metrics.totalFound, equals(10)); - expect(metrics.totalHits, equals(8)); + expect(metrics.totalFound, equals(3)); + expect( + metrics.uncoveredLines.keys, + isNot(contains(startsWith('lib/generated'))), + ); }); - test('excludes multiple space-separated globs', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'LF:10', - 'LH:8', - 'end_of_record', - 'SF:lib/generated/b.g.dart', - 'LF:5', - 'LH:5', - 'end_of_record', - 'SF:lib/mocks/mock_c.dart', - 'LF:4', - 'LH:4', - 'end_of_record', - ]); - - final metrics = CoverageMetrics.fromLcovRecords( - records, - excludeFromCoverage: 'lib/generated/** lib/mocks/**', - ); + for (final excludeFromCoverage in [ + 'lib/generated/** lib/mocks/**', + 'lib/generated/** lib/mocks/**', + ]) { + test('excludes space-separated globs "$excludeFromCoverage"', () { + final metrics = CoverageMetrics.fromLcov( + records, + excludeFromCoverage: excludeFromCoverage, + ); - expect(metrics.totalFound, equals(10)); - expect(metrics.totalHits, equals(8)); - }); + expect(metrics.totalFound, equals(2)); + expect(metrics.totalHits, equals(1)); + }); + } - test('handles multiple consecutive spaces between globs', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'LF:10', - 'LH:8', - 'end_of_record', - 'SF:lib/generated/b.g.dart', - 'LF:5', - 'LH:5', - 'end_of_record', - 'SF:lib/mocks/mock_c.dart', - 'LF:4', - 'LH:4', - 'end_of_record', - ]); - - final metrics = CoverageMetrics.fromLcovRecords( - records, - excludeFromCoverage: 'lib/generated/** lib/mocks/**', - ); + test('excludes source paths that contain a colon', () { + final metrics = CoverageMetrics.fromLcov([ + LcovRecord('lib/a.dart', lines: {1: 1}), + LcovRecord('C:/runner/lib/b.g.dart', lines: {1: 0}), + ], excludeFromCoverage: '**/*.g.dart'); - expect(metrics.totalFound, equals(10)); - expect(metrics.totalHits, equals(8)); + expect(metrics.totalFound, equals(1)); + expect(metrics.totalHits, equals(1)); }); - test('does not exclude file when glob does not match', () { - final records = parseRecords([ - 'SF:lib/a.dart', - 'LF:5', - 'LH:5', - 'end_of_record', - ]); - - final metrics = CoverageMetrics.fromLcovRecords( - records, - excludeFromCoverage: 'lib/generated/**', + test('matches globs against paths relative to packagePaths', () { + final metrics = CoverageMetrics.fromLcov( + [ + LcovRecord('packages/foo/lib/a.dart', lines: {1: 1}), + LcovRecord('packages/foo/lib/gen/b.dart', lines: {1: 0}), + LcovRecord('packages/bar/lib/gen/c.dart', lines: {1: 0}), + ], + excludeFromCoverage: 'lib/gen/**', + packagePaths: ['packages/foo'], ); - expect(metrics.totalFound, equals(5)); - expect(metrics.totalHits, equals(5)); + expect(metrics.totalFound, equals(2)); + expect( + metrics.uncoveredLines.keys, + equals(['packages/bar/lib/gen/c.dart']), + ); }); }); }); - - group('fromLcov', () { - test('derives the totals and uncovered lines from the line hits', () { - final metrics = CoverageMetrics.fromLcov([ - LcovRecord('lib/a.dart', lines: {3: 0, 1: 2, 2: 0}), - LcovRecord('lib/b.dart', lines: {1: 1}), - ]); - - expect(metrics.totalFound, equals(4)); - expect(metrics.totalHits, equals(2)); - expect( - metrics.uncoveredLines, - equals({ - 'lib/a.dart': [2, 3], - }), - ); - }); - - test('keeps source paths that contain a colon', () { - final metrics = CoverageMetrics.fromLcov([ - LcovRecord('C:/runner/lib/a.dart', lines: {1: 0}), - LcovRecord('D:/runner/lib/b.dart', lines: {1: 0}), - ]); - - expect( - metrics.uncoveredLines.keys, - equals(['C:/runner/lib/a.dart', 'D:/runner/lib/b.dart']), - ); - }); - - test('excludes source paths that contain a colon', () { - final metrics = CoverageMetrics.fromLcov([ - LcovRecord('lib/a.dart', lines: {1: 1}), - LcovRecord('C:/runner/lib/b.g.dart', lines: {1: 0}), - ], excludeFromCoverage: '**/*.g.dart'); - - expect(metrics.totalFound, equals(1)); - expect(metrics.totalHits, equals(1)); - }); - }); }); } diff --git a/test/src/cli/lcov_merger_test.dart b/test/src/cli/lcov_merger_test.dart index fad5b5bc5..ba724d7cd 100644 --- a/test/src/cli/lcov_merger_test.dart +++ b/test/src/cli/lcov_merger_test.dart @@ -1,3 +1,5 @@ +import 'dart:io'; + import 'package:lcov_parser/lcov_parser.dart'; import 'package:path/path.dart' as p; import 'package:test/test.dart'; @@ -96,6 +98,17 @@ void main() { expect(record.lines, equals({1: 1})); }); + test('keeps a record without end_of_record before the next SF', () { + final [a, b] = parseLcov( + 'SF:a.dart\nDA:1,1\nSF:b.dart\nDA:2,0\nend_of_record\n', + ); + + expect(a.file, equals('a.dart')); + expect(a.lines, equals({1: 1})); + expect(b.file, equals('b.dart')); + expect(b.lines, equals({2: 0})); + }); + group('throws $FormatException', () { for (final (description, content) in [ ('for a line without a tag', 'SF:a.dart\nfoo\n'), @@ -212,10 +225,12 @@ end_of_record final records = Parser.parseLines(merged.split('\n')); final expected = Parser.parseLines(lcov95.split('\n')); - expect( - CoverageMetrics.fromLcovRecords(records).percentage, - equals(CoverageMetrics.fromLcovRecords(expected).percentage), - ); + List<(String?, int?, int?)> summaries(List records) => [ + for (final record in records) + (record.file, record.lines?.found, record.lines?.hit), + ]; + + expect(summaries(records), equals(summaries(expected))); }); test('sorts branches by line, block and branch', () { @@ -231,6 +246,63 @@ end_of_record }); }); + group(discoverLcovPackages, () { + late Directory directory; + + setUp(() { + directory = Directory.systemTemp.createTempSync(); + addTearDown(() => directory.deleteSync(recursive: true)); + }); + + void createPackage(String path, {bool withReport = true}) { + File(p.join(directory.path, path, 'pubspec.yaml')) + ..createSync(recursive: true) + ..writeAsStringSync('name: package'); + if (withReport) { + File(p.join(directory.path, path, 'coverage', 'lcov.info')) + ..createSync(recursive: true) + ..writeAsStringSync(''); + } + } + + test('returns the packages with a report, sorted', () { + createPackage('.'); + createPackage(p.join('packages', 'b')); + createPackage(p.join('packages', 'a')); + + expect( + discoverLcovPackages(directory.path), + equals(['.', p.join('packages', 'a'), p.join('packages', 'b')]), + ); + }); + + test('skips packages without a report', () { + createPackage(p.join('packages', 'a')); + createPackage(p.join('packages', 'b'), withReport: false); + + expect( + discoverLcovPackages(directory.path), + equals([p.join('packages', 'a')]), + ); + }); + + test('skips platform, build and tool directories', () { + createPackage(p.join('packages', 'a')); + createPackage(p.join('packages', 'a', 'build', 'generated')); + createPackage(p.join('packages', 'a', 'ios', 'plugin')); + createPackage(p.join('.dart_tool', 'cache')); + + expect( + discoverLcovPackages(directory.path), + equals([p.join('packages', 'a')]), + ); + }); + + test('returns nothing without packages', () { + expect(discoverLcovPackages(directory.path), isEmpty); + }); + }); + group(normalizeLcovRecords, () { final posix = p.Context(style: p.Style.posix, current: '/repo'); final windows = p.Context(style: p.Style.windows, current: r'C:\repo'); diff --git a/test/src/commands/coverage/commands/merge_test.dart b/test/src/commands/coverage/commands/merge_test.dart index 04a2c2c9e..31756bd7c 100644 --- a/test/src/commands/coverage/commands/merge_test.dart +++ b/test/src/commands/coverage/commands/merge_test.dart @@ -82,6 +82,11 @@ void _writeFile(String path, String content) { ..writeAsStringSync(content); } +void _writePackage(String path, String lcov) { + _writeFile(p.join(path, 'pubspec.yaml'), 'name: ${p.basename(path)}'); + _writeFile(p.join(path, 'coverage', 'lcov.info'), lcov); +} + void _writeShards() { _writeFile(p.join('shards', '1', 'lcov.info'), _shard1); _writeFile(p.join('shards', '2', 'lcov.info'), _shard2); @@ -279,20 +284,91 @@ void main() { }), ); - group('without lcov files', () { - void writePackage(String path, String lcov) { - _writeFile(p.join(path, 'pubspec.yaml'), 'name: ${p.basename(path)}'); - _writeFile(p.join(path, 'coverage', 'lcov.info'), lcov); - } + group('with package reports', () { + test( + 'rebases the given reports onto their package', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writePackage(p.join('packages', 'foo'), _shard1); + _writePackage(p.join('packages', 'bar'), _shard2); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'packages/foo/coverage/lcov.info', + 'packages/bar/coverage/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect( + _readOutput(), + allOf( + contains('SF:packages/foo/lib/a.dart\n'), + contains('SF:packages/foo/lib/b.dart\n'), + contains('SF:packages/bar/lib/a.dart\n'), + isNot(contains('SF:lib/')), + ), + ); + }), + ); + + test( + 'does not rebase a coverage/lcov.info report outside of a package', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeFile(p.join('shard', 'coverage', 'lcov.info'), _shard2); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shard/coverage/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), startsWith('SF:lib/a.dart\n')); + }), + ); + + test( + 'skips glob matches in platform, build and tool directories', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writePackage(p.join('packages', 'foo'), _shard1); + _writePackage(p.join('packages', 'foo', 'build', 'pkg'), _shard2); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'packages/**/lcov.info', + ]); + + expect(result, equals(ExitCode.success.code)); + expect(_readOutput(), isNot(contains('build'))); + verify( + () => logger.warn( + 'Skipping these reports matched by "packages/**/lcov.info", ' + 'since they are in platform, build or tool directories. Pass ' + 'their paths to merge them too:\n' + ' - ${p.join('packages', 'foo', 'build', 'pkg', 'coverage', 'lcov.info')}', + ), + ).called(1); + verify( + () => + logger.info('Merged 1 lcov report(s) into coverage/lcov.info'), + ).called(1); + }), + ); + }); + group('without lcov files', () { test( 'merges the report of every package, keeping their files distinct', withRunner((commandRunner, logger, pubUpdater, printLogs) async { _enterTempDirectory(); - writePackage(p.join('packages', 'foo'), _shard1); - writePackage(p.join('packages', 'bar'), _shard2); + _writePackage(p.join('packages', 'foo'), _shard1); + _writePackage(p.join('packages', 'bar'), _shard2); for (final ignored in ['build', '.dart_tool', '.fvm', 'ios']) { - writePackage(p.join('packages', 'foo', ignored, 'pkg'), _shard1); + _writePackage(p.join('packages', 'foo', ignored, 'pkg'), _shard1); } // A package without tests leaves no report behind. _writeFile(p.join('packages', 'baz', 'pubspec.yaml'), 'name: baz'); @@ -333,8 +409,8 @@ end_of_record 'skips the --output report', withRunner((commandRunner, logger, pubUpdater, printLogs) async { _enterTempDirectory(); - writePackage('.', 'SF:stale.dart\nDA:1,1\nend_of_record\n'); - writePackage('foo', _shard2); + _writePackage('.', 'SF:stale.dart\nDA:1,1\nend_of_record\n'); + _writePackage('foo', _shard2); final result = await commandRunner.run(['coverage', 'merge']); @@ -354,8 +430,8 @@ end_of_record 'merges the root report into a different --output', withRunner((commandRunner, logger, pubUpdater, printLogs) async { _enterTempDirectory(); - writePackage('.', _shard1); - writePackage('foo', _shard2); + _writePackage('.', _shard1); + _writePackage('foo', _shard2); final result = await commandRunner.run([ 'coverage', @@ -449,6 +525,68 @@ end_of_record }), ); + test( + 'when a glob only matches reports in ignored directories', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeFile(p.join('build', 'coverage', 'lcov.info'), _shard1); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'build/**.info', + ]); + + expect(result, equals(ExitCode.noInput.code)); + verify( + () => logger.warn( + any(that: contains(p.join('build', 'coverage', 'lcov.info'))), + ), + ).called(1); + verify(() => logger.err('No lcov report found at "build/**.info".')) + .called(1); + }), + ); + + test( + 'when an lcov file cannot be read', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + File('shard.info').writeAsBytesSync([0xff, 0xfe, 0xfd]); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shard.info', + ]); + + expect(result, equals(ExitCode.ioError.code)); + verify(() => logger.err(any(that: contains('FileSystemException')))) + .called(1); + }), + ); + + test( + 'when the --output cannot be written', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + Directory('out').createSync(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/1/lcov.info', + '-o', + 'out', + ]); + + expect(result, equals(ExitCode.ioError.code)); + verify(() => logger.err(any(that: contains('FileSystemException')))) + .called(1); + }), + ); + test( 'when a glob only matches the --output report', withRunner((commandRunner, logger, pubUpdater, printLogs) async { @@ -568,6 +706,35 @@ end_of_record }), ); + for (final glob in ['lib/src/gen/**', '**/*.g.dart']) { + test( + 'ignores package files matching the --exclude-coverage $glob', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writePackage( + p.join('packages', 'foo'), + 'SF:lib/a.dart\nDA:1,1\nend_of_record\n' + 'SF:lib/src/gen/b.g.dart\nDA:1,0\nend_of_record\n', + ); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + '--min-coverage', + '100', + '--exclude-coverage', + glob, + ]); + + expect(result, equals(ExitCode.success.code)); + expect( + _readOutput(), + contains('SF:packages/foo/lib/src/gen/b.g.dart\n'), + ); + }), + ); + } + test( 'ignores external Windows source paths matching --exclude-coverage', withRunner((commandRunner, logger, pubUpdater, printLogs) async { From a0aa5e9f0ee0bcd21ef12fca8b60a0e4332c3f80 Mon Sep 17 00:00:00 2001 From: Marcos Sevilla Date: Fri, 2 Oct 2026 16:33:58 +0300 Subject: [PATCH 14/14] refactor(coverage): tidy lcov handling and coverage merge inputs - Rename lcov_merger.dart to lcov.dart. - Make LcovRecord read-only and sum hits in a private builder. - Read the lcov 2 FN:,, layout, reject functions without a name, and pin extra BRDA fields as ignored. - Expand lcov globs in the CLI layer, and skip the --output report and ignored directories in one explicit step. - Read coverage options from a single very_good.yaml section. - Compare the e2e merged report per source file. - Link the sharding docs to the coverage CI workflow. Addresses FINDING-09 to FINDING-19 from review. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../commands/coverage/merge/merge_test.dart | 19 +- lib/src/cli/cli.dart | 3 +- lib/src/cli/{lcov_merger.dart => lcov.dart} | 173 +++++++++++------- lib/src/commands/coverage/commands/merge.dart | 100 ++++++---- site/docs/commands/coverage.md | 5 +- site/docs/commands/test.md | 32 +--- site/docs/configuration.md | 6 +- .../{lcov_merger_test.dart => lcov_test.dart} | 81 +++++++- .../coverage/commands/merge_test.dart | 48 +++++ 9 files changed, 324 insertions(+), 143 deletions(-) rename lib/src/cli/{lcov_merger.dart => lcov.dart} (60%) rename test/src/cli/{lcov_merger_test.dart => lcov_test.dart} (82%) diff --git a/e2e/test/commands/coverage/merge/merge_test.dart b/e2e/test/commands/coverage/merge/merge_test.dart index befdc76cf..1a610c1c2 100644 --- a/e2e/test/commands/coverage/merge/merge_test.dart +++ b/e2e/test/commands/coverage/merge/merge_test.dart @@ -3,13 +3,19 @@ import 'package:mocktail/mocktail.dart'; import 'package:path/path.dart' as path; import 'package:test/test.dart'; import 'package:universal_io/io.dart'; +import 'package:very_good_cli/src/cli/cli.dart'; import '../../../../helpers/helpers.dart'; -/// The lines of the lcov report at [filePath], sorted so reports listing the -/// same files in a different order compare equal. -List _sortedLines(String filePath) => - File(filePath).readAsLinesSync()..sort(); +/// The records of the lcov report at [filePath], merged and serialized per +/// source file, so reports listing the same files in a different order compare +/// equal while lines moved between files do not. +Map _recordsByFile(String filePath) => { + for (final record in mergeLcovRecords( + parseLcov(File(filePath).readAsStringSync()), + )) + record.file: record.toLcov(), +}; void main() { test( @@ -72,7 +78,10 @@ void main() { ]), completion(equals(ExitCode.success.code)), ); - expect(_sortedLines('merged.info'), equals(_sortedLines(unshardedPath))); + expect( + _recordsByFile('merged.info'), + equals(_recordsByFile(unshardedPath)), + ); await expectLater( commandRunner.run([ diff --git a/lib/src/cli/cli.dart b/lib/src/cli/cli.dart index f4c1bea51..47493fad8 100644 --- a/lib/src/cli/cli.dart +++ b/lib/src/cli/cli.dart @@ -5,6 +5,7 @@ import 'dart:math'; import 'package:collection/collection.dart'; import 'package:coverage/coverage.dart' as coverage; import 'package:glob/glob.dart'; +import 'package:glob/list_local_fs.dart'; import 'package:mason/mason.dart'; import 'package:meta/meta.dart'; import 'package:path/path.dart' as p; @@ -19,7 +20,7 @@ part 'coverage_check.dart'; part 'dart_cli.dart'; part 'flutter_cli.dart'; part 'git_cli.dart'; -part 'lcov_merger.dart'; +part 'lcov.dart'; part 'test_cli_runner.dart'; const R Function( diff --git a/lib/src/cli/lcov_merger.dart b/lib/src/cli/lcov.dart similarity index 60% rename from lib/src/cli/lcov_merger.dart rename to lib/src/cli/lcov.dart index 28fb08d22..fcda9c109 100644 --- a/lib/src/cli/lcov_merger.dart +++ b/lib/src/cli/lcov.dart @@ -14,14 +14,14 @@ class LcovRecord { /// {@macro lcov_record} new( this.file, { - Map? lines, - Map? functionLines, - Map? functionHits, - Map? branches, - }) : lines = lines ?? {}, - functionLines = functionLines ?? {}, - functionHits = functionHits ?? {}, - branches = branches ?? {}; + Map lines = const {}, + Map functionLines = const {}, + Map functionHits = const {}, + Map branches = const {}, + }) : lines = Map.unmodifiable(lines), + functionLines = Map.unmodifiable(functionLines), + functionHits = Map.unmodifiable(functionHits), + branches = Map.unmodifiable(branches); /// The source file path (`SF:`). final String file; @@ -41,46 +41,33 @@ class LcovRecord { /// A copy of this record for the source [file]. LcovRecord withFile(String file) => LcovRecord( file, - lines: {...lines}, - functionLines: {...functionLines}, - functionHits: {...functionHits}, - branches: {...branches}, + lines: lines, + functionLines: functionLines, + functionHits: functionHits, + branches: branches, ); - /// Adds the hits of [other] into this record. - void addAll(LcovRecord other) { - void sum(Map target, Map source) { - for (final MapEntry(:key, :value) in source.entries) { - target[key] = (target[key] ?? 0) + value; - } - } - - sum(lines, other.lines); - sum(functionHits, other.functionHits); - sum(branches, other.branches); - for (final MapEntry(:key, :value) in other.functionLines.entries) { - functionLines.putIfAbsent(key, () => value); - } - } - /// Serializes this record to lcov, ending with `end_of_record`. String toLcov() { final buffer = StringBuffer()..writeln('SF:$file'); if (functionLines.isNotEmpty) { - final names = functionLines.keys.sortedBy((n) => functionLines[n]!); - for (final name in names) { - buffer.writeln('FN:${functionLines[name]},$name'); + final functions = functionLines.entries.sortedBy( + (function) => function.value, + ); + for (final MapEntry(key: name, value: line) in functions) { + buffer.writeln('FN:$line,$name'); } - for (final name in names) { - final hits = functionHits[name] ?? 0; - if (hits > 0) buffer.writeln('FNDA:$hits,$name'); + final hitNames = [ + for (final MapEntry(key: name) in functions) + if ((functionHits[name] ?? 0) > 0) name, + ]; + for (final name in hitNames) { + buffer.writeln('FNDA:${functionHits[name]},$name'); } buffer - ..writeln('FNF:${names.length}') - ..writeln( - 'FNH:${names.where((n) => (functionHits[n] ?? 0) > 0).length}', - ); + ..writeln('FNF:${functions.length}') + ..writeln('FNH:${hitNames.length}'); } for (final line in lines.keys.sorted((a, b) => a - b)) { @@ -105,6 +92,52 @@ class LcovRecord { } } +/// Sums the hits of the lcov details of a single source [file], while parsing +/// or merging, then [build]s its [LcovRecord]. +class _LcovRecordBuilder { + new(this.file); + + final String file; + + final _lines = {}; + + final _functionLines = {}; + + final _functionHits = {}; + + final _branches = {}; + + void addLine(int line, int hits) => _sum(_lines, line, hits); + + void addFunction(String name, int line) => + _functionLines.putIfAbsent(name, () => line); + + void addFunctionHits(String name, int hits) => + _sum(_functionHits, name, hits); + + void addBranch(LcovBranch branch, int taken) => + _sum(_branches, branch, taken); + + /// Adds the hits of [record]. + void addAll(LcovRecord record) { + record.lines.forEach(addLine); + record.functionLines.forEach(addFunction); + record.functionHits.forEach(addFunctionHits); + record.branches.forEach(addBranch); + } + + LcovRecord build() => LcovRecord( + file, + lines: _lines, + functionLines: _functionLines, + functionHits: _functionHits, + branches: _branches, + ); + + static void _sum(Map hits, K key, int value) => + hits[key] = (hits[key] ?? 0) + value; +} + /// Orders branches by line, then block, then branch number. int _compareBranches(LcovBranch a, LcovBranch b) { final (aLine, aBlock, aBranch) = a; @@ -118,19 +151,22 @@ int _compareBranches(LcovBranch a, LcovBranch b) { /// /// Unlike `package:lcov_parser`, this tolerates CRLF line endings, blank /// lines, `:` and `,` in source paths and tags it doesn't know about, which -/// are ignored along with the `LF/LH/FNF/FNH/BRF/BRH` summaries. +/// are ignored along with the `LF/LH/FNF/FNH/BRF/BRH` summaries. Both the +/// `FN:,` layout of `package:coverage` and the +/// `FN:,,` one of lcov 2 are read, the end line being +/// ignored. /// /// Throws a [FormatException] when a line is malformed. List parseLcov(String content) { final records = []; - LcovRecord? record; + _LcovRecordBuilder? record; for (final rawLine in const LineSplitter().convert(content)) { final line = rawLine.trim(); if (line.isEmpty) continue; if (line == 'end_of_record') { - if (record != null) records.add(record); + if (record != null) records.add(record.build()); record = null; continue; } @@ -143,39 +179,42 @@ List parseLcov(String content) { final value = line.substring(separator + 1); final fields = value.split(','); + Never invalid() => throw FormatException('Invalid lcov line "$line".'); + int number(int index) => - int.tryParse(fields.elementAtOrNull(index) ?? '') ?? - (throw FormatException('Invalid lcov line "$line".')); + int.tryParse(fields.elementAtOrNull(index) ?? '') ?? invalid(); // Function names may contain commas, so they span the remaining fields. - String name() => fields.skip(1).join(','); + String name(int index) => switch (fields.skip(index).join(',')) { + '' => invalid(), + final name => name, + }; switch ((tag, record)) { case ('SF', _): // A record left without `end_of_record` is kept, as at the end of the // report, rather than dropped. - if (record != null) records.add(record); - record = LcovRecord(value); + if (record != null) records.add(record.build()); + record = _LcovRecordBuilder(value); case ('DA' || 'FN' || 'FNDA' || 'BRDA', null): throw FormatException('Found "$line" before any "SF:" line.'); - case ('DA', final LcovRecord current): - final lineNumber = number(0); - current.lines[lineNumber] = - (current.lines[lineNumber] ?? 0) + number(1); - case ('FN', final LcovRecord current): - current.functionLines[name()] = number(0); - case ('FNDA', final LcovRecord current): - current.functionHits[name()] = - (current.functionHits[name()] ?? 0) + number(0); - case ('BRDA', final LcovRecord current): - final branch = (number(0), number(1), number(2)); + case ('DA', final _LcovRecordBuilder current): + current.addLine(number(0), number(1)); + case ('FN', final _LcovRecordBuilder current): + // Dart function names can't start with a digit, so a numeric second + // field is the end line of lcov 2. + final hasEndLine = fields.length > 2 && int.tryParse(fields[1]) != null; + current.addFunction(name(hasEndLine ? 2 : 1), number(0)); + case ('FNDA', final _LcovRecordBuilder current): + current.addFunctionHits(name(1), number(0)); + case ('BRDA', final _LcovRecordBuilder current): // A `-` means the branch was never reached, which counts as not taken. final taken = fields.elementAtOrNull(3) == '-' ? 0 : number(3); - current.branches[branch] = (current.branches[branch] ?? 0) + taken; + current.addBranch((number(0), number(1), number(2)), taken); } } - if (record != null) records.add(record); + if (record != null) records.add(record.build()); return records; } @@ -237,17 +276,27 @@ List discoverLcovPackages(String cwd) => Directory(cwd) ) .sorted(); +/// The files matched by [glob], relative to [cwd] and sorted. +/// +/// Throws a [FormatException] when [glob] is invalid. +List expandLcovGlob(String glob, {required String cwd}) => + Glob(glob) + .listSync(root: cwd) + .whereType() + .map((file) => p.relative(file.path, from: cwd)) + .sorted(); + /// Merges [records] that describe the same source file, summing their hits. /// /// The result keeps the order in which each file first appears. List mergeLcovRecords(Iterable records) { - final merged = {}; + final merged = {}; for (final record in records) { merged - .putIfAbsent(record.file, () => LcovRecord(record.file)) + .putIfAbsent(record.file, () => _LcovRecordBuilder(record.file)) .addAll(record); } - return merged.values.toList(); + return [for (final record in merged.values) record.build()]; } /// Serializes [records] to lcov. diff --git a/lib/src/commands/coverage/commands/merge.dart b/lib/src/commands/coverage/commands/merge.dart index c9a1bff52..47efbf617 100644 --- a/lib/src/commands/coverage/commands/merge.dart +++ b/lib/src/commands/coverage/commands/merge.dart @@ -1,7 +1,5 @@ import 'package:args/command_runner.dart'; import 'package:collection/collection.dart'; -import 'package:glob/glob.dart'; -import 'package:glob/list_local_fs.dart'; import 'package:mason/mason.dart'; import 'package:path/path.dart' as p; import 'package:universal_io/io.dart'; @@ -67,18 +65,37 @@ class CoverageMergeCommand extends Command { final testConfig = config.test; final dartTestConfig = config.dart.test; + // The coverage options all come from the first section that sets any of + // them, so that a run never mixes the `test` and `dart.test` sections. + final (configMinCoverage, configExcludeCoverage, configShowUncovered) = + [ + ( + testConfig.minCoverage, + testConfig.excludeCoverage, + testConfig.showUncovered, + ), + ( + dartTestConfig.minCoverage, + dartTestConfig.excludeCoverage, + dartTestConfig.showUncovered, + ), + ].firstWhere( + (section) => section != (null, null, null), + orElse: () => (null, null, null), + ); + final rawMinCoverage = argResults.resolve( 'min-coverage', - testConfig.minCoverage ?? dartTestConfig.minCoverage, + configMinCoverage, ); final minCoverage = double.tryParse(rawMinCoverage ?? ''); final excludeFromCoverage = argResults.resolve( 'exclude-coverage', - testConfig.excludeCoverage ?? dartTestConfig.excludeCoverage, + configExcludeCoverage, ); final showUncovered = argResults.resolve( 'show-uncovered', - testConfig.showUncovered ?? dartTestConfig.showUncovered, + configShowUncovered, ); final output = p.normalize(argResults['output'] as String); @@ -176,14 +193,7 @@ class CoverageMergeCommand extends Command { final List matches; try { - matches = Glob(arg) - .listSync(root: cwd) - .whereType() - .map((file) => p.relative(file.path, from: cwd)) - .whereNot( - (path) => _isOutput(path, cwd: cwd, outputPath: outputPath), - ) - .sorted(); + matches = expandLcovGlob(arg, cwd: cwd); } on FormatException catch (error) { throw _MergeError( 'Invalid glob "$arg": ${error.message}', @@ -191,17 +201,15 @@ class CoverageMergeCommand extends Command { ); } - final skipped = matches.where(isInIgnoredDirectory).toList(); - if (skipped.isNotEmpty) { - _logger.warn( - 'Skipping these reports matched by "$arg", since they are in ' - 'platform, build or tool directories. Pass their paths to merge ' - 'them too:\n' - '${skipped.map((path) => ' - $path').join('\n')}', - ); - } - - final kept = matches.whereNot(isInIgnoredDirectory).toList(); + final kept = _skip( + _skipOutput(matches, cwd: cwd, outputPath: outputPath), + where: isInIgnoredDirectory, + warning: (skipped) => + 'Skipping these reports matched by "$arg", since they are in ' + 'platform, build or tool directories. Pass their paths to merge ' + 'them too:\n' + '${skipped.map((path) => ' - $path').join('\n')}', + ); if (kept.isEmpty) { throw _MergeError( 'No lcov report found at "$arg".', @@ -221,13 +229,14 @@ class CoverageMergeCommand extends Command { required String cwd, required String outputPath, }) { - final inputs = [ - for (final package in discoverLcovPackages(cwd)) - if (p.normalize(p.join(package, 'coverage', 'lcov.info')) - case final path - when !_isOutput(path, cwd: cwd, outputPath: outputPath)) - path, - ]; + final inputs = _skipOutput( + [ + for (final package in discoverLcovPackages(cwd)) + p.normalize(p.join(package, 'coverage', 'lcov.info')), + ], + cwd: cwd, + outputPath: outputPath, + ); if (inputs.isEmpty) { throw _MergeError( @@ -240,21 +249,32 @@ class CoverageMergeCommand extends Command { return inputs; } - /// Whether [path], relative to [cwd], is the report at [outputPath]. + /// [paths], relative to [cwd], without the report at [outputPath]. /// /// Such a report is likely a previous merge, so it is reported as skipped, /// since merging it again would count its hits twice. - bool _isOutput( - String path, { + List _skipOutput( + List paths, { required String cwd, required String outputPath, + }) => _skip( + paths, + where: (path) => p.equals(p.join(cwd, path), outputPath), + warning: (skipped) => + 'Skipping ${skipped.join(', ')}, since it is the --output report. ' + 'Pass a different --output to merge it too.', + ); + + /// [paths] without the ones matching [where], which are reported with the + /// [warning] built from them. + List _skip( + List paths, { + required bool Function(String path) where, + required String Function(List skipped) warning, }) { - if (!p.equals(p.join(cwd, path), outputPath)) return false; - _logger.warn( - 'Skipping $path, since it is the --output report. Pass a different ' - '--output to merge it too.', - ); - return true; + final skipped = paths.where(where).toList(); + if (skipped.isNotEmpty) _logger.warn(warning(skipped)); + return paths.whereNot(where).toList(); } /// The package directory, relative to [cwd], that the relative source paths diff --git a/site/docs/commands/coverage.md b/site/docs/commands/coverage.md index 933222136..d1f83ab61 100644 --- a/site/docs/commands/coverage.md +++ b/site/docs/commands/coverage.md @@ -80,8 +80,9 @@ Reports are merged by source file: The command has no section of its own in [`very_good.yaml`](../configuration.md). When a flag isn't passed, it reads the `min_coverage`, `exclude_coverage`, and `show_uncovered` values from the -`test` section, and then from the `dart.test` section. This lets the threshold -you already enforce in un-sharded runs apply to the merged report. +`test` section or, when it sets none of them, from the `dart.test` section. The +values of the two sections are never mixed. This lets the threshold you already +enforce in un-sharded runs apply to the merged report. ## Example CI workflow diff --git a/site/docs/commands/test.md b/site/docs/commands/test.md index 165917aea..0aff61650 100644 --- a/site/docs/commands/test.md +++ b/site/docs/commands/test.md @@ -133,36 +133,12 @@ Enforce it on the merged report instead: very_good coverage merge ``` `very_good coverage merge` reads the same `min_coverage`, so the merge job -enforces it without repeating the threshold. The full workflow, where a matrix -of shards uploads its reports and a final job downloads and merges them, looks -like this: - -```yaml -jobs: - test: - strategy: - matrix: - shard: [1, 2, 3] - steps: - - run: very_good test --coverage --shard-index ${{ matrix.shard }} --total-shards 3 - - uses: actions/upload-artifact@v4 - with: - name: coverage-${{ matrix.shard }} - path: coverage/lcov.info - - coverage: - needs: test - steps: - - uses: actions/download-artifact@v4 - with: - pattern: coverage-* - path: shards - - run: very_good coverage merge 'shards/*/lcov.info' -``` +enforces it without repeating the threshold. See the +[example CI workflow](coverage.md#example-ci-workflow), where a matrix of shards +uploads its reports and a final job downloads and merges them. A shard without tests still writes an empty `coverage/lcov.info`, which -`very_good coverage merge` accepts. See [Coverage](coverage.md) for the -complete workflow, including `--recursive` runs. +`very_good coverage merge` accepts. :::info Sharding requires the test optimizer, so it is rejected whenever the optimizer diff --git a/site/docs/configuration.md b/site/docs/configuration.md index 850271ff1..0dbe99e5d 100644 --- a/site/docs/configuration.md +++ b/site/docs/configuration.md @@ -217,9 +217,9 @@ dart: [`very_good coverage merge`](commands/coverage.md) has no section of its own. When a flag isn't passed, it reads `min_coverage`, `exclude_coverage`, and -`show_uncovered` from the `test` section, and then from the `dart.test` -section, so the threshold you enforce in un-sharded runs also applies to the -merged report. +`show_uncovered` from the `test` section or, when it sets none of them, from +the `dart.test` section, without mixing the two. The threshold you enforce in +un-sharded runs then also applies to the merged report. ### `packages.get` diff --git a/test/src/cli/lcov_merger_test.dart b/test/src/cli/lcov_test.dart similarity index 82% rename from test/src/cli/lcov_merger_test.dart rename to test/src/cli/lcov_test.dart index ba724d7cd..7d58b5f4b 100644 --- a/test/src/cli/lcov_merger_test.dart +++ b/test/src/cli/lcov_test.dart @@ -28,6 +28,32 @@ end_of_record '''; void main() { + group(LcovRecord, () { + test('cannot be modified', () { + final record = LcovRecord( + 'a.dart', + lines: {1: 1}, + functionLines: {'f': 1}, + functionHits: {'f': 1}, + branches: {(1, 0, 0): 1}, + ); + + expect(() => record.lines[1] = 2, throwsUnsupportedError); + expect(() => record.functionLines['f'] = 2, throwsUnsupportedError); + expect(() => record.functionHits['f'] = 2, throwsUnsupportedError); + expect(() => record.branches[(1, 0, 0)] = 2, throwsUnsupportedError); + }); + + test('is not affected by changes to the given maps', () { + final lines = {1: 1}; + final record = LcovRecord('a.dart', lines: lines); + + lines[1] = 2; + + expect(record.lines, equals({1: 1})); + }); + }); + group(parseLcov, () { test('parses lines, functions and branches', () { final [record] = parseLcov(_dartLcov); @@ -72,6 +98,22 @@ void main() { expect(record.functionHits, equals({'f': 1})); }); + test('reads the lcov 2 function layout, ignoring the end line', () { + final [record] = parseLcov( + 'SF:a.dart\nFN:1,3,main\nFN:5,9,f\nFNDA:1,main\n' + 'end_of_record\n', + ); + + expect(record.functionLines, equals({'main': 1, 'f': 5})); + expect(record.functionHits, equals({'main': 1})); + }); + + test('ignores extra branch fields', () { + final [record] = parseLcov('SF:a.dart\nBRDA:1,0,0,2,9\nend_of_record\n'); + + expect(record.branches, equals({(1, 0, 0): 2})); + }); + test('ignores unknown tags and summaries', () { final [record] = parseLcov( 'TN:\nVER:2\nSF:a.dart\nFNL:0,1,2\nDA:1,1\nLF:9\nLH:9\nend_of_record\n', @@ -115,6 +157,8 @@ void main() { ('for details before any SF', 'DA:1,1\n'), ('for a non numeric value', 'SF:a.dart\nDA:1,x\n'), ('for a missing value', 'SF:a.dart\nBRDA:1,0\n'), + ('for a function without a name', 'SF:a.dart\nFN:1\n'), + ('for function hits without a name', 'SF:a.dart\nFNDA:1,\n'), ]) { test(description, () { expect(() => parseLcov(content), throwsFormatException); @@ -246,6 +290,39 @@ end_of_record }); }); + group(expandLcovGlob, () { + late Directory directory; + + setUp(() { + directory = Directory.systemTemp.createTempSync(); + addTearDown(() => directory.deleteSync(recursive: true)); + }); + + test('returns the matched files, relative and sorted', () { + for (final shard in ['2', '1']) { + File(p.join(directory.path, 'shards', shard, 'lcov.info')) + ..createSync(recursive: true) + ..writeAsStringSync(''); + } + Directory(p.join(directory.path, 'shards', 'dir.info')).createSync(); + + expect( + expandLcovGlob('shards/**.info', cwd: directory.path), + equals([ + p.join('shards', '1', 'lcov.info'), + p.join('shards', '2', 'lcov.info'), + ]), + ); + }); + + test('throws $FormatException for an invalid glob', () { + expect( + () => expandLcovGlob('[', cwd: directory.path), + throwsFormatException, + ); + }); + }); + group(discoverLcovPackages, () { late Directory directory; @@ -383,10 +460,10 @@ end_of_record final record = LcovRecord('lib/a.dart', lines: {1: 1}); final [normalized] = normalizeLcovRecords([record], packagePath: 'foo'); - normalized.lines[1] = 2; + expect(normalized.file, equals('foo/lib/a.dart')); + expect(normalized.lines, equals({1: 1})); expect(record.file, equals('lib/a.dart')); - expect(record.lines, equals({1: 1})); }); }); } diff --git a/test/src/commands/coverage/commands/merge_test.dart b/test/src/commands/coverage/commands/merge_test.dart index 31756bd7c..5d86719a0 100644 --- a/test/src/commands/coverage/commands/merge_test.dart +++ b/test/src/commands/coverage/commands/merge_test.dart @@ -626,6 +626,28 @@ end_of_record }), ); + test( + 'when --min-coverage is empty', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/1/lcov.info', + '--min-coverage', + '', + ]); + + expect(result, equals(ExitCode.usage.code)); + verify( + () => logger.err('--min-coverage must be a number, but got "".'), + ).called(1); + expect(File('coverage/lcov.info').existsSync(), isFalse); + }), + ); + test( 'when very_good.yaml is invalid', withRunner((commandRunner, logger, pubUpdater, printLogs) async { @@ -825,6 +847,32 @@ end_of_record }), ); + test( + 'does not mix the test and dart test sections', + withRunner((commandRunner, logger, pubUpdater, printLogs) async { + _enterTempDirectory(); + _writeShards(); + _writeFile( + 'very_good.yaml', + 'test:\n min_coverage: 100\n' + 'dart:\n test:\n exclude_coverage: lib/b.dart\n', + ); + + final result = await commandRunner.run([ + 'coverage', + 'merge', + 'shards/*/lcov.info', + ]); + + expect(result, equals(ExitCode.software.code)); + verify( + () => logger.err( + 'Expected coverage >= 100.00% but actual is 66.67%.', + ), + ).called(1); + }), + ); + test( 'is overridden by command line arguments', withRunner((commandRunner, logger, pubUpdater, printLogs) async {