Update test.dart for detection output of machine formatted errors

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 144 matching lines @@
       arguments.getRange(1, arguments.length - 1);
 }
 
 
 /**
  * TestOutput records the output of a completed test: the process's exit code,
  * the standard output and standard error, whether the process timed out, and
  * the time the process took to run.  It also contains a pointer to the
  * [TestCase] this is the output of.
  */
-class TestOutput {
+
+// TODO(zundel): should be abstract?
+class TestOutput { 
+  

[Bill Hesse, 2012/02/28 14:06:38]

Could this be an interface?  Do interfaces have factories?

[zundel, 2012/02/28 14:12:09]

you can put a constructor and provide a default implementation, I'll play with
that.

+  TestOutput() {
+  }

[Bill Hesse, 2012/02/28 14:06:38]

Is the split between TestOutput and TestOutputImpl really necessary?  TestOutput
can have a factory TestOutput, and an instance constructor TestOutput.instance,
which is called by the factory if a subclass is not asked for.

+  

[Bill Hesse, 2012/02/28 14:06:38]

I think the factory should just be called TestOutput, without the .fromCase. 
It's cleaner.  TestOutputs are only created from running test cases.

[zundel, 2012/02/28 14:12:09]

I agree, BUT it turns out that there is an implicit super() constructor call
from subclasses that you can't satisfy if you create TestOutput as a factory
class.

[zundel, 2012/02/28 14:22:35]

I meant factory constructor.  Like this:

class Foo {
  factory Foo(a,b,c) {
    return new FooSub(a,b,c);
  }
}

class FooSub {
  FooSub(a,b,c) { // implicit super() initializer here
  }
}

You work yourself into a pickle here, because there is no constructor for the
superclass of FooSub you can call.

+  factory TestOutput.fromCase (testCase, exitCode, timedOut, stdout, stderr, 
+    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);
+  }
+
+  abstract String get result();
+  
+  abstract bool get unexpectedOutput();
+  
+  abstract bool get hasCrashed();
+  
+  abstract bool get hasTimedOut(); 
+  
+  abstract bool get didFail();
+  
+  List<String> get errors() { return []; }
+  
+  List<String> get staticWarnings() { return []; }
+  
+  List<String> get warnings() { return []; }
+}
+
+class TestOutputImpl extends TestOutput {
   TestCase testCase;
   int exitCode;
   bool timedOut;
   bool failed = false;
   List<String> stdout;
   List<String> stderr;
   Duration time;
+  
   /**
    * Set to true if we encounter a condition in the output that indicates we
    * need to rerun this test.
    */
   bool requestRetry;
 
-  TestOutput(this.testCase, this.exitCode, this.timedOut, this.stdout,
+  TestOutputImpl(this.testCase, this.exitCode, this.timedOut, this.stdout,
              this.stderr, this.time) {
     testCase.output = this;
     requestRetry = false;
   }
 
   String get result() =>
       hasCrashed ? CRASH : (hasTimedOut ? TIMEOUT : (hasFailed ? FAIL : PASS));
 
   bool get unexpectedOutput() => !testCase.expectedOutcomes.contains(result);
 
   bool get hasCrashed() {
     if (new Platform().operatingSystem() == 'windows') {
       // The VM uses std::abort to terminate on asserts.
       // std::abort terminates with exit code 3 on Windows.
       if (exitCode == 3) {
         return !timedOut;
       }
       return (!timedOut && (exitCode < 0) && ((0x3FFFFF00 & exitCode) == 0));
     }
     // The Java dartc runner exits with code 253 in case of unhandled
     // exceptions.
     return (!timedOut && ((exitCode < 0) || (exitCode == 253)));
   }
 
   bool get hasTimedOut() => timedOut;
 
   bool get didFail() {
-    if (testCase is !BrowserTestCase) return (exitCode != 0 && !hasCrashed);
+    return (exitCode != 0 && !hasCrashed);
+  }
+  
+  // Reverse result of a negative test.
+  bool get hasFailed() => (testCase.isNegative ? !didFail : didFail);  
+}
 
+class BrowserTestOutputImpl extends TestOutputImpl {

[Bill Hesse, 2012/02/28 14:06:38]

This is good.  A step towards a final refactoring, which I think would be that 
TestOutput.didFail() => testCase.component.didFail(this);
So that the special handling of output for each component would be in a class
representing that component.  If we have a set of classes representing different
component types, then a lot of custom logic can go into there, rather than being
in TestCase, TestOutput, and runner.  But this is good for now.

+  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) {
       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.
         requestRetry = true;
         return true;
       }
     }
 
     // Browser tests fail unless stdout contains
     // 'Content-Type: text/plain\nPASS'.
     String previous_line = '';
     for (String line in stdout) {
       if (line == 'PASS' && previous_line == 'Content-Type: text/plain') {
         return (exitCode != 0 && !hasCrashed);
       }
       previous_line = line;
     }
-
     return true;
   }
+}
 
-  // Reverse result of a negative test.
-  bool get hasFailed() => (testCase.isNegative ? !didFail : didFail);
+class AnalysisTestOutputImpl extends TestOutputImpl {
+  final errors;
+  final warnings;
+  final staticWarnings;
+  
+  AnalysisTestOutputImpl(testCase, exitCode, timedOut, stdout, stderr, time) : 
+    super(testCase, exitCode, timedOut, stdout, stderr, time), 
+    errors = new Map<int, String>(),
+    warnings = new Map<int, String>(),
+    staticWarnings = new Map<int, String>() {
+    
+    // read the returned list of errors and stuff them away.
+    for (String line in stderr) {
+      if (line.length == 0) continue;
+      List<String> fields = splitMachineError(line);
+      switch(fields[0]) {
+      case "ERROR":
+        errors[fields[4]] = fields;
+        break;
+      case "WARNING":
+        if (fields[1] == "STATIC_TYPE") {
+          staticWarnings[fields[4]] = fields;
+        } else {
+          warnings[fields[4]] = fields;
+        }
+        break;
+      }

[Bill Hesse, 2012/02/28 14:06:38]

Do you want a default case here, or an assert?

[zundel, 2012/02/28 14:12:09]

There is some output to stderr that needs to be ignored so I'll document that.

+    }
+  }
+  
+  bool get didFail() {
+    if (hasCrashed) return false;
+    
+    if (this.testCase.commandLine.contains('--fatal-type-errors')) {
+      if (staticWarnings.length != 0) {
+        return true;
+      }
+    } else if (errors.length != 0) {
+      return true;
+    }
+    
+    // TODO(zundel): more sophisticated analysis
+   
+    return false;
+    //return exitCode != 0;  
+  }
+  
+  // Parse a line delimited by the | character using \ as an escape charager
+  // like:  FOO|BAR|FOO\|BAR|FOO\\BAZ as 4 fields: FOO BAR FOO|BAR FOO\BAZ
+  List<String> splitMachineError(String line) {
+    StringBuffer field = new StringBuffer();
+    List<String> result = [];
+    bool escaped = false;
+    for (var i = 0 ; i < line.length; i++) {
+      var c = line[i];
+      if (!escaped && c == '\\') {
+        escaped = true;
+        continue;
+      }
+      escaped = false;
+      if (c == '|') {
+        result.add(field.toString());
+        field.clear();
+        continue;
+      }
+      field.add(c);
+    }
+    result.add(field.toString());
+    return result;
+  }
 }
 
 /**
  * A RunningProcess actually runs a test, getting the command lines from
  * its [TestCase], starting the test process (and first, a compilation
  * process if the TestCase is a [BrowserTestCase]), creating a timeout
  * timer, and recording the results in a new [TestOutput] object, which it
  * attaches to the TestCase.  The lifetime of the RunningProcess is limited
  * to the time it takes to start the process, run the process, and record
  * the result; there are no pointers to it, so it should be available to
  * be garbage collected as soon as it is done.
  */
 class RunningProcess {
   ProcessQueue processQueue;
   Process process;
   TestCase testCase;
   bool timedOut = false;
   Date startTime;
   Timer timeoutTimer;
   List<String> stdout;
   List<String> stderr;
   List<Function> handlers;
   bool allowRetries = false;
 
   RunningProcess(TestCase this.testCase,
       [this.allowRetries, this.processQueue]);
 
   void exitHandler(int exitCode) {
-    new TestOutput(testCase, exitCode, timedOut, stdout,
-                   stderr, new Date.now().difference(startTime));
+    new TestOutput.fromCase(testCase, exitCode, timedOut, stdout,
+                            stderr, new Date.now().difference(startTime));
     process.close();
     timeoutTimer.cancel();
     if (testCase.output.unexpectedOutput && testCase.configuration['verbose']) {
       print(testCase.displayName);
       for (var line in testCase.output.stderr) print(line);
       for (var line in testCase.output.stdout) print(line);
     }
     if (allowRetries != null && allowRetries
         && testCase.configuration['component'] == 'webdriver' &&
         testCase.output.unexpectedOutput && testCase.numRetries > 0) {
@@ skipping 170 matching lines @@
 
   int _reportResult(String output) {
     var test = _currentTest;
     _currentTest = null;
 
     // output = '>>> TEST {PASS, FAIL, OK, CRASH, FAIL, TIMEOUT}'
     var outcome = output.split(" ")[2];
     var exitCode = 0;
     if (outcome == "CRASH") exitCode = -10;
     if (outcome == "FAIL" || outcome == "TIMEOUT") exitCode = 1;
-    new TestOutput(test, exitCode, outcome == "TIMEOUT", _testStdout,
-                   _testStderr, new Date.now().difference(_startTime));
+    new TestOutput.fromCase(test, exitCode, outcome == "TIMEOUT", _testStdout,
+                            _testStderr, new Date.now().difference(_startTime));
     test.completed();
   }
 
   Function _readOutput(StringInputStream stream, List<String> buffer) {
     return () {
       var status;
       var line = stream.readLine();
       // Drain the input stream to get the error output.
       while (line != null) {
         if (line.startsWith('>>> TEST')) {
@@ skipping 286 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++;
     }
   }
 }