From 8df62188f012e5451374d2ba8dc6b77cb1fd2b46 Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Tue, 24 Oct 2023 02:38:27 +0000 Subject: [PATCH] Reverts "Use `coverage.collect`'s `coverableLineCache` param to speed up coverage" (#137121) Reverts flutter/flutter#136851 Initiated by: CaseyHillers This change reverts the following previous change: Original Description: One of the reasons gathering coverage information is expensive is that we have to force compile every function in the libraries we're interested in. Without this, functions that haven't been invoked (so haven't been compiled) won't have any line number information, so the coverage tool doesn't know which lines to add to the list of misses. In flutter's case, the test infra spawns many VMs, and each of these needs to recompile all those libraries. To fix this, we need a way of skipping force compilation for libraries we've already seen in previous tests, without losing the information about which lines in each library are coverable. So I [added](https://github.com/dart-lang/coverage/pull/466) the `coverableLineCache` to `coverage.collect` in package:coverage v1.7.0. This cache starts out empty, but fills up with lists of all the lines that are coverable for every library as coverage is gathered. package:coverage can then tell the VM not to force compile any libraries in this cache (using `getSourceReport`'s `librariesAlreadyCompiled` param). So the first test suite will still have to compile everything, but subsequent test suites will be much faster. This speeds up coverage collection significantly, for large test suites: | Running flutter/packages/flutter tests... | Time | Overhead | | --- | --- | --- | | without coverage | 8:53 | - | | with coverage | 20:25 | 130% | | with `coverableLineCache` | 12:21 | 40% | Bug: https://github.com/flutter/flutter/issues/100751 --- .../lib/src/test/coverage_collector.dart | 25 +-- .../coverage_collector_test.dart | 179 ------------------ 2 files changed, 7 insertions(+), 197 deletions(-) diff --git a/packages/flutter_tools/lib/src/test/coverage_collector.dart b/packages/flutter_tools/lib/src/test/coverage_collector.dart index 7b88837ffe9..48a5da2596d 100644 --- a/packages/flutter_tools/lib/src/test/coverage_collector.dart +++ b/packages/flutter_tools/lib/src/test/coverage_collector.dart @@ -37,7 +37,6 @@ class CoverageCollector extends TestWatcher { final coverage.Resolver? resolver; final Map>?> _ignoredLinesInFilesCache = >?>{}; - final Map> _coverableLineCache = >{}; final TestTimeRecorder? testTimeRecorder; @@ -104,11 +103,7 @@ class CoverageCollector extends TestWatcher { Future collectCoverageIsolate(Uri vmServiceUri) async { _logMessage('collecting coverage data from $vmServiceUri...'); final Map data = await collect( - vmServiceUri, - libraryNames, - branchCoverage: branchCoverage, - coverableLineCache: _coverableLineCache, - ); + vmServiceUri, libraryNames, branchCoverage: branchCoverage); _logMessage('($vmServiceUri): collected coverage data; merging...'); _addHitmap(await coverage.HitMap.parseJson( @@ -150,12 +145,9 @@ class CoverageCollector extends TestWatcher { .then((Uri? vmServiceUri) { _logMessage('collecting coverage data from $testDevice at $vmServiceUri...'); return collect( - vmServiceUri!, - libraryNames, - serviceOverride: serviceOverride, - branchCoverage: branchCoverage, - coverableLineCache: _coverableLineCache, - ).then((Map result) { + vmServiceUri!, libraryNames, serviceOverride: serviceOverride, + branchCoverage: branchCoverage) + .then((Map result) { _logMessage('Collected coverage data.'); data = result; }); @@ -275,12 +267,9 @@ Future> collect(Uri serviceUri, Set? libraryNames, @visibleForTesting bool forceSequential = false, @visibleForTesting FlutterVmService? serviceOverride, bool branchCoverage = false, - required Map> coverableLineCache, }) { return coverage.collect( - serviceUri, false, false, false, libraryNames, - serviceOverrideForTesting: serviceOverride?.service, - branchCoverage: branchCoverage, - coverableLineCache: coverableLineCache, - ); + serviceUri, false, false, false, libraryNames, + serviceOverrideForTesting: serviceOverride?.service, + branchCoverage: branchCoverage); } diff --git a/packages/flutter_tools/test/general.shard/coverage_collector_test.dart b/packages/flutter_tools/test/general.shard/coverage_collector_test.dart index 695ae3bee30..9ecce507434 100644 --- a/packages/flutter_tools/test/general.shard/coverage_collector_test.dart +++ b/packages/flutter_tools/test/general.shard/coverage_collector_test.dart @@ -53,7 +53,6 @@ void main() { Uri(), {'foo'}, serviceOverride: fakeVmServiceHost.vmService, - coverableLineCache: >{}, ); expect(result, {'type': 'CodeCoverage', 'coverage': []}); @@ -124,7 +123,6 @@ void main() { Uri(), {'foo'}, serviceOverride: fakeVmServiceHost.vmService, - coverableLineCache: >{}, ); expect(result, { @@ -153,7 +151,6 @@ void main() { Uri(), null, serviceOverride: fakeVmServiceHost.vmService, - coverableLineCache: >{}, ); expect(result, { @@ -240,7 +237,6 @@ void main() { Uri(), {'foo'}, serviceOverride: fakeVmServiceHost.vmService, - coverableLineCache: >{}, ); expect(result, { @@ -315,7 +311,6 @@ void main() { Uri(), null, serviceOverride: fakeVmServiceHost.vmService, - coverableLineCache: >{}, ); expect(result, { @@ -406,7 +401,6 @@ void main() { {'foo'}, serviceOverride: fakeVmServiceHost.vmService, branchCoverage: true, - coverableLineCache: >{}, ); expect(result, { @@ -607,179 +601,6 @@ void main() { tempDir?.deleteSync(recursive: true); } }); - - testWithoutContext('Coverage collector fills coverableLineCache', () async { - final FakeVmServiceHost fakeVmServiceHost = FakeVmServiceHost( - requests: [ - FakeVmServiceRequest( - method: 'getVM', - jsonResponse: (VM.parse({})! - ..isolates = [ - IsolateRef.parse({ - 'id': '1', - })!, - ] - ).toJson(), - ), - FakeVmServiceRequest( - method: 'getVersion', - jsonResponse: Version(major: 4, minor: 13).toJson(), - ), - FakeVmServiceRequest( - method: 'getSourceReport', - args: { - 'isolateId': '1', - 'reports': ['Coverage'], - 'forceCompile': true, - 'reportLines': true, - 'libraryFilters': ['package:foo/'], - 'librariesAlreadyCompiled': [], - }, - jsonResponse: SourceReport( - ranges: [ - SourceReportRange( - scriptIndex: 0, - startPos: 0, - endPos: 0, - compiled: true, - coverage: SourceReportCoverage( - hits: [1, 3], - misses: [2], - ), - ), - ], - scripts: [ - ScriptRef( - uri: 'package:foo/foo.dart', - id: '1', - ), - ], - ).toJson(), - ), - ], - ); - - final Map> coverableLineCache = >{}; - final Map result = await collect( - Uri(), - {'foo'}, - serviceOverride: fakeVmServiceHost.vmService, - coverableLineCache: coverableLineCache, - ); - - expect(result, { - 'type': 'CodeCoverage', - 'coverage': [ - { - 'source': 'package:foo/foo.dart', - 'script': { - 'type': '@Script', - 'fixedId': true, - 'id': 'libraries/1/scripts/package%3Afoo%2Ffoo.dart', - 'uri': 'package:foo/foo.dart', - '_kind': 'library', - }, - 'hits': [1, 1, 3, 1, 2, 0], - }, - ], - }); - - // coverableLineCache should contain every line mentioned in the report. - expect(coverableLineCache, >{ - 'package:foo/foo.dart': {1, 2, 3}, - }); - - expect(fakeVmServiceHost.hasRemainingExpectations, false); - }); - - testWithoutContext('Coverage collector avoids recompiling libraries in coverableLineCache', () async { - final FakeVmServiceHost fakeVmServiceHost = FakeVmServiceHost( - requests: [ - FakeVmServiceRequest( - method: 'getVM', - jsonResponse: (VM.parse({})! - ..isolates = [ - IsolateRef.parse({ - 'id': '1', - })!, - ] - ).toJson(), - ), - FakeVmServiceRequest( - method: 'getVersion', - jsonResponse: Version(major: 4, minor: 13).toJson(), - ), - - // This collection sets librariesAlreadyCompiled. The response doesn't - // include any misses. - FakeVmServiceRequest( - method: 'getSourceReport', - args: { - 'isolateId': '1', - 'reports': ['Coverage'], - 'forceCompile': true, - 'reportLines': true, - 'libraryFilters': ['package:foo/'], - 'librariesAlreadyCompiled': ['package:foo/foo.dart'], - }, - jsonResponse: SourceReport( - ranges: [ - SourceReportRange( - scriptIndex: 0, - startPos: 0, - endPos: 0, - compiled: true, - coverage: SourceReportCoverage( - hits: [1, 3], - misses: [], - ), - ), - ], - scripts: [ - ScriptRef( - uri: 'package:foo/foo.dart', - id: '1', - ), - ], - ).toJson(), - ), - ], - ); - - final Map> coverableLineCache = >{ - 'package:foo/foo.dart': {1, 2, 3}, - }; - final Map result2 = await collect( - Uri(), - {'foo'}, - serviceOverride: fakeVmServiceHost.vmService, - coverableLineCache: coverableLineCache, - ); - - // Expect that line 2 is marked as missed, even though it wasn't mentioned - // in the getSourceReport response. - expect(result2, { - 'type': 'CodeCoverage', - 'coverage': [ - { - 'source': 'package:foo/foo.dart', - 'script': { - 'type': '@Script', - 'fixedId': true, - 'id': 'libraries/1/scripts/package%3Afoo%2Ffoo.dart', - 'uri': 'package:foo/foo.dart', - '_kind': 'library', - }, - 'hits': [1, 1, 2, 0, 3, 1], - }, - ], - }); - expect(coverableLineCache, >{ - 'package:foo/foo.dart': {1, 2, 3}, - }); - - expect(fakeVmServiceHost.hasRemainingExpectations, false); - }); } File writeFooBarPackagesJson(Directory tempDir) {