From 06d289792b298a204cb2d6129218b87e86d41f61 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 17 Oct 2023 02:40:42 +0000 Subject: [PATCH 1/6] Add coverableLineCache param to collect --- CHANGELOG.md | 8 ++ lib/src/collect.dart | 89 +++++++++++--- pubspec.yaml | 4 +- test/collect_coverage_mock_test.dart | 173 +++++++++++++++++++++++++++ 4 files changed, 253 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 71ca39cf..76272e30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,11 @@ +## 1.7.0 + +- Update `package:vm_service` constraints to '>=12.0.0 <13.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/lib/src/collect.dart b/lib/src/collect.dart index d33f9e92..25415006 100644 --- a/lib/src/collect.dart +++ b/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,18 @@ 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. + Set? coverableLines; + if (coverableLineCache != null) { + coverableLines = coverableLineCache[scriptUriString] ??= {}; + } // Not returned in scripts section of source report. if (scriptUri.scheme == 'evaluate') continue; @@ -379,7 +421,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 +436,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 +460,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/pubspec.yaml b/pubspec.yaml index a749847d..aab1d83e 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: coverage -version: 1.6.4 +version: 1.7.0 description: Coverage data manipulation and formatting repository: https://github.com/dart-lang/coverage @@ -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 <13.0.0' dev_dependencies: benchmark_harness: ^2.2.0 diff --git a/test/collect_coverage_mock_test.dart b/test/collect_coverage_mock_test.dart index 7ae61523..e36a8367 100644 --- a/test/collect_coverage_mock_test.dart +++ b/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}, + }); + }); }); } From 9ecb38a817d3d4dae50fbab191f46697b1c3b998 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 17 Oct 2023 03:23:22 +0000 Subject: [PATCH 2/6] Debugging collect_coverage_api_test --- test/collect_coverage_api_test.dart | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/test/collect_coverage_api_test.dart b/test/collect_coverage_api_test.dart index 832a7b8d..2b9f0072 100644 --- a/test/collect_coverage_api_test.dart +++ b/test/collect_coverage_api_test.dart @@ -95,13 +95,23 @@ 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); + + print(coverage); + print(coverableLineCache); + expect(false, true); + }, 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 From 0e55fccb37c69d71dc37f19e5cb3cb2cb2f1296e Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 17 Oct 2023 03:34:28 +0000 Subject: [PATCH 3/6] fmt --- test/collect_coverage_api_test.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/collect_coverage_api_test.dart b/test/collect_coverage_api_test.dart index 2b9f0072..1b4eee2c 100644 --- a/test/collect_coverage_api_test.dart +++ b/test/collect_coverage_api_test.dart @@ -98,7 +98,8 @@ void main() { test('collect_coverage_api with coverableLineCache', () async { final coverableLineCache = >{}; - final coverage = await _collectCoverage(coverableLineCache: coverableLineCache); + final coverage = + await _collectCoverage(coverableLineCache: coverableLineCache); print(coverage); print(coverableLineCache); From 0a8c4895b50002682e71ca5bd403f34d78a7c4de Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 17 Oct 2023 04:07:11 +0000 Subject: [PATCH 4/6] Add integration test --- test/collect_coverage_api_test.dart | 38 +++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/test/collect_coverage_api_test.dart b/test/collect_coverage_api_test.dart index 1b4eee2c..e41f5c14 100644 --- a/test/collect_coverage_api_test.dart +++ b/test/collect_coverage_api_test.dart @@ -100,10 +100,37 @@ void main() { final coverableLineCache = >{}; final coverage = await _collectCoverage(coverableLineCache: coverableLineCache); - - print(coverage); - print(coverableLineCache); - expect(false, true); + 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)); } @@ -125,5 +152,6 @@ Future> _collectCoverage( timeout: timeout, isolateIds: isolateIdSet, functionCoverage: functionCoverage, - branchCoverage: branchCoverage); + branchCoverage: branchCoverage, + coverableLineCache: coverableLineCache); } From 7ce547d9153fcc4c65f401488b5b7e7b72b2ad4f Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 17 Oct 2023 04:12:16 +0000 Subject: [PATCH 5/6] Bump min SDK version --- .github/workflows/test-package.yml | 2 +- CHANGELOG.md | 1 + pubspec.yaml | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test-package.yml b/.github/workflows/test-package.yml index 654443c2..dfd95dbe 100644 --- a/.github/workflows/test-package.yml +++ b/.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/CHANGELOG.md b/CHANGELOG.md index 76272e30..784c4561 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## 1.7.0 +- Require Dart 3.0.0 - Update `package:vm_service` constraints to '>=12.0.0 <13.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 diff --git a/pubspec.yaml b/pubspec.yaml index aab1d83e..77401ea0 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -4,7 +4,7 @@ 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 <4.0.0' dependencies: args: ^2.0.0 From 0b258e7ffd92fc3170a0ef940218eabaf82f3f70 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Tue, 17 Oct 2023 20:36:41 +0000 Subject: [PATCH 6/6] Ben's comments --- CHANGELOG.md | 2 +- lib/src/collect.dart | 6 ++---- pubspec.yaml | 4 ++-- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 784c4561..3da66c3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ ## 1.7.0 - Require Dart 3.0.0 -- Update `package:vm_service` constraints to '>=12.0.0 <13.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 diff --git a/lib/src/collect.dart b/lib/src/collect.dart index 25415006..03bed3ce 100644 --- a/lib/src/collect.dart +++ b/lib/src/collect.dart @@ -372,10 +372,8 @@ Future>> _processSourceReport( // 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. - Set? coverableLines; - if (coverableLineCache != null) { - coverableLines = coverableLineCache[scriptUriString] ??= {}; - } + final coverableLines = + coverableLineCache?.putIfAbsent(scriptUriString, () => {}); // Not returned in scripts section of source report. if (scriptUri.scheme == 'evaluate') continue; diff --git a/pubspec.yaml b/pubspec.yaml index 77401ea0..30293d14 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -4,7 +4,7 @@ description: Coverage data manipulation and formatting repository: https://github.com/dart-lang/coverage environment: - sdk: '>=3.0.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: '>=12.0.0 <13.0.0' + vm_service: '^12.0.0' dev_dependencies: benchmark_harness: ^2.2.0