Normalize Compact and Expanded reporters (#1325)

Working towards #1311

The `Compact` and `Expanded` reporters have a lot of code in common,
along with many superficial, and a few not-superficial differences. Some
of the impactful differences look like they may be bugs. Make it easier
to understand both reporters and their differences by reducing the
superficial changes.

The JSON reporter shares less code with the others, but aim for a
consistent field ordering there as well.

- Avoid using `Configuration.current` from the compact reporter. This
  was never added in the expanded reporter to avoid a transitive import
  to `dart:io`. The pattern of being configured at construction is
  already baked in to the `ReporterFactory` typedef as is a more clear
  pattern than reading from a magic zone scoped variable. Add required
  constructor args for the compact reporter which match those in the
  expanded reporter. Remove the TODO.
- Use the same pattern for setting the color escape code fields. In the
  future we will likely want to migrate to `package:io` for these.
- Shuffle some fields so they are ordered consistently in both
  reporters.
- Add a default for `_lastProgressPassed` to avoid making it nullable.
- Add `_printPath` and `_printPlatform` fields in the compact reporter
  rather than read them lazily through the zone scoped configuration.
- Make the arguments required for the constructors instead of adding
  defaults which are never used.
- Use 2 slashs for comments.

Author: natebosch

pkgs/test_core/lib/src/runner/configuration/reporters.dart

@@ -36,8 +36,13 @@ final _allReporters = <String, ReporterDetails>{
           printPath: config.paths.length > 1 ||
               Directory(config.paths.single).existsSync(),
           printPlatform: config.suiteDefaults.runtimes.length > 1)),
-  'compact': ReporterDetails('A single line, updated continuously.',
-      (_, engine, sink) => CompactReporter.watch(engine, sink)),
+  'compact': ReporterDetails(
+      'A single line, updated continuously.',
+      (config, engine, sink) => CompactReporter.watch(engine, sink,
+          color: config.color,
+          printPath: config.paths.length > 1 ||
+              Directory(config.paths.single).existsSync(),
+          printPlatform: config.suiteDefaults.runtimes.length > 1)),
   'json': ReporterDetails(
       'A machine-readable format (see https://bit.ly/2Z7J0OH).',
       (_, engine, sink) => JsonReporter.watch(engine, sink)),

pkgs/test_core/lib/src/runner/reporter/compact.dart

@@ -14,7 +14,6 @@ import 'package:test_api/src/utils.dart'; // ignore: implementation_imports
 import 'package:test_api/src/utils.dart' as utils;
 
 import '../../util/io.dart';
-import '../configuration.dart';
 import '../engine.dart';
 import '../load_exception.dart';
 import '../load_suite.dart';
@@ -23,40 +22,41 @@ import '../reporter.dart';
 /// A reporter that prints test results to the console in a single
 /// continuously-updating line.
 class CompactReporter implements Reporter {
-  final _config = Configuration.current;
+  /// Whether the reporter should emit terminal color escapes.
+  final bool _color;
 
   /// The terminal escape for green text, or the empty string if this is Windows
   /// or not outputting to a terminal.
-  String get _green => _config.color ? '\u001b[32m' : '';
+  final String _green;
 
   /// The terminal escape for red text, or the empty string if this is Windows
   /// or not outputting to a terminal.
-  String get _red => _config.color ? '\u001b[31m' : '';
+  final String _red;
 
   /// The terminal escape for yellow text, or the empty string if this is
   /// Windows or not outputting to a terminal.
-  String get _yellow => _config.color ? '\u001b[33m' : '';
+  final String _yellow;
 
   /// The terminal escape for gray text, or the empty string if this is
   /// Windows or not outputting to a terminal.
-  String get _gray => _config.color ? '\u001b[1;30m' : '';
+  final String _gray;
 
   /// The terminal escape for bold text, or the empty string if this is
   /// Windows or not outputting to a terminal.
-  String get _bold => _config.color ? '\u001b[1m' : '';
+  final String _bold;
 
   /// The terminal escape for removing test coloring, or the empty string if
   /// this is Windows or not outputting to a terminal.
-  String get _noColor => _config.color ? '\u001b[0m' : '';
-
-  /// Whether the path to each test's suite should be printed.
-  final bool _printPath = Configuration.current.paths.length > 1 ||
-      Directory(Configuration.current.paths.single).existsSync();
+  final String _noColor;
 
   /// The engine used to run the tests.
   final Engine _engine;
 
-  final StringSink _sink;
+  /// Whether the path to each test's suite should be printed.
+  final bool _printPath;
+
+  /// Whether the platform each test is running on should be printed.
+  final bool _printPlatform;
 
   /// A stopwatch that tracks the duration of the full run.
   final _stopwatch = Stopwatch();
@@ -69,9 +69,10 @@ class CompactReporter implements Reporter {
 
   /// The size of `_engine.passed` last time a progress notification was
   /// printed.
-  int? _lastProgressPassed;
+  int _lastProgressPassed = 0;
 
-  /// The size of `_engine.skipped` last time a progress notification was printed.
+  /// The size of `_engine.skipped` last time a progress notification was
+  /// printed.
   int? _lastProgressSkipped;
 
   /// The size of `_engine.failed` last time a progress notification was
@@ -101,16 +102,39 @@ class CompactReporter implements Reporter {
   /// The set of all subscriptions to various streams.
   final _subscriptions = <StreamSubscription>{};
 
+  final StringSink _sink;
+
   /// Watches the tests run by [engine] and prints their results to the
   /// terminal.
-  static CompactReporter watch(Engine engine, StringSink sink) =>
-      CompactReporter._(engine, sink);
-
-  CompactReporter._(this._engine, this._sink) {
+  ///
+  /// If [color] is `true`, this will use terminal colors; if it's `false`, it
+  /// won't. If [printPath] is `true`, this will print the path name as part of
+  /// the test description. Likewise, if [printPlatform] is `true`, this will
+  /// print the platform as part of the test description.
+  static CompactReporter watch(Engine engine, StringSink sink,
+          {required bool color,
+          required bool printPath,
+          required bool printPlatform}) =>
+      CompactReporter._(engine, sink,
+          color: color, printPath: printPath, printPlatform: printPlatform);
+
+  CompactReporter._(this._engine, this._sink,
+      {required bool color,
+      required bool printPath,
+      required bool printPlatform})
+      : _printPath = printPath,
+        _printPlatform = printPlatform,
+        _color = color,
+        _green = color ? '\u001b[32m' : '',
+        _red = color ? '\u001b[31m' : '',
+        _yellow = color ? '\u001b[33m' : '',
+        _gray = color ? '\u001b[1;30m' : '',
+        _bold = color ? '\u001b[1m' : '',
+        _noColor = color ? '\u001b[0m' : '' {
     _subscriptions.add(_engine.onTestStarted.listen(_onTestStarted));
 
-    /// Convert the future to a stream so that the subscription can be paused or
-    /// canceled.
+    // Convert the future to a stream so that the subscription can be paused or
+    // canceled.
     _subscriptions.add(_engine.success.asStream().listen(_onDone));
   }
 
@@ -158,7 +182,7 @@ class CompactReporter implements Reporter {
       _stopwatchStarted = true;
       _stopwatch.start();
 
-      /// Keep updating the time even when nothing else is happening.
+      // Keep updating the time even when nothing else is happening.
       _subscriptions.add(Stream.periodic(Duration(seconds: 1))
           .listen((_) => _progressLine(_lastProgressMessage!)));
     }
@@ -222,7 +246,7 @@ class CompactReporter implements Reporter {
     }
 
     // TODO - what type is this?
-    _sink.writeln(indent(error.toString(color: _config.color)));
+    _sink.writeln(indent(error.toString(color: _color)));
 
     // Only print stack traces for load errors that come from the user's code.
     if (error.innerError is! IOException &&
@@ -376,7 +400,7 @@ class CompactReporter implements Reporter {
       name = '${liveTest.suite.path}: $name';
     }
 
-    if (_config.suiteDefaults.runtimes.length > 1) {
+    if (_printPlatform) {
       name = '[${liveTest.suite.platform.runtime.name}] $name';
     }
 

pkgs/test_core/lib/src/runner/reporter/expanded.dart

@@ -86,8 +86,6 @@ class ExpandedReporter implements Reporter {
 
   final StringSink _sink;
 
-  // TODO(nweiz): Get configuration from [Configuration.current] once we have
-  // cross-platform imports.
   /// Watches the tests run by [engine] and prints their results to the
   /// terminal.
   ///
@@ -96,13 +94,16 @@ class ExpandedReporter implements Reporter {
   /// the test description. Likewise, if [printPlatform] is `true`, this will
   /// print the platform as part of the test description.
   static ExpandedReporter watch(Engine engine, StringSink sink,
-      {bool color = true, bool printPath = true, bool printPlatform = true}) {
-    return ExpandedReporter._(engine, sink,
-        color: color, printPath: printPath, printPlatform: printPlatform);
-  }
+          {required bool color,
+          required bool printPath,
+          required bool printPlatform}) =>
+      ExpandedReporter._(engine, sink,
+          color: color, printPath: printPath, printPlatform: printPlatform);
 
   ExpandedReporter._(this._engine, this._sink,
-      {bool color = true, bool printPath = true, bool printPlatform = true})
+      {required bool color,
+      required bool printPath,
+      required bool printPlatform})
       : _printPath = printPath,
         _printPlatform = printPlatform,
         _color = color,
@@ -114,8 +115,8 @@ class ExpandedReporter implements Reporter {
         _noColor = color ? '\u001b[0m' : '' {
     _subscriptions.add(_engine.onTestStarted.listen(_onTestStarted));
 
-    /// Convert the future to a stream so that the subscription can be paused or
-    /// canceled.
+    // Convert the future to a stream so that the subscription can be paused or
+    // canceled.
     _subscriptions.add(_engine.success.asStream().listen(_onDone));
   }
 
@@ -134,6 +135,7 @@ class ExpandedReporter implements Reporter {
   @override
   void resume() {
     if (!_paused) return;
+
     _stopwatch.start();
 
     for (var subscription in _subscriptions) {
@@ -206,7 +208,7 @@ class ExpandedReporter implements Reporter {
     }
 
     // TODO - what type is this?
-    _sink.writeln(indent((error as dynamic).toString(color: _color) as String));
+    _sink.writeln(indent(error.toString(color: _color)));
 
     // Only print stack traces for load errors that come from the user's code.
     if (error.innerError is! FormatException && error.innerError is! String) {

pkgs/test_core/lib/src/runner/reporter/json.dart

@@ -34,8 +34,6 @@ class JsonReporter implements Reporter {
   /// The engine used to run the tests.
   final Engine _engine;
 
-  final StringSink _sink;
-
   /// A stopwatch that tracks the duration of the full run.
   final _stopwatch = Stopwatch();
 
@@ -45,12 +43,6 @@ class JsonReporter implements Reporter {
   /// when the reporter is paused.
   var _stopwatchStarted = false;
 
-  /// Whether the reporter is paused.
-  var _paused = false;
-
-  /// The set of all subscriptions to various streams.
-  final _subscriptions = <StreamSubscription>{};
-
   /// An expando that associates unique IDs with [LiveTest]s.
   final _liveTestIDs = <LiveTest, int>{};
 
@@ -63,15 +55,23 @@ class JsonReporter implements Reporter {
   /// The next ID to associate with a [LiveTest].
   var _nextID = 0;
 
+  /// Whether the reporter is paused.
+  var _paused = false;
+
+  /// The set of all subscriptions to various streams.
+  final _subscriptions = <StreamSubscription>{};
+
+  final StringSink _sink;
+
   /// Watches the tests run by [engine] and prints their results as JSON.
   static JsonReporter watch(Engine engine, StringSink sink) =>
       JsonReporter._(engine, sink);
 
   JsonReporter._(this._engine, this._sink) : _config = Configuration.current {
     _subscriptions.add(_engine.onTestStarted.listen(_onTestStarted));
 
-    /// Convert the future to a stream so that the subscription can be paused or
-    /// canceled.
+    // Convert the future to a stream so that the subscription can be paused or
+    // canceled.
     _subscriptions.add(_engine.success.asStream().listen(_onDone));
 
     _subscriptions.add(_engine.onSuiteAdded.listen(null, onDone: () {
@@ -146,8 +146,8 @@ class JsonReporter implements Reporter {
           liveTest.suite.path!)
     });
 
-    /// Convert the future to a stream so that the subscription can be paused or
-    /// canceled.
+    // Convert the future to a stream so that the subscription can be paused or
+    // canceled.
     _subscriptions.add(
         liveTest.onComplete.asStream().listen((_) => _onComplete(liveTest)));