Update test.dart for detection output of machine formatted errors

Update test.dart for detection output of machine formatted errors

[zundel, 2012-02-28 02:17:01]

Not yet ready for prime time - is this what you had in mind?

This change hacks in reading the output of the machine format error codes for
dartc.  What I really need to do is to pass in  the expectations of the  ///
annotations in the source code down to the TestOutput object so it can do a more
precise job of checking for errors and get rid of the --fatal-type-errors hack.

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

LGTM.

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:168: 
Could this be an interface?  Do interfaces have factories?

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:170: }
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.

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:171: 
I think the factory should just be called TestOutput, without the .fromCase. 
It's cleaner.  TestOutputs are only created from running test cases.

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:252: class BrowserTestOutputImpl extends
TestOutputImpl {
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.

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:311: }
Do you want a default case here, or an assert?

[zundel, 2012-02-28 14:12:08]

Thanks for taking a look

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:168: 
On 2012/02/28 14:06:38, Bill Hesse wrote:
> Could this be an interface?  Do interfaces have factories?

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

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:170: }
On 2012/02/28 14:06:38, Bill Hesse wrote:
> 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 aske

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:171: 
On 2012/02/28 14:06:38, Bill Hesse wrote:
> I think the factory should just be called TestOutput, without the .fromCase. 
> It's cleaner.  TestOutputs are only created from running test cases.

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.

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:311: }
On 2012/02/28 14:06:38, Bill Hesse wrote:
> Do you want a default case here, or an assert?

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

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

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9479034/diff/1005/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:171: 
On 2012/02/28 14:12:09, zundel wrote:
> On 2012/02/28 14:06:38, Bill Hesse wrote:
> > I think the factory should just be called TestOutput, without the .fromCase.

> > It's cleaner.  TestOutputs are only created from running test cases.
> 
> 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.

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.

[scheglov, 2012-02-28 23:59:29]

LGTM for DartC part.

https://chromiumcodereview.appspot.com/9479034/diff/1005/compiler/java/com/go...
File compiler/java/com/google/dart/compiler/PrettyErrorFormatter.java (right):

https://chromiumcodereview.appspot.com/9479034/diff/1005/compiler/java/com/go...
compiler/java/com/google/dart/compiler/PrettyErrorFormatter.java:158:
StringBuffer result = new StringBuffer();
AFAIK it is better to use StringBuilder if only one thread has access to it.

[zundel, 2012-02-29 08:11:40]

I am still stymied by the stderr from one test creeping
in to the test output of another.

http://code.google.com/p/dart/issues/detail?id=1898

The assertions in test_runner.dart I added show that the command listed in the
test case doesn't match the name of file in the error output from the tool:

Expect.isTrue(false, 'Unexpected error: exitCode:0
command[0]:out/Debug_ia32/compiler/bin/dartc --ignore-unrecognized-flags
--error_format machine
/home/zundel/dart/tests/co19/src/Language/08_Interfaces/2_Getters_and_Setters_A01_t01.dart
 errors[0]:[ERROR, RESOLVER, CYCLIC_CLASS,
file:/home/zundel/dart/tests/co19/src/Language/08_Interfaces/4_Superinterfaces_A04_t01.dart/4_Superinterfaces_A04_t01.dart,
15, 1, 24, I causes a cycle in the supertype graph]') fails.
 0. Function: 'Expect._fail@127eafe4' url: 'bootstrap' line:1452 col:5
 1. Function: 'Expect.isTrue' url: 'bootstrap' line:1196 col:10
 2. Function: 'AnalysisTestOutputImpl._didStandardTestFail@3f8293b0' url:
'/home/zundel/dart/tools/testing/dart/test_runner.dart' line:364 col:20

https://chromiumcodereview.appspot.com/9479034/diff/1005/compiler/java/com/go...
File compiler/java/com/google/dart/compiler/PrettyErrorFormatter.java (right):

https://chromiumcodereview.appspot.com/9479034/diff/1005/compiler/java/com/go...
compiler/java/com/google/dart/compiler/PrettyErrorFormatter.java:158:
StringBuffer result = new StringBuffer();
On 2012/02/28 23:59:29, scheglov wrote:
> AFAIK it is better to use StringBuilder if only one thread has access to it.

Done.

https://chromiumcodereview.appspot.com/9479034/diff/16001/tools/testing/dart/...
File tools/testing/dart/test_runner.dart (right):

https://chromiumcodereview.appspot.com/9479034/diff/16001/tools/testing/dart/...
tools/testing/dart/test_runner.dart:172: interface TestOutput default
TestOutputImpl {
I didn't come up with much better than what was here before, but I did change
this to an interface.  I didn't want to expose the generative constructor of
TestOutputImpl since I want to direct users of the API to the factory method
instead.

https://chromiumcodereview.appspot.com/9479034/diff/16001/tools/testing/dart/...
tools/testing/dart/test_runner.dart:681: _reportResult(">>> TEST CRASH");
I moved this because _startProcess() clears the _testStdout and _testStdin
fields, thus we loose diagnostic output on a crash.

[zundel, 2012-03-01 06:09:14]

I split this CL in half, one to address the errors in the wrong place, and the
other to change the way dartc tests are evaluated.   The latter is here:

http://codereview.chromium.org/9559007/

On 2012/02/29 08:11:40, zundel wrote:
> I am still stymied by the stderr from one test creeping
> in to the test output of another.
> 
> http://code.google.com/p/dart/issues/detail?id=1898
> 
> The assertions in test_runner.dart I added show that the command listed in the
> test case doesn't match the name of file in the error output from the tool:
> 
> Expect.isTrue(false, 'Unexpected error: exitCode:0
> command[0]:out/Debug_ia32/compiler/bin/dartc --ignore-unrecognized-flags
> --error_format machine
>
/home/zundel/dart/tests/co19/src/Language/08_Interfaces/2_Getters_and_Setters_A01_t01.dart
>  errors[0]:[ERROR, RESOLVER, CYCLIC_CLASS,
>
file:/home/zundel/dart/tests/co19/src/Language/08_Interfaces/4_Superinterfaces_A04_t01.dart/4_Superinterfaces_A04_t01.dart,
> 15, 1, 24, I causes a cycle in the supertype graph]') fails.
>  0. Function: 'Expect._fail@127eafe4' url: 'bootstrap' line:1452 col:5
>  1. Function: 'Expect.isTrue' url: 'bootstrap' line:1196 col:10
>  2. Function: 'AnalysisTestOutputImpl._didStandardTestFail@3f8293b0' url:
> '/home/zundel/dart/tools/testing/dart/test_runner.dart' line:364 col:20
> 
>
https://chromiumcodereview.appspot.com/9479034/diff/1005/compiler/java/com/go...
> File compiler/java/com/google/dart/compiler/PrettyErrorFormatter.java (right):
> 
>
https://chromiumcodereview.appspot.com/9479034/diff/1005/compiler/java/com/go...
> compiler/java/com/google/dart/compiler/PrettyErrorFormatter.java:158:
> StringBuffer result = new StringBuffer();
> On 2012/02/28 23:59:29, scheglov wrote:
> > AFAIK it is better to use StringBuilder if only one thread has access to it.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/9479034/diff/16001/tools/testing/dart/...
> File tools/testing/dart/test_runner.dart (right):
> 
>
https://chromiumcodereview.appspot.com/9479034/diff/16001/tools/testing/dart/...
> tools/testing/dart/test_runner.dart:172: interface TestOutput default
> TestOutputImpl {
> I didn't come up with much better than what was here before, but I did change
> this to an interface.  I didn't want to expose the generative constructor of
> TestOutputImpl since I want to direct users of the API to the factory method
> instead.
> 
>
https://chromiumcodereview.appspot.com/9479034/diff/16001/tools/testing/dart/...
> tools/testing/dart/test_runner.dart:681: _reportResult(">>> TEST CRASH");
> I moved this because _startProcess() clears the _testStdout and _testStdin
> fields, thus we loose diagnostic output on a crash.

[scheglov, 2012-03-01 21:42:29]

No DartC changes, so still LGTM for me.

https://chromiumcodereview.appspot.com/9479034/diff/16001/tests/language/src/...
File tests/language/src/BoolTest.dart (right):

https://chromiumcodereview.appspot.com/9479034/diff/16001/tests/language/src/...
tests/language/src/BoolTest.dart:40: Expect.equals(true, true !== (true ==
false));
Why these trailing spaces?

[zundel, 2012-03-01 21:51:43]

https://chromiumcodereview.appspot.com/9479034/diff/16001/tests/language/src/...
File tests/language/src/BoolTest.dart (right):

https://chromiumcodereview.appspot.com/9479034/diff/16001/tests/language/src/...
tests/language/src/BoolTest.dart:40: Expect.equals(true, true !== (true ==
false));
On 2012/03/01 21:42:30, scheglov wrote:
> Why these trailing spaces?

I will take care of them. I was abusing BoolTest to make sure all the different
scenarios I just added were working.