Fix some type and logical errors in test.dart, found by the Dart editor.

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.
  */
 #library("test_runner");
 
 #import("dart:io");
+#import("dart:builtin");
 #import("status_file_parser.dart");
 #import("test_progress.dart");
 #import("test_suite.dart");
 
 final int NO_TIMEOUT = 0;
 
 /** A command executed as a step in a test case. */
 class Command {
   /** Path to the executable of this command. */
   String executable;
@@ skipping 27 matching lines @@
  * The TestCase has a callback function, [completedHandler], that is run when
  * the test is completed.
  */
 class TestCase {
   /**
    * A list of commands to execute. Most test cases have a single command. Frog
    * 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.
    */
-  final List<Command> commands;
+  List<Command> commands;
 
   Map configuration;
   String displayName;
   TestOutput output;
   bool isNegative;
   Set<String> expectedOutcomes;
   Function completedHandler;
   TestInformation info;
 
   TestCase(this.displayName,
@@ skipping 25 matching lines @@
 
         if (prefix.length > 0) {
           var prefixSplit = prefix.split(' ');
           newExecutablePath = prefixSplit[0];
           for (int i = 1; i < prefixSplit.length; i++) {
             var current = prefixSplit[i];
             if (!current.isEmpty()) newArguments.add(current);
           }
           newArguments.add(c.executable);
         }
-        newArguments.addAll(arguments);
+        newArguments.addAll(c.arguments);
         var suffixSplit = suffix.split(' ');
         suffixSplit.forEach((e) {
           if (!e.isEmpty()) newArguments.add(e);
         });
         final newCommand = new Command(newExecutablePath, newArguments);
         newCommands.add(newCommand);
         Expect.stringEquals('$prefix ${c.commandLine} $suffix',
             newCommand.commandLine);
       }
-      commands = newCommand;
+      commands = newCommands;
     }
   }
 
   int get timeout() => configuration['timeout'];
 
   String get configurationString() {
     final component = configuration['component'];
     final mode = configuration['mode'];
     final arch = configuration['arch'];
     return "$component ${mode}_$arch";
@@ skipping 48 matching lines @@
 
   String get result();
 
   bool get unexpectedOutput();
 
   bool get hasCrashed();
 
   bool get hasTimedOut();
 
   bool get didFail();
+  
+  Duration get time();
+  
+  List<String> get stdout();
 
+  List<String> get stderr();
+  
   List<String> get diagnostics();
 }
 
 class TestOutputImpl implements TestOutput {
   TestCase testCase;
   int exitCode;
   bool timedOut;
   bool failed = false;
   List<String> stdout;
   List<String> stderr;
   Duration time;
   List<String> diagnostics;
 
   /**
    * Set to true if we encounter a condition in the output that indicates we
    * need to rerun this test.
    */
   bool requestRetry = false;
 
   // Don't call  this constructor, call TestOutput.fromCase() to
   // get anew TestOutput instance.
-  TestOutputImpl(this.testCase, this.exitCode, this.timedOut, this.stdout,
-             this.stderr, this.time) {
+  TestOutputImpl(TestCase this.testCase,
+                 int this.exitCode,
+                 bool this.timedOut,
+                 List<String> this.stdout,
+                 List<String> this.stderr,
+                 Duration this.time) {
     testCase.output = this;
     diagnostics = [];
   }
 
-  factory TestOutputImpl.fromCase (testCase, exitCode, timedOut,
-                                   stdout, stderr, time) {
+  factory TestOutputImpl.fromCase (TestCase testCase, int exitCode, bool timedOut,
+                                   List<String> stdout, List<String> stderr, Duration time) {
     if (testCase is BrowserTestCase) {
       return new BrowserTestOutputImpl(testCase, exitCode, timedOut,
         stdout, stderr, time);
     } else if (testCase.configuration['component'] == 'dartc') {
       return new AnalysisTestOutputImpl(testCase, exitCode, timedOut,
         stdout, stderr, time);
     }
     return new TestOutputImpl(testCase, exitCode, timedOut,
       stdout, stderr, time);
   }
@@ skipping 31 matching lines @@
 class BrowserTestOutputImpl extends TestOutputImpl {
   BrowserTestOutputImpl(testCase, exitCode, timedOut, stdout, stderr, time) :
     super(testCase, exitCode, timedOut, stdout, stderr, time);
 
   bool get didFail() {
     // Browser case:
     // If the browser test failed, it may have been because DumpRenderTree
     // and the virtual framebuffer X server didn't hook up, or DRT crashed with
     // a core dump. Sometimes DRT crashes after it has set the stdout to PASS,
     // so we have to do this check first.
-    for (String line in stderr) {
+    for (String line in super.stderr) {

[zundel, 2012/03/12 15:35:27]

This is really tricky, overriding a field/getter in the superclass with a
different type.  I believe that is supposed to cause a type error as well.  I
think it would be better to rename one of the fields.

[zundel, 2012/03/12 16:40:19]

Nm, this isn't the problem I thought it was.  I hit 'F3' and th editor took me
back to the wrong stderr.

[Bill Hesse, 2012/03/12 17:28:10]

This class and the superclass are supposed to have the same types for stderr. 
I'm just trying to avoid the top level OutputStream stderr from dart:io. 
Inherited names are weaker than global scope names.

Otherwise, we can change all of these to stderrLines.


On 2012/03/12 15:35:27, zundel wrote:

       if (line.contains('Gtk-WARNING **: cannot open display: :99') ||
         line.contains('Failed to run command. return code=1')) {
         // If we get the X server error, or DRT crashes with a core dump, retry
         // the test.
         if (testCase.dynamic.numRetries > 0) {
           requestRetry = true;
         }
         return true;
       }
     }
 
     // Browser tests fail unless stdout contains
     // 'Content-Type: text/plain\nPASS'.
     String previous_line = '';
-    for (String line in stdout) {
+    for (String line in super.stdout) {
       if (line == 'PASS' && previous_line == 'Content-Type: text/plain') {
         return (exitCode != 0 && !hasCrashed);
       }
       previous_line = line;
     }
     return true;
   }
 }
 
-// The static analyzer does not actaully execute code, so
+// The static analyzer does not actually execute code, so
 // the criteria for success now depend on the text sent
 // to stderr.
 class AnalysisTestOutputImpl extends TestOutputImpl {
   bool alreadyComputed = false;
   bool failResult;
   AnalysisTestOutputImpl(testCase, exitCode, timedOut, stdout, stderr, time) :
     super(testCase, exitCode, timedOut, stdout, stderr, time) {
   }
 
   bool get didFail() {
     if (!alreadyComputed) {
       failResult = _didFail();
       alreadyComputed = true;
     }
     return failResult;
   }
 
   bool _didFail() {
     if (hasCrashed) return false;
 
     List<String> errors = [];
     List<String> staticWarnings = [];
 
     // Read the returned list of errors and stuff them away.
-    for (String line in stderr) {
+    for (String line in super.stderr) {
       if (line.length == 0) continue;
       List<String> fields = splitMachineError(line);
       if (fields[0] == 'ERROR') {
         errors.add(fields);
       } else if (fields[0] == 'WARNING') {
         // We only care about testing Static type warnings
         // ignore all others
         if (fields[1] == 'STATIC_TYPE') {
           staticWarnings.add(fields);
         }
@@ skipping 667 matching lines @@
    * Java server.
    */
   void _startSeleniumServer() {
     // Get the absolute path to the Selenium jar.
     String filePath = new Options().script;
     String pathSep = new Platform().pathSeparator();
     int index = filePath.lastIndexOf(pathSep);
     filePath = filePath.substring(0, index) + '${pathSep}testing${pathSep}';
     var dir = new Directory(filePath);
     dir.onFile = (String file) {
-      if (const RegExp(@"selenium-server-standalone-.*\.jar").hasMatch(file)
+      if (const RegExp("selenium-server-standalone-.*\.jar").hasMatch(file)

[Bill Hesse, 2012/03/12 17:28:10]

This is a stray typo.

This accidentally made it into the commit.  Will be reverted.

           && _seleniumServer == null) {
         _seleniumServer = new Process.start('java', ['-jar', file]);
         // Heads up: there seems to an obscure data race of some form in
         // the VM between launching the server process and launching the test
         // tasks that disappears when you read IO (which is convenient, since
         // that is our condition for knowing that the server is ready).
         StringInputStream stdoutStringStream =
             new StringInputStream(_seleniumServer.stdout);
         StringInputStream stderrStringStream =
             new StringInputStream(_seleniumServer.stderr);
@@ skipping 77 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++;
     }
   }
 }