[coverage] Partial workspace support (#2095)

Author: liamappelbe

.github/workflows/coverage.yaml

@@ -56,12 +56,7 @@ jobs:
       fail-fast: false
       matrix:
         os: [ubuntu-latest, macos-latest, windows-latest]
-        sdk: [3.4, dev]
-        exclude:
-          # VM service times out on windows before Dart 3.5
-          # https://github.com/dart-lang/coverage/issues/490
-          - os: windows-latest
-            sdk: 3.4
+        sdk: [3.6, dev]
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
       - uses: dart-lang/setup-dart@e51d8e571e22473a2ddebf0ef8a2123f0ab2c02c
@@ -89,7 +84,7 @@ jobs:
         name: Install dependencies
         run: dart pub get
       - name: Collect and report coverage
-        run: dart run bin/test_with_coverage.dart --port=9292
+        run: dart run bin/test_with_coverage.dart
       - name: Upload coverage
         uses: coverallsapp/github-action@648a8eb78e6d50909eff900e4ec85cab4524a45b
         with:

pkgs/coverage/CHANGELOG.md

@@ -1,3 +1,11 @@
+## 1.14.0
+
+- Require Dart ^3.6.0
+- Partial support for workspace packages in `test_wth_coverage`.
+- Deprecate `test_wth_coverage`'s `--package-name` flag, because it doesn't make
+  sense for workspaces.
+- Change the default `--port` to 0, allowing the VM to choose a free port.
+
 ## 1.13.1
 
 - Fix a bug where the VM service can be shut down while some coverage

pkgs/coverage/README.md

@@ -43,6 +43,30 @@ dart pub global run coverage:format_coverage --packages=.dart_tool/package_confi
 For more complicated use cases, where you want to control each of these stages,
 see the sections below.
 
+#### Workspaces
+
+package:coverage has partial support for
+[workspaces](https://dart.dev/tools/pub/workspaces). You can run
+`test_with_coverage` from the root of the workspace to collect coverage for all
+the tests in all the subpackages, but you must specify the test directories to
+run.
+
+For example, in a workspace with subpackages `pkgs/foo` and `pkgs/bar`, you
+could run the following command from the root directory of the workspace:
+
+```
+dart run coverage:test_with_coverage -- pkgs/foo/test pkgs/bar/test
+```
+
+This would output coverage to ./coverage/ as normal. An important caveat is that
+the working directory of the tests will be the workspace's root directory. So
+this approach won't work if your tests assume that they are being run from the
+subpackage directory.
+
+[Full support](https://github.com/dart-lang/tools/issues/2083) for workspaces
+will likely be added in a future version. This will mean you won't need to
+explicitly specify the test directories: `dart run coverage:test_with_coverage`
+
 #### Collecting coverage from the VM
 
 ```

pkgs/coverage/bin/test_with_coverage.dart

@@ -7,10 +7,8 @@ import 'dart:io';
 
 import 'package:args/args.dart';
 import 'package:coverage/src/coverage_options.dart';
-import 'package:coverage/src/util.dart'
-    show StandardOutExtension, extractVMServiceUri;
+import 'package:coverage/src/util.dart';
 import 'package:meta/meta.dart';
-import 'package:package_config/package_config.dart';
 import 'package:path/path.dart' as path;
 
 import 'collect_coverage.dart' as collect_coverage;
@@ -19,37 +17,36 @@ import 'format_coverage.dart' as format_coverage;
 final _allProcesses = <Process>[];
 
 Future<void> _dartRun(List<String> args,
-    {void Function(String)? onStdout, String? workingDir}) async {
-  final process = await Process.start(
-    Platform.executable,
-    args,
-    workingDirectory: workingDir,
-  );
+    {required void Function(String) onStdout,
+    required void Function(String) onStderr}) async {
+  final process = await Process.start(Platform.executable, args);
   _allProcesses.add(process);
-  final broadStdout = process.stdout.asBroadcastStream();
-  broadStdout.listen(stdout.add);
-  if (onStdout != null) {
-    broadStdout.lines().listen(onStdout);
+
+  void listen(
+      Stream<List<int>> stream, IOSink sink, void Function(String) onLine) {
+    final broadStream = stream.asBroadcastStream();
+    broadStream.listen(sink.add);
+    broadStream.lines().listen(onLine);
   }
-  process.stderr.listen(stderr.add);
+
+  listen(process.stdout, stdout, onStdout);
+  listen(process.stderr, stderr, onStderr);
+
   final result = await process.exitCode;
   if (result != 0) {
     throw ProcessException(Platform.executable, args, '', result);
   }
 }
 
-Future<String?> _packageNameFromConfig(String packageDir) async {
-  final config = await findPackageConfig(Directory(packageDir));
-  return config?.packageOf(Uri.directory(packageDir))?.name;
+void _killSubprocessesAndExit(ProcessSignal signal) {
+  for (final process in _allProcesses) {
+    process.kill(signal);
+  }
+  exit(1);
 }
 
 void _watchExitSignal(ProcessSignal signal) {
-  signal.watch().listen((sig) {
-    for (final process in _allProcesses) {
-      process.kill(sig);
-    }
-    exit(1);
-  });
+  signal.watch().listen(_killSubprocessesAndExit);
 }
 
 ArgParser _createArgParser(CoverageOptions defaultOptions) => ArgParser()
@@ -61,10 +58,10 @@ ArgParser _createArgParser(CoverageOptions defaultOptions) => ArgParser()
   ..addOption(
     'package-name',
     help: 'Name of the package to test. '
-        'Deduced from --package if not provided.',
-    defaultsTo: defaultOptions.packageName,
+        'Deduced from --package if not provided. '
+        'DEPRECATED: use --scope-output',
   )
-  ..addOption('port', help: 'VM service port.', defaultsTo: '8181')
+  ..addOption('port', help: 'VM service port. Defaults to using any free port.')
   ..addOption(
     'out',
     defaultsTo: defaultOptions.outputDirectory,
@@ -93,13 +90,13 @@ ArgParser _createArgParser(CoverageOptions defaultOptions) => ArgParser()
       defaultsTo: defaultOptions.scopeOutput,
       help: 'restrict coverage results so that only scripts that start with '
           'the provided package path are considered. Defaults to the name of '
-          'the package under test.')
+          'the current package (including all subpackages, if this is a '
+          'workspace).')
   ..addFlag('help', abbr: 'h', negatable: false, help: 'Show this help.');
 
 class Flags {
   Flags(
     this.packageDir,
-    this.packageName,
     this.outDir,
     this.port,
     this.testScript,
@@ -111,7 +108,6 @@ class Flags {
   });
 
   final String packageDir;
-  final String packageName;
   final String outDir;
   final String port;
   final String testScript;
@@ -157,25 +153,23 @@ ${parser.usage}
     fail('--package is not a valid directory.');
   }
 
-  final packageName = (args['package-name'] as String?) ??
-      await _packageNameFromConfig(packageDir);
-  if (packageName == null) {
+  final pubspecPath = getPubspecPath(packageDir);
+  if (!File(pubspecPath).existsSync()) {
     fail(
-      "Couldn't figure out package name from --package. Make sure this is a "
-      'package directory, or try passing --package-name explicitly.',
+      "Couldn't find $pubspecPath. Make sure this command is run in a "
+      'package directory, or pass --package to explicitly set the directory.',
     );
   }
 
   return Flags(
     packageDir,
-    packageName,
-    (args['out'] as String?) ?? path.join(packageDir, 'coverage'),
-    args['port'] as String,
-    args['test'] as String,
-    args['function-coverage'] as bool,
-    args['branch-coverage'] as bool,
-    args['scope-output'] as List<String>,
-    args['fail-under'] as String?,
+    args.option('out') ?? path.join(packageDir, 'coverage'),
+    args.option('port') ?? '0',
+    args.option('test')!,
+    args.flag('function-coverage'),
+    args.flag('branch-coverage'),
+    args.multiOption('scope-output'),
+    args.option('fail-under'),
     rest: args.rest,
   );
 }
@@ -215,11 +209,19 @@ Future<void> main(List<String> arguments) async {
         }
       }
     },
+    onStderr: (line) {
+      if (!serviceUriCompleter.isCompleted) {
+        if (line.contains('Could not start the VM service')) {
+          _killSubprocessesAndExit(ProcessSignal.sigkill);
+        }
+      }
+    },
   );
   final serviceUri = await serviceUriCompleter.future;
 
-  final scopes =
-      flags.scopeOutput.isEmpty ? [flags.packageName] : flags.scopeOutput;
+  final scopes = flags.scopeOutput.isEmpty
+      ? getAllWorkspaceNames(flags.packageDir)
+      : flags.scopeOutput;
   await collect_coverage.main([
     '--wait-paused',
     '--resume-isolates',

pkgs/coverage/dart_test.yaml

@@ -0,0 +1,5 @@
+tags:
+  # Tests that start subprocesses, so are slower and can be a bit flaky.
+  integration:
+    timeout: 2x
+    retry: 3

pkgs/coverage/lib/src/collect.dart

@@ -170,6 +170,7 @@ Future<Map<String, dynamic>> _getAllCoverage(
         isolateReport,
         includeDart,
         functionCoverage,
+        branchCoverage,
         coverableLineCache,
         scopedOutput);
     allCoverage.addAll(coverage);
@@ -244,6 +245,7 @@ Future<List<Map<String, dynamic>>> _processSourceReport(
     SourceReport report,
     bool includeDart,
     bool functionCoverage,
+    bool branchCoverage,
     Map<String, Set<int>>? coverableLineCache,
     Set<String> scopedOutput) async {
   final hitMaps = <Uri, HitMap>{};
@@ -262,7 +264,10 @@ Future<List<Map<String, dynamic>>> _processSourceReport(
     return scripts[scriptRef];
   }
 
-  HitMap getHitMap(Uri scriptUri) => hitMaps.putIfAbsent(scriptUri, HitMap.new);
+  HitMap getHitMap(Uri scriptUri) => hitMaps.putIfAbsent(
+      scriptUri,
+      () => HitMap.empty(
+          functionCoverage: functionCoverage, branchCoverage: branchCoverage));
 
   Future<void> processFunction(FuncRef funcRef) async {
     final func = await service.getObject(isolateRef.id!, funcRef.id!) as Func;
@@ -290,8 +295,7 @@ Future<List<Map<String, dynamic>>> _processSourceReport(
       return;
     }
     final hits = getHitMap(Uri.parse(script.uri!));
-    hits.funcHits ??= <int, int>{};
-    (hits.funcNames ??= <int, String>{})[line] = funcName;
+    hits.funcNames![line] = funcName;
   }
 
   for (var range in report.ranges!) {
@@ -385,13 +389,12 @@ Future<List<Map<String, dynamic>>> _processSourceReport(
       hits.funcHits?.putIfAbsent(line, () => 0);
     });
 
-    final branchCoverage = range.branchCoverage;
-    if (branchCoverage != null) {
-      hits.branchHits ??= <int, int>{};
-      forEachLine(branchCoverage.hits, (line) {
+    final branches = range.branchCoverage;
+    if (branchCoverage && branches != null) {
+      forEachLine(branches.hits, (line) {
         hits.branchHits!.increment(line);
       });
-      forEachLine(branchCoverage.misses, (line) {
+      forEachLine(branches.misses, (line) {
         hits.branchHits!.putIfAbsent(line, () => 0);
       });
     }

pkgs/coverage/lib/src/coverage_options.dart

@@ -9,7 +9,6 @@ class CoverageOptions {
     required this.functionCoverage,
     required this.branchCoverage,
     required this.packageDirectory,
-    this.packageName,
     required this.testScript,
   });
 
@@ -40,8 +39,6 @@ class CoverageOptions {
       branchCoverage: options.optionalBool('branch_coverage') ??
           defaultOptions.branchCoverage,
       packageDirectory: packageDirectory,
-      packageName:
-          options.optionalString('package_name') ?? defaultOptions.packageName,
       testScript:
           options.optionalString('test_script') ?? defaultOptions.testScript,
     );
@@ -52,7 +49,6 @@ class CoverageOptions {
   final bool functionCoverage;
   final bool branchCoverage;
   final String packageDirectory;
-  final String? packageName;
   final String testScript;
 }
 
@@ -119,7 +115,6 @@ class CoverageOptionsProvider {
     functionCoverage: false,
     branchCoverage: false,
     packageDirectory: '.',
-    packageName: null,
     testScript: 'test',
   );
 }

pkgs/coverage/lib/src/formatter.dart

@@ -151,6 +151,20 @@ extension FileHitMapsFormatter on Map<String, HitMap> {
     );
     final buf = StringBuffer();
     for (final entry in entries) {
+      final source = resolver.resolve(entry.key);
+      if (source == null) {
+        continue;
+      }
+
+      if (!pathFilter(source)) {
+        continue;
+      }
+
+      final lines = await loader.load(source);
+      if (lines == null) {
+        continue;
+      }
+
       final v = entry.value;
       if (reportFuncs && v.funcHits == null) {
         throw StateError(
@@ -165,24 +179,12 @@ extension FileHitMapsFormatter on Map<String, HitMap> {
             'missing branch coverage information. Did you run '
             'collect_coverage with the --branch-coverage flag?');
       }
+
       final hits = reportFuncs
           ? v.funcHits!
           : reportBranches
               ? v.branchHits!
               : v.lineHits;
-      final source = resolver.resolve(entry.key);
-      if (source == null) {
-        continue;
-      }
-
-      if (!pathFilter(source)) {
-        continue;
-      }
-
-      final lines = await loader.load(source);
-      if (lines == null) {
-        continue;
-      }
       buf.writeln(source);
       for (var line = 1; line <= lines.length; line++) {
         var prefix = _prefix;

pkgs/coverage/lib/src/hitmap.dart

@@ -18,6 +18,15 @@ class HitMap {
     this.branchHits,
   ]) : lineHits = lineHits ?? {};
 
+  /// Constructs an empty hitmap, optionally with function and branch coverage
+  /// tables.
+  HitMap.empty({bool functionCoverage = false, bool branchCoverage = false})
+      : this(
+            null,
+            functionCoverage ? <int, int>{} : null,
+            functionCoverage ? <int, String>{} : null,
+            branchCoverage ? <int, int>{} : null);
+
   /// Map from line to hit count for that line.
   final Map<int, int> lineHits;