Simplify the enqueuing of tests from all test suites

tools/testing/dart/test_runner.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /**
  * Classes and methods for executing tests.
  *
  * This module includes:
  * - Managing parallel execution of tests, including timeout checks.
  * - Evaluating the output of each test as pass/fail/crash/timeout.
@@ skipping 1408 matching lines @@
  * and counters to determine when all of the tests in all of the test suites
  * have completed.
  *
  * Because multiple configurations may be run on each test suite, the
  * ProcessQueue contains a cache in which a test suite may record information
  * about its list of tests, and may retrieve that information when it is called
  * upon to enqueue its tests again.
  */
 class ProcessQueue {
   int _numProcesses = 0;
-  int _activeTestListers = 0;
   int _maxProcesses;
+  bool _allTestsWereEnqueued = false;
 
   /** The number of tests we allow to actually fail before we stop retrying. */
   int _MAX_FAILED_NO_RETRY = 4;
   bool _verbose;
   bool _listTests;
   Function _allDone;
-  EnqueueMoreWork _enqueueMoreWork;
   Queue<TestCase> _tests;
   ProgressIndicator _progress;
 
   // For dartc/selenium batch processing we keep a list of batch processes.
   Map<String, List<BatchRunnerProcess>> _batchProcesses;
 
   // Cache information about test cases per test suite. For multiple
   // configurations there is no need to repeatedly search the file
   // system, generate tests, and search test files for options.
   Map<String, List<TestInformation>> _testCache;
@@ skipping 13 matching lines @@
   /** True if we are in the process of starting the server. */
   bool _startingServer = false;
 
   /** True if we find that there is already a selenium jar running. */
   bool _seleniumAlreadyRunning = false;
 
   ProcessQueue(int this._maxProcesses,
                String progress,
                Date startTime,
                bool printTiming,
-               this._enqueueMoreWork,
+               testSuites,
                this._allDone,
                [bool verbose = false,
                 bool listTests = false])
       : _verbose = verbose,
         _listTests = listTests,
         _tests = new Queue<TestCase>(),
         _progress = new ProgressIndicator.fromName(progress,
                                                    startTime,
                                                    printTiming),
         _batchProcesses = new Map<String, List<BatchRunnerProcess>>(),
         _testCache = new Map<String, List<TestInformation>>() {
-    _checkDone();
+    _runTests(testSuites);
   }
 
   /**
-   * Registers a TestSuite so that all of its tests will be run.
-   */
-  void addTestSuite(TestSuite testSuite) {
-    _activeTestListers++;
-    testSuite.forEachTest(_runTest, _testCache, _testListerDone);
-  }
-
-  void _testListerDone() {
-    _activeTestListers--;
-    _checkDone();
-  }
-
-  /**
    * Perform any cleanup needed once all tests in a TestSuite have completed
    * and notify our progress indicator that we are done.
    */
   void _cleanupAndMarkDone() {
     _allDone();
     if (browserUsed != '' && _seleniumServer != null) {
       _seleniumServer.kill();
     } else {
       _progress.allDone();
     }
   }
 
-  void _checkDone() {
-    // When there are no more active test listers ask for more work
-    // from process queue users.
-    if (_activeTestListers == 0) {
-     _enqueueMoreWork(this);
-    }
-    // If there is still no work, we are done.
-    if (_activeTestListers == 0) {
-      _progress.allTestsKnown();
-      if (_tests.isEmpty && _numProcesses == 0) {
-        _terminateBatchRunners().then((_) => _cleanupAndMarkDone());
+  void _runTests(List<TestSuite> testSuites) {
+    var numberOfTestSuitesFinished = 0;
+
+    void testSuiteFinished() {

[ricow1, 2013/01/15 12:38:44]

how about making this:
void _runTests(List<TestSuite> testSuites) {
  Iterator<TestSuite> iterator = testSuites.iterator();
  void enqueueNextSuite() {
    if (!iterator.hasNext()) {
      _allTestsWereEnqueued = true;
      _progress.allTestsKnown();
  } else {
    iterator.next().forEachTest(_runTest,
				_testCache,
				enqueueNextSuite);
  }
  enqueueNextSuite();
}

[kustermann, 2013/01/16 09:09:09]

Done.

+      numberOfTestSuitesFinished++;
+      if (numberOfTestSuitesFinished == testSuites.length) {
+        _allTestsWereEnqueued = true;
+        _progress.allTestsKnown();
+      } else {
+        testSuites[numberOfTestSuitesFinished].forEachTest(_runTest,
+                                                           _testCache,
+                                                           testSuiteFinished);
       }
     }
+    // FIXME: For some reason we cannot call this method on all test suites

[ricow1, 2013/01/15 12:38:44]

could we file a bug for this

[kustermann, 2013/01/16 09:09:09]

Done.

+    // in parallel.
+    // If we do, not all tests get enqueued (if --arch=all was specified,
+    // we don't get twice the number of tests [tested on -rvm -cnone])
+    testSuites[0].forEachTest(_runTest,
+                              _testCache,
+                              testSuiteFinished);
   }
 
   /**
    * True if we are using a browser + platform combination that needs the
    * Selenium server jar.
    */
   bool get _needsSelenium => (io.Platform.operatingSystem == 'macos' &&
       browserUsed == 'safari') || browserUsed == 'opera';
 
   /** True if the Selenium Server is ready to be used. */
@@ skipping 147 matching lines @@
       _batchProcesses[compiler] = runners;
     }
 
     for (var runner in runners) {
       if (!runner.active) return runner;
     }
     throw new Exception('Unable to find inactive batch runner.');
   }
 
   void _tryRunTest() {
-    _checkDone();
+    if (_allTestsWereEnqueued && _tests.isEmpty && _numProcesses == 0) {
+      _terminateBatchRunners().then((_) => _cleanupAndMarkDone());
+    }
     if (_numProcesses < _maxProcesses && !_tests.isEmpty) {
       TestCase test = _tests.removeFirst();
       if (_listTests) {
         var fields = [test.displayName,
                       Strings.join(new List.from(test.expectedOutcomes), ','),
                       test.isNegative.toString()];
         fields.addAll(test.commands.last.arguments);
         print(Strings.join(fields, '\t'));
         return;
       }
@@ skipping 48 matching lines @@
         // the developer doesn't waste his or her time trying to fix a bunch of
         // tests that appear to be broken but were actually just flakes that
         // didn't get retried because there had already been one failure.
         bool allowRetry = _MAX_FAILED_NO_RETRY > _progress.numFailedTests;
         new RunningProcess(test, allowRetry, this).start();
       }
       _numProcesses++;
     }
   }
 }