From 803ef6a456c3127a4c5509eb726f9319569edb3d Mon Sep 17 00:00:00 2001 From: jensjoha Date: Wed, 13 Jul 2022 11:07:59 +0200 Subject: [PATCH] Improve coverage speed by using new caching option for package:coverage (#107395) Speedup coverage test runs by using (new) coverage handle with caching. On a `flutter test --coverage` run on `packages/flutter` the runtime goes from ~828 seconds to ~617 seconds, or a ~25% reduction in time spent (testing without coverage takes ~236 seconds so the overhead of `--coverage` in this case goes from ~592 seconds to ~381 seconds, or a ~35% reduction). --- .../flutter_tools/bin/fuchsia_tester.dart | 6 +- .../flutter_tools/lib/src/commands/test.dart | 3 +- .../lib/src/test/coverage_collector.dart | 34 ++- .../coverage_collector_test.dart | 274 ++++++++++++------ 4 files changed, 216 insertions(+), 101 deletions(-) diff --git a/packages/flutter_tools/bin/fuchsia_tester.dart b/packages/flutter_tools/bin/fuchsia_tester.dart index 848ed9bda1f4..4270ee87471c 100644 --- a/packages/flutter_tools/bin/fuchsia_tester.dart +++ b/packages/flutter_tools/bin/fuchsia_tester.dart @@ -116,9 +116,11 @@ Future run(List args) async { // setting libraryNames to null. final Set libraryNames = coverageDirectory != null ? null : {FlutterProject.current().manifest.appName}; + final String packagesPath = globals.fs.path.normalize(globals.fs.path.absolute(argResults[_kOptionPackages] as String)); collector = CoverageCollector( - packagesPath: globals.fs.path.normalize(globals.fs.path.absolute(argResults[_kOptionPackages] as String)), - libraryNames: libraryNames); + packagesPath: packagesPath, + libraryNames: libraryNames, + resolver: await CoverageCollector.getResolver(packagesPath)); if (!argResults.options.contains(_kOptionTestDirectory)) { throwToolExit('Use of --coverage requires setting --test-directory'); } diff --git a/packages/flutter_tools/lib/src/commands/test.dart b/packages/flutter_tools/lib/src/commands/test.dart index 748d01bb70b6..8473989b6811 100644 --- a/packages/flutter_tools/lib/src/commands/test.dart +++ b/packages/flutter_tools/lib/src/commands/test.dart @@ -374,7 +374,8 @@ class TestCommand extends FlutterCommand with DeviceBasedDevelopmentArtifacts { collector = CoverageCollector( verbose: !machine, libraryNames: {projectName}, - packagesPath: buildInfo.packagesPath + packagesPath: buildInfo.packagesPath, + resolver: await CoverageCollector.getResolver(buildInfo.packagesPath) ); } diff --git a/packages/flutter_tools/lib/src/test/coverage_collector.dart b/packages/flutter_tools/lib/src/test/coverage_collector.dart index 62ad1f609dd3..5319f4a700fd 100644 --- a/packages/flutter_tools/lib/src/test/coverage_collector.dart +++ b/packages/flutter_tools/lib/src/test/coverage_collector.dart @@ -19,7 +19,7 @@ import 'watcher.dart'; /// A class that collects code coverage data during test runs. class CoverageCollector extends TestWatcher { - CoverageCollector({this.libraryNames, this.verbose = true, required this.packagesPath}); + CoverageCollector({this.libraryNames, this.verbose = true, required this.packagesPath, this.resolver}); /// True when log messages should be emitted. final bool verbose; @@ -35,6 +35,19 @@ class CoverageCollector extends TestWatcher { /// will be accepted. Set? libraryNames; + final coverage.Resolver? resolver; + final Map>> _ignoredLinesInFilesCache = >>{}; + + static Future getResolver(String? packagesPath) async { + try { + return await coverage.Resolver.create(packagesPath: packagesPath); + } on FileSystemException { + // When given a bad packages path (as for instance done in some tests) + // just ignore it and return one without a packages path. + return coverage.Resolver.create(); + } + } + @override Future handleFinishedTest(TestDevice testDevice) async { _logMessage('Starting coverage collection'); @@ -104,7 +117,7 @@ class CoverageCollector extends TestWatcher { /// has been run to completion so that all coverage data has been recorded. /// /// The returned [Future] completes when the coverage is collected. - Future collectCoverage(TestDevice testDevice) async { + Future collectCoverage(TestDevice testDevice, {@visibleForTesting Future Function(Uri?)? connector}) async { assert(testDevice != null); Map? data; @@ -119,7 +132,7 @@ class CoverageCollector extends TestWatcher { final Future collectionComplete = testDevice.observatoryUri .then((Uri? observatoryUri) { _logMessage('collecting coverage data from $testDevice at $observatoryUri...'); - return collect(observatoryUri, libraryNames) + return collect(observatoryUri, libraryNames, connector: connector ?? _defaultConnect) .then((Map result) { if (result == null) { throw Exception('Failed to collect coverage.'); @@ -133,11 +146,12 @@ class CoverageCollector extends TestWatcher { assert(data != null); _logMessage('Merging coverage data...'); - _addHitmap(await coverage.HitMap.parseJson( - data!['coverage'] as List>, - packagePath: packageDirectory, - checkIgnoredLines: true, - )); + final Map hitmap = coverage.HitMap.parseJsonSync( + data!['coverage'] as List>, + checkIgnoredLines: true, + resolver: resolver ?? await CoverageCollector.getResolver(packageDirectory), + ignoredLinesInFilesCache: _ignoredLinesInFilesCache); + _addHitmap(hitmap); _logMessage('Done merging coverage data into global coverage map.'); } @@ -154,13 +168,13 @@ class CoverageCollector extends TestWatcher { return null; } if (formatter == null) { - resolver ??= await coverage.Resolver.create(packagesPath: packagesPath); + final coverage.Resolver usedResolver = resolver ?? this.resolver ?? await CoverageCollector.getResolver(packagesPath); final String packagePath = globals.fs.currentDirectory.path; final List reportOn = coverageDirectory == null ? [globals.fs.path.join(packagePath, 'lib')] : [coverageDirectory.path]; formatter = (Map hitmap) => hitmap - .formatLcov(resolver!, reportOn: reportOn, basePath: packagePath); + .formatLcov(usedResolver, reportOn: reportOn, basePath: packagePath); } final String result = formatter(_globalHitmap!); _globalHitmap = null; 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 0e64618778f5..e63db2bf4941 100644 --- a/packages/flutter_tools/test/general.shard/coverage_collector_test.dart +++ b/packages/flutter_tools/test/general.shard/coverage_collector_test.dart @@ -2,7 +2,13 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:convert' show jsonEncode; +import 'dart:io' show Directory, File; + +import 'package:coverage/src/hitmap.dart'; import 'package:flutter_tools/src/test/coverage_collector.dart'; +import 'package:flutter_tools/src/test/test_device.dart' show TestDevice; +import 'package:stream_channel/stream_channel.dart' show StreamChannel; import 'package:vm_service/vm_service.dart'; import '../src/common.dart'; @@ -138,94 +144,7 @@ void main() { }); testWithoutContext('Coverage collector with null libraryNames accepts all libraries', () async { - final FakeVmServiceHost fakeVmServiceHost = FakeVmServiceHost( - requests: [ - FakeVmServiceRequest( - method: 'getVersion', - jsonResponse: Version(major: 3, minor: 51).toJson(), - ), - FakeVmServiceRequest( - method: 'getVM', - jsonResponse: (VM.parse({})! - ..isolates = [ - IsolateRef.parse({ - 'id': '1', - })!, - ] - ).toJson(), - ), - FakeVmServiceRequest( - method: 'getScripts', - args: { - 'isolateId': '1', - }, - jsonResponse: ScriptList(scripts: [ - ScriptRef(uri: 'package:foo/foo.dart', id: '1'), - ScriptRef(uri: 'package:bar/bar.dart', id: '2'), - ]).toJson(), - ), - FakeVmServiceRequest( - method: 'getSourceReport', - args: { - 'isolateId': '1', - 'reports': ['Coverage'], - 'scriptId': '1', - 'forceCompile': true, - 'reportLines': true, - }, - 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(), - ), - FakeVmServiceRequest( - method: 'getSourceReport', - args: { - 'isolateId': '1', - 'reports': ['Coverage'], - 'scriptId': '2', - 'forceCompile': true, - 'reportLines': true, - }, - jsonResponse: SourceReport( - ranges: [ - SourceReportRange( - scriptIndex: 0, - startPos: 0, - endPos: 0, - compiled: true, - coverage: SourceReportCoverage( - hits: [47, 21], - misses: [32, 86], - ), - ), - ], - scripts: [ - ScriptRef( - uri: 'package:bar/bar.dart', - id: '2', - ), - ], - ).toJson(), - ), - ], - ); + final FakeVmServiceHost fakeVmServiceHost = createFakeVmServiceHostWithFooAndBar(); final Map result = await collect( null, @@ -417,4 +336,183 @@ void main() { }); expect(fakeVmServiceHost.hasRemainingExpectations, false); }); + + testWithoutContext('Coverage collector caches read files', () async { + Directory? tempDir; + try { + tempDir = Directory.systemTemp.createTempSync('flutter_coverage_collector_test.'); + final File file = File('${tempDir.path}/packages.json'); + file.writeAsStringSync(jsonEncode({ + 'configVersion': 2, + 'packages': >[ + { + 'name': 'foo', + 'rootUri': 'foo', + }, + { + 'name': 'bar', + 'rootUri': 'bar', + }, + ], + })); + final Directory fooDir = Directory('${tempDir.path}/foo/'); + fooDir.createSync(); + final File fooFile = File('${fooDir.path}/foo.dart'); + fooFile.writeAsStringSync('hit\nnohit but ignored // coverage:ignore-line\nhit\n'); + + final String packagesPath = file.path; + final CoverageCollector collector = CoverageCollector( + libraryNames: {'foo', 'bar'}, + verbose: false, + packagesPath: packagesPath, + resolver: await CoverageCollector.getResolver(packagesPath) + ); + await collector.collectCoverage(TestTestDevice(), connector: (Uri? uri) async { + return createFakeVmServiceHostWithFooAndBar().vmService; + }); + + Future getHitMapAndVerify() async { + final Map gottenHitmap = {}; + await collector.finalizeCoverage(formatter: (Map hitmap) { + gottenHitmap.addAll(hitmap); + return ''; + }); + expect(gottenHitmap.keys.toList()..sort(), ['package:bar/bar.dart', 'package:foo/foo.dart']); + expect(gottenHitmap['package:foo/foo.dart']!.lineHits, {1: 1, /* 2: 0, is ignored in file */ 3: 1}); + expect(gottenHitmap['package:bar/bar.dart']!.lineHits, {21: 1, 32: 0, 47: 1, 86: 0}); + } + + Future verifyHitmapEmpty() async { + final Map gottenHitmap = {}; + await collector.finalizeCoverage(formatter: (Map hitmap) { + gottenHitmap.addAll(hitmap); + return ''; + }); + expect(gottenHitmap.isEmpty, isTrue); + } + + // Get hit map the first time. + await getHitMapAndVerify(); + + // Getting the hitmap clears it so we now doesn't get any data. + await verifyHitmapEmpty(); + + // Collecting again gets us the same data even though the foo file has been deleted. + // This means that the fact that line 2 was ignored has been cached. + fooFile.deleteSync(); + await collector.collectCoverage(TestTestDevice(), connector: (Uri? uri) async { + return createFakeVmServiceHostWithFooAndBar().vmService; + }); + await getHitMapAndVerify(); + } finally { + tempDir?.deleteSync(recursive: true); + } + }); +} + +FakeVmServiceHost createFakeVmServiceHostWithFooAndBar() { + return FakeVmServiceHost( + requests: [ + FakeVmServiceRequest( + method: 'getVersion', + jsonResponse: Version(major: 3, minor: 51).toJson(), + ), + FakeVmServiceRequest( + method: 'getVM', + jsonResponse: (VM.parse({})! + ..isolates = [ + IsolateRef.parse({ + 'id': '1', + })!, + ] + ).toJson(), + ), + FakeVmServiceRequest( + method: 'getScripts', + args: { + 'isolateId': '1', + }, + jsonResponse: ScriptList(scripts: [ + ScriptRef(uri: 'package:foo/foo.dart', id: '1'), + ScriptRef(uri: 'package:bar/bar.dart', id: '2'), + ]).toJson(), + ), + FakeVmServiceRequest( + method: 'getSourceReport', + args: { + 'isolateId': '1', + 'reports': ['Coverage'], + 'scriptId': '1', + 'forceCompile': true, + 'reportLines': true, + }, + 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(), + ), + FakeVmServiceRequest( + method: 'getSourceReport', + args: { + 'isolateId': '1', + 'reports': ['Coverage'], + 'scriptId': '2', + 'forceCompile': true, + 'reportLines': true, + }, + jsonResponse: SourceReport( + ranges: [ + SourceReportRange( + scriptIndex: 0, + startPos: 0, + endPos: 0, + compiled: true, + coverage: SourceReportCoverage( + hits: [47, 21], + misses: [32, 86], + ), + ), + ], + scripts: [ + ScriptRef( + uri: 'package:bar/bar.dart', + id: '2', + ), + ], + ).toJson(), + ), + ], + ); +} + +class TestTestDevice extends TestDevice { + @override + Future get finished => Future.delayed(const Duration(seconds: 1)); + + @override + Future kill() => Future.value(); + + @override + Future get observatoryUri => Future.value(); + + @override + Future> start(String entrypointPath) { + throw UnimplementedError(); + } }