Stop putting suite loads into active test

Towards #1311

Removes a UX concern from `Engine` which no longer needs to worry about
how a reporter might display information about load suites. Makes the
interaction between the engine and reporters less subtle.

Use the term "suite load" over "load test" to start pushing terminology
away from conflating tests and loading.

Author: natebosch

pkgs/test_core/lib/src/runner/engine.dart

@@ -36,15 +36,10 @@ import 'util/iterable_set.dart';
 ///
 /// The engine has some special logic for [LoadSuite]s and the tests they
 /// contain, referred to as "load tests". Load tests exist to provide visibility
-/// into the process of loading test files, but as long as that process is
-/// proceeding normally users usually don't care about it, so the engine only
-/// surfaces running load tests (that is, includes them in [liveTests] and other
-/// collections) under specific circumstances.
-///
-/// If only load tests are running, exactly one load test will be in [active]
-/// and [liveTests]. If this test passes, it will be removed from both [active]
-/// and [liveTests] and *will not* be added to [passed]. If at any point a load
-/// test fails, it will be added to [failed] and [liveTests].
+/// into the process of loading test files. As long as that process is
+/// proceeding normally users usually don't care about it, so the engine does
+/// not include them in [liveTests] and other collections.
+/// If a load test fails, it will be added to [failed] and [liveTests].
 ///
 /// The test suite loaded by a load suite will be automatically be run by the
 /// engine; it doesn't need to be added to [suiteSink] manually.
@@ -174,22 +169,22 @@ class Engine {
   Set<LiveTest> get failed => _failedGroup.set;
   final _failedGroup = UnionSetController<LiveTest>(disjoint: true);
 
-  /// The tests that are still running, in the order they begain running.
+  /// The tests that are still running, in the order they began running.
   List<LiveTest> get active => UnmodifiableListView(_active);
   final _active = QueueList<LiveTest>();
 
+  /// The tests from [LoadSuite]s that are still running, in the order they
+  /// began running.
+  List<LiveTest> get activeSuiteLoads =>
+      UnmodifiableListView(_activeSuiteLoads);
+  final _activeSuiteLoads = <LiveTest>{};
+
   /// The set of tests that have been marked for restarting.
   ///
   /// This is always a subset of [active]. Once a test in here has finished
   /// running, it's run again.
   final _restarted = <LiveTest>{};
 
-  /// The tests from [LoadSuite]s that are still running, in the order they
-  /// began running.
-  ///
-  /// This is separate from [active] because load tests aren't always surfaced.
-  final _activeLoadTests = <LiveTest>{};
-
   /// Whether this engine is idle—that is, not currently executing a test.
   bool get isIdle => _group.isIdle;
 
@@ -356,21 +351,11 @@ class Engine {
     await _onUnpaused;
     _active.add(liveTest);
 
-    // If there were no active non-load tests, the current active test would
-    // have been a load test. In that case, remove it, since now we have a
-    // non-load test to add.
-    if (_active.first.suite is LoadSuite) _active.removeFirst();
-
     var subscription = liveTest.onStateChange.listen(null);
     subscription
       ..onData((state) {
         if (state.status != Status.complete) return;
         _active.remove(liveTest);
-
-        // If we're out of non-load tests, surface a load test.
-        if (_active.isEmpty && _activeLoadTests.isNotEmpty) {
-          _active.add(_activeLoadTests.first);
-        }
       })
       ..onDone(() {
         _subscriptions.remove(subscription);
@@ -425,7 +410,7 @@ class Engine {
   ///
   /// Returns the same future as [LiveTest.close].
   Future restartTest(LiveTest liveTest) async {
-    if (_activeLoadTests.contains(liveTest)) {
+    if (_activeSuiteLoads.contains(liveTest)) {
       throw ArgumentError("Can't restart a load test.");
     }
 
@@ -447,24 +432,13 @@ class Engine {
     _addLiveSuite(controller.liveSuite);
 
     var liveTest = suite.test.load(suite);
-    _activeLoadTests.add(liveTest);
-
-    // Only surface the load test if there are no other tests currently running.
-    if (_active.isEmpty) _active.add(liveTest);
+    _activeSuiteLoads.add(liveTest);
 
     var subscription = liveTest.onStateChange.listen(null);
     subscription
       ..onData((state) {
         if (state.status != Status.complete) return;
-        _activeLoadTests.remove(liveTest);
-
-        // Only one load test will be active at any given time, and it will always
-        // be the only active test. Remove it and, if possible, surface another
-        // load test.
-        if (_active.isNotEmpty && _active.first.suite == suite) {
-          _active.remove(liveTest);
-          if (_activeLoadTests.isNotEmpty) _active.add(_activeLoadTests.last);
-        }
+        _activeSuiteLoads.remove(liveTest);
       })
       ..onDone(() {
         _subscriptions.remove(subscription);
@@ -547,7 +521,7 @@ class Engine {
 
     // Close the running tests first so that we're sure to wait for them to
     // finish before we close their suites and cause them to become unloaded.
-    var allLiveTests = liveTests.toSet()..addAll(_activeLoadTests);
+    var allLiveTests = liveTests.toSet()..addAll(_activeSuiteLoads);
     var futures = allLiveTests.map((liveTest) => liveTest.close()).toList();
 
     // Closing the run pool will close the test suites as soon as their tests

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

@@ -186,10 +186,12 @@ class CompactReporter implements Reporter {
           .listen((_) => _progressLine(_lastProgressMessage ?? '')));
     }
 
-    // If this is the first test to start, print a progress line so the user
-    // knows what's running. It's possible that the active test may not be
-    // [liveTest] because the engine doesn't always surface load tests.
-    if (_engine.active.length == 1 && _engine.active.first == liveTest) {
+    // If this is the first test or suite load to start, print a progress line
+    // so the user knows what's running.
+    if ((_engine.active.length == 1 && _engine.active.first == liveTest) ||
+        (_engine.liveTests.isEmpty &&
+            _engine.activeSuiteLoads.length == 1 &&
+            _engine.activeSuiteLoads.first == liveTest)) {
       _progressLine(_description(liveTest));
     }
 

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

@@ -164,8 +164,9 @@ class ExpandedReporter implements Reporter {
       // emit information about them unless they fail.
       _subscriptions.add(liveTest.onStateChange
           .listen((state) => _onStateChange(liveTest, state)));
-    } else if (_engine.active.length == 1 &&
-        _engine.active.first == liveTest &&
+    } else if (_engine.active.isEmpty &&
+        _engine.activeSuiteLoads.length == 1 &&
+        _engine.activeSuiteLoads.first == liveTest &&
         liveTest.test.name.startsWith('compiling ')) {
       // Print a progress line for load tests that come from compiling JS, since
       // that takes a long time.