Improve coverage speed by using new caching option for package:coverage (flutter#107395)

jensjoha · web-flow · commit 803ef6a456c3 · 2022-07-13T11:07:59.000+02:00
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).
diff --git a/packages/flutter_tools/bin/fuchsia_tester.dart b/packages/flutter_tools/bin/fuchsia_tester.dart
@@ -116,9 +116,11 @@ Future<void> run(List<String> args) async {
       // setting libraryNames to null.
       final Set<String> libraryNames = coverageDirectory != null ? null :
           <String>{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
@@ -374,7 +374,8 @@ class TestCommand extends FlutterCommand with DeviceBasedDevelopmentArtifacts {
       collector = CoverageCollector(
         verbose: !machine,
         libraryNames: <String>{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
@@ -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<String>? libraryNames;
 
+  final coverage.Resolver? resolver;
+  final Map<String, List<List<int>>> _ignoredLinesInFilesCache = <String, List<List<int>>>{};
+
+  static Future<coverage.Resolver> 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<void> 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<void> collectCoverage(TestDevice testDevice) async {
+  Future<void> collectCoverage(TestDevice testDevice, {@visibleForTesting Future<FlutterVmService> Function(Uri?)? connector}) async {
     assert(testDevice != null);
 
     Map<String, dynamic>? data;
@@ -119,7 +132,7 @@ class CoverageCollector extends TestWatcher {
     final Future<void> 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<void>((Map<String, dynamic> 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<Map<String, dynamic>>,
-      packagePath: packageDirectory,
-      checkIgnoredLines: true,
-    ));
+    final Map<String, coverage.HitMap> hitmap = coverage.HitMap.parseJsonSync(
+        data!['coverage'] as List<Map<String, dynamic>>,
+        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<String> reportOn = coverageDirectory == null
           ? <String>[globals.fs.path.join(packagePath, 'lib')]
           : <String>[coverageDirectory.path];
       formatter = (Map<String, coverage.HitMap> 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
@@ -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: <VmServiceExpectation>[
-        FakeVmServiceRequest(
-          method: 'getVersion',
-          jsonResponse: Version(major: 3, minor: 51).toJson(),
-        ),
-        FakeVmServiceRequest(
-          method: 'getVM',
-          jsonResponse: (VM.parse(<String, Object>{})!
-            ..isolates = <IsolateRef>[
-              IsolateRef.parse(<String, Object>{
-                'id': '1',
-              })!,
-            ]
-          ).toJson(),
-        ),
-        FakeVmServiceRequest(
-          method: 'getScripts',
-          args: <String, Object>{
-            'isolateId': '1',
-          },
-          jsonResponse: ScriptList(scripts: <ScriptRef>[
-            ScriptRef(uri: 'package:foo/foo.dart', id: '1'),
-            ScriptRef(uri: 'package:bar/bar.dart', id: '2'),
-          ]).toJson(),
-        ),
-        FakeVmServiceRequest(
-          method: 'getSourceReport',
-          args: <String, Object>{
-            'isolateId': '1',
-            'reports': <Object>['Coverage'],
-            'scriptId': '1',
-            'forceCompile': true,
-            'reportLines': true,
-          },
-          jsonResponse: SourceReport(
-            ranges: <SourceReportRange>[
-              SourceReportRange(
-                scriptIndex: 0,
-                startPos: 0,
-                endPos: 0,
-                compiled: true,
-                coverage: SourceReportCoverage(
-                  hits: <int>[1, 3],
-                  misses: <int>[2],
-                ),
-              ),
-            ],
-            scripts: <ScriptRef>[
-              ScriptRef(
-                uri: 'package:foo/foo.dart',
-                id: '1',
-              ),
-            ],
-          ).toJson(),
-        ),
-        FakeVmServiceRequest(
-          method: 'getSourceReport',
-          args: <String, Object>{
-            'isolateId': '1',
-            'reports': <Object>['Coverage'],
-            'scriptId': '2',
-            'forceCompile': true,
-            'reportLines': true,
-          },
-          jsonResponse: SourceReport(
-            ranges: <SourceReportRange>[
-              SourceReportRange(
-                scriptIndex: 0,
-                startPos: 0,
-                endPos: 0,
-                compiled: true,
-                coverage: SourceReportCoverage(
-                  hits: <int>[47, 21],
-                  misses: <int>[32, 86],
-                ),
-              ),
-            ],
-            scripts: <ScriptRef>[
-              ScriptRef(
-                uri: 'package:bar/bar.dart',
-                id: '2',
-              ),
-            ],
-          ).toJson(),
-        ),
-      ],
-    );
+    final FakeVmServiceHost fakeVmServiceHost = createFakeVmServiceHostWithFooAndBar();
 
     final Map<String, Object?> 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(<String, dynamic>{
+        'configVersion': 2,
+        'packages': <Map<String, String>>[
+          <String, String>{
+            'name': 'foo',
+            'rootUri': 'foo',
+          },
+          <String, String>{
+            '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: <String>{'foo', 'bar'},
+          verbose: false,
+          packagesPath: packagesPath,
+          resolver: await CoverageCollector.getResolver(packagesPath)
+        );
+      await collector.collectCoverage(TestTestDevice(), connector: (Uri? uri) async {
+        return createFakeVmServiceHostWithFooAndBar().vmService;
+      });
+
+      Future<void> getHitMapAndVerify() async {
+        final Map<String, HitMap> gottenHitmap = <String, HitMap>{};
+        await collector.finalizeCoverage(formatter: (Map<String, HitMap> hitmap) {
+          gottenHitmap.addAll(hitmap);
+          return '';
+        });
+        expect(gottenHitmap.keys.toList()..sort(), <String>['package:bar/bar.dart', 'package:foo/foo.dart']);
+        expect(gottenHitmap['package:foo/foo.dart']!.lineHits, <int, int>{1: 1, /* 2: 0, is ignored in file */ 3: 1});
+        expect(gottenHitmap['package:bar/bar.dart']!.lineHits, <int, int>{21: 1, 32: 0, 47: 1, 86: 0});
+      }
+
+      Future<void> verifyHitmapEmpty() async {
+        final Map<String, HitMap> gottenHitmap = <String, HitMap>{};
+        await collector.finalizeCoverage(formatter: (Map<String, HitMap> 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: <VmServiceExpectation>[
+      FakeVmServiceRequest(
+        method: 'getVersion',
+        jsonResponse: Version(major: 3, minor: 51).toJson(),
+      ),
+      FakeVmServiceRequest(
+        method: 'getVM',
+        jsonResponse: (VM.parse(<String, Object>{})!
+          ..isolates = <IsolateRef>[
+            IsolateRef.parse(<String, Object>{
+              'id': '1',
+            })!,
+          ]
+        ).toJson(),
+      ),
+      FakeVmServiceRequest(
+        method: 'getScripts',
+        args: <String, Object>{
+          'isolateId': '1',
+        },
+        jsonResponse: ScriptList(scripts: <ScriptRef>[
+          ScriptRef(uri: 'package:foo/foo.dart', id: '1'),
+          ScriptRef(uri: 'package:bar/bar.dart', id: '2'),
+        ]).toJson(),
+      ),
+      FakeVmServiceRequest(
+        method: 'getSourceReport',
+        args: <String, Object>{
+          'isolateId': '1',
+          'reports': <Object>['Coverage'],
+          'scriptId': '1',
+          'forceCompile': true,
+          'reportLines': true,
+        },
+        jsonResponse: SourceReport(
+          ranges: <SourceReportRange>[
+            SourceReportRange(
+              scriptIndex: 0,
+              startPos: 0,
+              endPos: 0,
+              compiled: true,
+              coverage: SourceReportCoverage(
+                hits: <int>[1, 3],
+                misses: <int>[2],
+              ),
+            ),
+          ],
+          scripts: <ScriptRef>[
+            ScriptRef(
+              uri: 'package:foo/foo.dart',
+              id: '1',
+            ),
+          ],
+        ).toJson(),
+      ),
+      FakeVmServiceRequest(
+        method: 'getSourceReport',
+        args: <String, Object>{
+          'isolateId': '1',
+          'reports': <Object>['Coverage'],
+          'scriptId': '2',
+          'forceCompile': true,
+          'reportLines': true,
+        },
+        jsonResponse: SourceReport(
+          ranges: <SourceReportRange>[
+            SourceReportRange(
+              scriptIndex: 0,
+              startPos: 0,
+              endPos: 0,
+              compiled: true,
+              coverage: SourceReportCoverage(
+                hits: <int>[47, 21],
+                misses: <int>[32, 86],
+              ),
+            ),
+          ],
+          scripts: <ScriptRef>[
+            ScriptRef(
+              uri: 'package:bar/bar.dart',
+              id: '2',
+            ),
+          ],
+        ).toJson(),
+      ),
+    ],
+  );
+}
+
+class TestTestDevice extends TestDevice {
+  @override
+  Future<void> get finished => Future<void>.delayed(const Duration(seconds: 1));
+
+  @override
+  Future<void> kill() => Future<void>.value();
+
+  @override
+  Future<Uri?> get observatoryUri => Future<Uri?>.value();
+
+  @override
+  Future<StreamChannel<String>> start(String entrypointPath) {
+    throw UnimplementedError();
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -374,7 +374,8 @@ class TestCommand extends FlutterCommand with DeviceBasedDevelopmentArtifacts {`
`374`	`374`	`collector = CoverageCollector(`
`375`	`375`	`verbose: !machine,`
`376`	`376`	`libraryNames: <String>{projectName},`
`377`		`- packagesPath: buildInfo.packagesPath`
	`377`	`+ packagesPath: buildInfo.packagesPath,`
	`378`	`+ resolver: await CoverageCollector.getResolver(buildInfo.packagesPath)`
`378`	`379`	`);`
`379`	`380`	`}`
`380`	`381`