Clean up test.dart framework.

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 59 matching lines @@
    * tests have two commands, one to compilate the source and another to execute
    * it. Some isolate tests might even have three, if they require compiling
    * multiple sources that are run in isolation.
    */
   List<Command> commands;
 
   Map configuration;
   String displayName;
   TestOutput output;
   bool isNegative;
+  bool usesWebDriver;

[Mads Ager (google), 2012/10/25 08:11:55]

I'm not sure what we gain by having this as a member instead of the getter. It
seems to be a lot more code for the same functionality?

[Emily Fortuna, 2012/10/25 18:21:20]

I agree. The reason I changed this is I was finding I was having to add "ie10"
in an awful lot of extra places sprinkled throughout our test framework, so this
was an attempt to avoid that. But perhaps the extra spots are worth having less
code.?

[Mads Ager (google), 2012/10/25 18:47:44]

I strongly agree with your use of TestUtils.usesWebDriver as the one place to
have the list of browser runtimes!

What I was wondering is if it would be better to just keep the getter on line
167 of the 'before' part of this change list and not introduce the argument to
the constructor and the field. It seems to me that the getter will give the
correct result and that the amount of places where we have the string 'ie9' will
be the same (no changed line in this CL contains browser names as strings)? That
would reduce this patch set to only the change to fix the use of 'browserUsed'.
I'm fine either way so I'll leave it to your judgement.

   Set<String> expectedOutcomes;
   TestCaseEvent completedHandler;
   TestInformation info;
 
   TestCase(this.displayName,
            this.commands,
            this.configuration,
            this.completedHandler,
            this.expectedOutcomes,
            {this.isNegative: false,
-            this.info: null}) {
+            this.info: null,
+            this.usesWebDriver: false}) {
     if (!isNegative) {
       this.isNegative = displayName.contains("negative_test");
     }
 
     // Special command handling. If a special command is specified
     // we have to completely rewrite the command that we are using.
     // We generate a new command-line that is the special command where we
     // replace '@' with the original command executable, and generate
     // a command formed like the following
     // Let PREFIX be what is before the @.
@@ skipping 56 matching lines @@
     final runtime = configuration['runtime'];
     final mode = configuration['mode'];
     final arch = configuration['arch'];
     final checked = configuration['checked'] ? '-checked' : '';
     return "$compiler-$runtime$checked ${mode}_$arch";
   }
 
   List<String> get batchRunnerArguments => ['-batch'];
   List<String> get batchTestArguments => commands.last().arguments;
 
-  bool get usesWebDriver => TestUtils.usesWebDriver(configuration['runtime']);
-
   void completed() { completedHandler(this); }
 }
 
 
 /**
  * BrowserTestCase has an extra compilation command that is run in a separate
  * process, before the regular test is run as in the base class [TestCase].
  * If the compilation command fails, then the rest of the test is not run.
  */
 class BrowserTestCase extends TestCase {
   /**
    * Indicates the number of potential retries remaining, to compensate for
    * flaky browser tests.
    */
   int numRetries;
 
   BrowserTestCase(displayName, commands, configuration, completedHandler,
-      expectedOutcomes, info, isNegative)
+      expectedOutcomes, info, isNegative, {useWebDriver: false})
     : super(displayName, commands, configuration, completedHandler,
-        expectedOutcomes, isNegative: isNegative, info: info) {
+        expectedOutcomes, isNegative: isNegative, info: info,
+        usesWebDriver: useWebDriver) {
     numRetries = 2; // Allow two retries to compensate for flaky browser tests.
   }
 
   List<String> get _lastArguments => commands.last().arguments;
 
   List<String> get batchRunnerArguments => [_lastArguments[0], '--batch'];
 
   List<String> get batchTestArguments =>
       _lastArguments.getRange(1, _lastArguments.length - 1);
 }
@@ skipping 935 matching lines @@
         print("  Error: $e");
         // TODO(ahe): How to report this as a test failure?
         exit(1);
         return true;
       });
     }
   }
 
   void _runTest(TestCase test) {
     if (test.usesWebDriver) {
-      browserUsed = test.configuration['browser'];
+      browserUsed = test.configuration['runtime'];

[Mads Ager (google), 2012/10/25 08:11:55]

Thank you! I hope this will work out on the bots.

       if (_needsSelenium) _ensureSeleniumServerRunning();
     }
     _progress.testAdded();
     _tests.add(test);
     _tryRunTest();
   }
 
   /**
    * Monitor the output of the Selenium server, to know when we are ready to
    * begin running tests.
@@ skipping 131 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++;
     }
   }
 }