From 070b491644ba17b53912494d5093f8389f36345e Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 17 Oct 2023 17:18:43 -0700 Subject: [PATCH] Add `coverableLineCache` param to `collect` (dart-lang/coverage#466) * Add coverableLineCache param to collect * Debugging collect_coverage_api_test * fmt * Add integration test * Bump min SDK version * Ben's comments --- .../.github/workflows/test-package.yml | 2 +- pkgs/coverage/CHANGELOG.md | 9 + pkgs/coverage/lib/src/collect.dart | 87 +++++++-- pkgs/coverage/pubspec.yaml | 6 +- .../test/collect_coverage_api_test.dart | 43 ++++- .../test/collect_coverage_mock_test.dart | 173 ++++++++++++++++++ 6 files changed, 295 insertions(+), 25 deletions(-) diff --git a/pkgs/coverage/.github/workflows/test-package.yml b/pkgs/coverage/.github/workflows/test-package.yml index 654443c2c..dfd95dbee 100644 --- a/pkgs/coverage/.github/workflows/test-package.yml +++ b/pkgs/coverage/.github/workflows/test-package.yml @@ -46,7 +46,7 @@ jobs: fail-fast: false matrix: os: [ubuntu-latest, macos-latest, windows-latest] - sdk: [2.19.0, dev] + sdk: [3.0.0, dev] steps: - uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 - uses: dart-lang/setup-dart@8a4b97ea2017cc079571daec46542f76189836b1 diff --git a/pkgs/coverage/CHANGELOG.md b/pkgs/coverage/CHANGELOG.md index 71ca39cfd..3da66c3d1 100644 --- a/pkgs/coverage/CHANGELOG.md +++ b/pkgs/coverage/CHANGELOG.md @@ -1,3 +1,12 @@ +## 1.7.0 + +- Require Dart 3.0.0 +- Update `package:vm_service` constraints to '^12.0.0'. +- Add `coverableLineCache` parameter to `collect`. This allows the set of + coverable lines to be cached between calls to `collect`, avoiding the need to + force compile the same libraries repeatedly. This is only useful when running + multiple coverage collections over the same libraries. + ## 1.6.4 - allow omitting space between `//` and `coverage` in coverage ignore comments diff --git a/pkgs/coverage/lib/src/collect.dart b/pkgs/coverage/lib/src/collect.dart index d33f9e927..03bed3ced 100644 --- a/pkgs/coverage/lib/src/collect.dart +++ b/pkgs/coverage/lib/src/collect.dart @@ -41,9 +41,15 @@ const _debugTokenPositions = bool.fromEnvironment('DEBUG_COVERAGE'); /// If [scopedOutput] is non-empty, coverage will be restricted so that only /// scripts that start with any of the provided paths are considered. /// -/// if [isolateIds] is set, the coverage gathering will be restricted to only +/// If [isolateIds] is set, the coverage gathering will be restricted to only /// those VM isolates. /// +/// If [coverableLineCache] is set, the collector will avoid recompiling +/// libraries it has already seen (see VmService.getSourceReport's +/// librariesAlreadyCompiled parameter). This is only useful when doing more +/// than one [collect] call over the same libraries. Pass an empty map to the +/// first call, and then pass the same map to all subsequent calls. +/// /// [serviceOverrideForTesting] is for internal testing only, and should not be /// set by users. Future> collect(Uri serviceUri, bool resume, @@ -52,6 +58,7 @@ Future> collect(Uri serviceUri, bool resume, Duration? timeout, bool functionCoverage = false, bool branchCoverage = false, + Map>? coverableLineCache, VmService? serviceOverrideForTesting}) async { scopedOutput ??= {}; @@ -92,7 +99,7 @@ Future> collect(Uri serviceUri, bool resume, } return await _getAllCoverage(service, includeDart, functionCoverage, - branchCoverage, scopedOutput, isolateIds); + branchCoverage, scopedOutput, isolateIds, coverableLineCache); } finally { if (resume) { await _resumeIsolates(service); @@ -115,7 +122,8 @@ Future> _getAllCoverage( bool functionCoverage, bool branchCoverage, Set? scopedOutput, - Set? isolateIds) async { + Set? isolateIds, + Map>? coverableLineCache) async { scopedOutput ??= {}; final vm = await service.getVM(); final allCoverage = >[]; @@ -124,16 +132,22 @@ Future> _getAllCoverage( final branchCoverageSupported = _versionCheck(version, 3, 56); final libraryFilters = _versionCheck(version, 3, 57); final fastIsoGroups = _versionCheck(version, 3, 61); + final lineCacheSupported = _versionCheck(version, 4, 13); + if (branchCoverage && !branchCoverageSupported) { branchCoverage = false; stderr.writeln('Branch coverage was requested, but is not supported' ' by the VM version. Try updating to a newer version of Dart'); } + final sourceReportKinds = [ SourceReportKind.kCoverage, if (branchCoverage) SourceReportKind.kBranchCoverage, ]; + final librariesAlreadyCompiled = + lineCacheSupported ? coverableLineCache?.keys.toList() : null; + // Program counters are shared between isolates in the same group. So we need // to make sure we're only gathering coverage data for one isolate in each // group, otherwise we'll double count the hits. @@ -173,15 +187,24 @@ Future> _getAllCoverage( late final SourceReport scriptReport; try { scriptReport = await service.getSourceReport( - isolateRef.id!, sourceReportKinds, - forceCompile: true, - scriptId: script.id, - reportLines: reportLines ? true : null); + isolateRef.id!, + sourceReportKinds, + forceCompile: true, + scriptId: script.id, + reportLines: reportLines ? true : null, + librariesAlreadyCompiled: librariesAlreadyCompiled, + ); } on SentinelException { continue; } - final coverage = await _getCoverageJson(service, isolateRef, - scriptReport, includeDart, functionCoverage, reportLines); + final coverage = await _processSourceReport( + service, + isolateRef, + scriptReport, + includeDart, + functionCoverage, + reportLines, + coverableLineCache); allCoverage.addAll(coverage); } } else { @@ -195,12 +218,19 @@ Future> _getAllCoverage( libraryFilters: scopedOutput.isNotEmpty && libraryFilters ? List.from(scopedOutput.map((filter) => 'package:$filter/')) : null, + librariesAlreadyCompiled: librariesAlreadyCompiled, ); } on SentinelException { continue; } - final coverage = await _getCoverageJson(service, isolateRef, - isolateReport, includeDart, functionCoverage, reportLines); + final coverage = await _processSourceReport( + service, + isolateRef, + isolateReport, + includeDart, + functionCoverage, + reportLines, + coverableLineCache); allCoverage.addAll(coverage); } } @@ -276,13 +306,14 @@ int? _getLineFromTokenPos(Script script, int tokenPos) { } /// Returns a JSON coverage list backward-compatible with pre-1.16.0 SDKs. -Future>> _getCoverageJson( +Future>> _processSourceReport( VmService service, IsolateRef isolateRef, SourceReport report, bool includeDart, bool functionCoverage, - bool reportLines) async { + bool reportLines, + Map>? coverableLineCache) async { final hitMaps = {}; final scripts = {}; final libraries = {}; @@ -333,7 +364,16 @@ Future>> _getCoverageJson( for (var range in report.ranges!) { final scriptRef = report.scripts![range.scriptIndex!]; - final scriptUri = Uri.parse(scriptRef.uri!); + final scriptUriString = scriptRef.uri!; + final scriptUri = Uri.parse(scriptUriString); + + // If we have a coverableLineCache, use it in the same way we use + // SourceReportCoverage.misses: to add zeros to the coverage result for all + // the lines that don't have a hit. Afterwards, add all the lines that were + // hit or missed to the cache, so that the next coverage collection won't + // need to compile this libarry. + final coverableLines = + coverableLineCache?.putIfAbsent(scriptUriString, () => {}); // Not returned in scripts section of source report. if (scriptUri.scheme == 'evaluate') continue; @@ -379,7 +419,8 @@ Future>> _getCoverageJson( if (coverage == null) continue; - void forEachLine(List tokenPositions, void Function(int line) body) { + void forEachLine(List? tokenPositions, void Function(int line) body) { + if (tokenPositions == null) return; for (final pos in tokenPositions) { final line = reportLines ? pos : _getLineFromTokenPos(script!, pos); if (line == null) { @@ -393,14 +434,22 @@ Future>> _getCoverageJson( } } - forEachLine(coverage.hits!, (line) { + if (coverableLines != null) { + for (final line in coverableLines) { + hits.lineHits.putIfAbsent(line, () => 0); + } + } + + forEachLine(coverage.hits, (line) { hits.lineHits.increment(line); + coverableLines?.add(line); if (hits.funcNames != null && hits.funcNames!.containsKey(line)) { hits.funcHits!.increment(line); } }); - forEachLine(coverage.misses!, (line) { + forEachLine(coverage.misses, (line) { hits.lineHits.putIfAbsent(line, () => 0); + coverableLines?.add(line); }); hits.funcNames?.forEach((line, funcName) { hits.funcHits?.putIfAbsent(line, () => 0); @@ -409,10 +458,10 @@ Future>> _getCoverageJson( final branchCoverage = range.branchCoverage; if (branchCoverage != null) { hits.branchHits ??= {}; - forEachLine(branchCoverage.hits!, (line) { + forEachLine(branchCoverage.hits, (line) { hits.branchHits!.increment(line); }); - forEachLine(branchCoverage.misses!, (line) { + forEachLine(branchCoverage.misses, (line) { hits.branchHits!.putIfAbsent(line, () => 0); }); } diff --git a/pkgs/coverage/pubspec.yaml b/pkgs/coverage/pubspec.yaml index a749847dc..30293d142 100644 --- a/pkgs/coverage/pubspec.yaml +++ b/pkgs/coverage/pubspec.yaml @@ -1,10 +1,10 @@ name: coverage -version: 1.6.4 +version: 1.7.0 description: Coverage data manipulation and formatting repository: https://github.com/dart-lang/coverage environment: - sdk: '>=2.18.0 <4.0.0' + sdk: '^3.0.0' dependencies: args: ^2.0.0 @@ -13,7 +13,7 @@ dependencies: path: ^1.8.0 source_maps: ^0.10.10 stack_trace: ^1.10.0 - vm_service: '>=11.9.0 <13.0.0' + vm_service: '^12.0.0' dev_dependencies: benchmark_harness: ^2.2.0 diff --git a/pkgs/coverage/test/collect_coverage_api_test.dart b/pkgs/coverage/test/collect_coverage_api_test.dart index 832a7b8dd..e41f5c148 100644 --- a/pkgs/coverage/test/collect_coverage_api_test.dart +++ b/pkgs/coverage/test/collect_coverage_api_test.dart @@ -95,13 +95,51 @@ void main() { expect(sources[_isolateLibFileUri], everyElement(containsPair('branchHits', isNotEmpty))); }, skip: !platformVersionCheck(2, 17)); + + test('collect_coverage_api with coverableLineCache', () async { + final coverableLineCache = >{}; + final coverage = + await _collectCoverage(coverableLineCache: coverableLineCache); + final result = await HitMap.parseJson( + coverage['coverage'] as List>); + + expect(coverableLineCache, contains(_sampleAppFileUri)); + expect(coverableLineCache, contains(_isolateLibFileUri)); + + // Expect that we have some missed lines. + expect(result[_sampleAppFileUri]!.lineHits.containsValue(0), isTrue); + expect(result[_isolateLibFileUri]!.lineHits.containsValue(0), isTrue); + + // Clear _sampleAppFileUri's cache entry, then gather coverage again. We're + // doing this to verify that force compilation is disabled for these + // libraries. The result should be that _isolateLibFileUri should be the + // same, but _sampleAppFileUri should be missing all its missed lines. + coverableLineCache[_sampleAppFileUri] = {}; + final coverage2 = + await _collectCoverage(coverableLineCache: coverableLineCache); + final result2 = await HitMap.parseJson( + coverage2['coverage'] as List>); + + // _isolateLibFileUri still has missed lines, but _sampleAppFileUri doesn't. + expect(result2[_sampleAppFileUri]!.lineHits.containsValue(0), isFalse); + expect(result2[_isolateLibFileUri]!.lineHits.containsValue(0), isTrue); + + // _isolateLibFileUri is the same. _sampleAppFileUri is the same, but + // without all its missed lines. + expect(result2[_isolateLibFileUri]!.lineHits, + result[_isolateLibFileUri]!.lineHits); + result[_sampleAppFileUri]!.lineHits.removeWhere((line, hits) => hits == 0); + expect(result2[_sampleAppFileUri]!.lineHits, + result[_sampleAppFileUri]!.lineHits); + }, skip: !platformVersionCheck(3, 2)); } Future> _collectCoverage( {Set scopedOutput = const {}, bool isolateIds = false, bool functionCoverage = false, - bool branchCoverage = false}) async { + bool branchCoverage = false, + Map>? coverableLineCache}) async { final openPort = await getOpenPort(); // run the sample app, with the right flags @@ -114,5 +152,6 @@ Future> _collectCoverage( timeout: timeout, isolateIds: isolateIdSet, functionCoverage: functionCoverage, - branchCoverage: branchCoverage); + branchCoverage: branchCoverage, + coverableLineCache: coverableLineCache); } diff --git a/pkgs/coverage/test/collect_coverage_mock_test.dart b/pkgs/coverage/test/collect_coverage_mock_test.dart index 7ae615233..e36a83671 100644 --- a/pkgs/coverage/test/collect_coverage_mock_test.dart +++ b/pkgs/coverage/test/collect_coverage_mock_test.dart @@ -455,5 +455,178 @@ void main() { expect(result.length, 0); }); + + test('Collect coverage, coverableLineCache, old vm service', () async { + // Expect that getSourceReport's librariesAlreadyCompiled param is not set + // when coverableLineCache is non-null but the service version is too old. + final service = _mockService(4, 12); + when(service.getSourceReport('isolate', ['Coverage'], + forceCompile: true, reportLines: true)) + .thenAnswer((_) async => SourceReport( + ranges: [ + _range( + 0, + SourceReportCoverage( + hits: [12], + misses: [47], + ), + ), + _range( + 1, + SourceReportCoverage( + hits: [95], + misses: [52], + ), + ), + ], + scripts: [ + ScriptRef( + uri: 'package:foo/foo.dart', + id: 'foo', + ), + ScriptRef( + uri: 'package:bar/bar.dart', + id: 'bar', + ), + ], + )); + + final coverableLineCache = >{}; + final jsonResult = await collect(Uri(), false, false, false, null, + coverableLineCache: coverableLineCache, + serviceOverrideForTesting: service); + final result = await HitMap.parseJson( + jsonResult['coverage'] as List>); + + expect(result.length, 2); + expect(result['package:foo/foo.dart']?.lineHits, {12: 1, 47: 0}); + expect(result['package:bar/bar.dart']?.lineHits, {95: 1, 52: 0}); + }); + + test('Collect coverage, coverableLineCache', () async { + // Expect that on the first getSourceReport call, librariesAlreadyCompiled + // is empty. + final service = _mockService(4, 13); + when( + service.getSourceReport('isolate', ['Coverage'], + forceCompile: true, + reportLines: true, + librariesAlreadyCompiled: [])) + .thenAnswer((_) async => SourceReport( + ranges: [ + _range( + 0, + SourceReportCoverage( + hits: [12], + misses: [47], + ), + ), + _range( + 1, + SourceReportCoverage( + hits: [95], + misses: [52], + ), + ), + ], + scripts: [ + ScriptRef( + uri: 'package:foo/foo.dart', + id: 'foo', + ), + ScriptRef( + uri: 'package:bar/bar.dart', + id: 'bar', + ), + ], + )); + + final coverableLineCache = >{}; + final jsonResult = await collect(Uri(), false, false, false, null, + coverableLineCache: coverableLineCache, + serviceOverrideForTesting: service); + final result = await HitMap.parseJson( + jsonResult['coverage'] as List>); + + expect(result.length, 2); + expect(result['package:foo/foo.dart']?.lineHits, {12: 1, 47: 0}); + expect(result['package:bar/bar.dart']?.lineHits, {95: 1, 52: 0}); + + // The coverableLineCache should now be filled with all the lines that + // were hit or missed. + expect(coverableLineCache, { + 'package:foo/foo.dart': {12, 47}, + 'package:bar/bar.dart': {95, 52}, + }); + + // The second getSourceReport call should now list all the libraries we've + // seen. The response won't contain any misses for these libraries, + // because they won't be force compiled. We'll also return a 3rd library, + // which will contain misses, as it hasn't been compiled yet. + when( + service.getSourceReport('isolate', ['Coverage'], + forceCompile: true, + reportLines: true, + librariesAlreadyCompiled: [ + 'package:foo/foo.dart', + 'package:bar/bar.dart' + ])).thenAnswer((_) async => SourceReport( + ranges: [ + _range( + 0, + SourceReportCoverage( + hits: [47], + ), + ), + _range( + 1, + SourceReportCoverage( + hits: [95], + ), + ), + _range( + 2, + SourceReportCoverage( + hits: [36], + misses: [81], + ), + ), + ], + scripts: [ + ScriptRef( + uri: 'package:foo/foo.dart', + id: 'foo', + ), + ScriptRef( + uri: 'package:bar/bar.dart', + id: 'bar', + ), + ScriptRef( + uri: 'package:baz/baz.dart', + id: 'baz', + ), + ], + )); + + final jsonResult2 = await collect(Uri(), false, false, false, null, + coverableLineCache: coverableLineCache, + serviceOverrideForTesting: service); + final result2 = await HitMap.parseJson( + jsonResult2['coverage'] as List>); + + // The missed lines still appear in foo and bar, even though they weren't + // returned in the response. They were read from the cache. + expect(result2.length, 3); + expect(result2['package:foo/foo.dart']?.lineHits, {12: 0, 47: 1}); + expect(result2['package:bar/bar.dart']?.lineHits, {95: 1, 52: 0}); + expect(result2['package:baz/baz.dart']?.lineHits, {36: 1, 81: 0}); + + // The coverableLineCache should now also contain the baz library. + expect(coverableLineCache, { + 'package:foo/foo.dart': {12, 47}, + 'package:bar/bar.dart': {95, 52}, + 'package:baz/baz.dart': {36, 81}, + }); + }); }); }