Add test_runner.dart to Dart version of test scripts. Include TestRunnerTest unit test.

Add test_runner.dart to Dart version of test scripts.  Include TestRunnerTest unit test.


BUG=
TEST=standalone/TestRunnerTest

Committed: https://code.google.com/p/dart/source/detail?r=1245

[Bill Hesse, 2011-11-03 14:10:09]

test_runner.dart:133 - Is this a data race?  Starting a process, and then adding
a read callback to its output stream?  But the stream can't be created until the
process is started, right?

Are callbacks resolved at the time of scheduling or at the time of execution?

[Søren Gjesse, 2011-11-04 09:05:34]

lgtm

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/CrashTe...
File tests/standalone/src/CrashTest.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/CrashTe...
tests/standalone/src/CrashTest.dart:6: // A test that crashes (for now).
Does this crash due to a VM bug? We have a binary (process_test) which can be
made to crash reliabily, which you should probably use here. See the test
standalone/ProcessSegfaulttest.dart. And http://codereview.chromium.org/8365032/
for locating the binary.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
File tests/standalone/src/TestRunnerTest.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
tests/standalone/src/TestRunnerTest.dart:38: return new
TestCase("out/Debug_ia32/dart_bin", <String>[
This path does not work cross platform/mode, see
http://codereview.chromium.org/8365032/ on how to handle this for now.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
tests/standalone/src/TestRunnerTest.dart:41:
"tests/standalone/src/$testName.dart"],
Maybe use ${testName} for clarity.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/Timeout...
File tests/standalone/src/TimeoutTest.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/Timeout...
tests/standalone/src/TimeoutTest.dart:7: void main() {
I don't like busy-looping just to test timeout. How about making a test which
misses closing a port to cause the timeout.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:9: // #import("test_configuration.dart");
"Code" in comments.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:47: 
Add empty line.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:89: List<String> output;
Use names stdout and stderr as on TestOutput class above?

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:98: void exitHandler(int exitCode) {
When we get the stream close handling working correctly (hopefully
http://codereview.chromium.org/8437090/ will be a step in that direction). We
should wait for exit and then end of both streams.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:106: 
Only one empty line between methods for consistency.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:117: 
Ditto.

[Mads Ager (google), 2011-11-04 09:06:13]

LGTM

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/CrashTe...
File tests/standalone/src/CrashTest.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/CrashTe...
tests/standalone/src/CrashTest.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
I would prefer to find some other way of testing this. Adding a new test here
that relies on this crashing is going to make it annoying when that bug is
fixed.

Maybe we need to have a mode where we expose a couple of test methods. Including
one to crash the system. I would remove this test until we have that.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
File tests/standalone/src/TestRunnerTest.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
tests/standalone/src/TestRunnerTest.dart:52: Expect.throws(() =>
Funky indentation and you don't need the () => ...()

Expect.throws(
    new RunningProcess(MakeTestCase("CrashTest", [SKIP]), 30).start);

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/standalone....
File tests/standalone/standalone.status (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/standalone....
tests/standalone/standalone.status:16: CrashTest: Skip
Crash, but I think we need a better test for this so just remove for now?

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:9: // #import("test_configuration.dart");
Remove commented out code.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:49: TestCase testCase;  // The TestCase this
is the output for.
I would add the comment above the line instead.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:61: 
Remove extra blank?

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:68: bool get hasCrashed() =>
This will fit on one line I think?

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:75: bool get hasFailed() =>  // Reverse
result of a negative test.
Please move this comment to the line above and make hasFailed a one-liner:

// Reverse result of a negative test.
bool get hasFailed() => (testCase.isNegative ? !didFail : didFail);

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:83: String executablePath;
Isn't the executablePath and the arguments also in the testCase? Do you need
them here?

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:87: int timerKey;  // Running processes have
a timeout check callback in a map.
Move comment above the field instead?

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:95: RunningProcess(this.testCase,
One line?

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:105: 
Either consistently use two new lines or one.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:148: class TimerChecks {
I'm not sure I like this approach. This means that we spent time testing each
test for timeout each second. Why not just start a new Timer per test and cancel
it when the test completes?

[Bill Hesse, 2011-11-04 14:49:22]

All changes made, centralized timer removed.  Will be modified to use
getProcessTestFileName() (in comments) before commit.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
File tests/standalone/src/TestRunnerTest.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
tests/standalone/src/TestRunnerTest.dart:38: return new
TestCase("out/Debug_ia32/dart_bin", <String>[
On 2011/11/04 09:05:34, Søren Gjesse wrote:
> This path does not work cross platform/mode, see
> http://codereview.chromium.org/8365032/ on how to handle this for now.

Done.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
tests/standalone/src/TestRunnerTest.dart:41:
"tests/standalone/src/$testName.dart"],
On 2011/11/04 09:05:34, Søren Gjesse wrote:
> Maybe use ${testName} for clarity.

Done.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/TestRun...
tests/standalone/src/TestRunnerTest.dart:52: Expect.throws(() =>
On 2011/11/04 09:06:13, Mads Ager wrote:
> Funky indentation and you don't need the () => ...()
> 
> Expect.throws(
>     new RunningProcess(MakeTestCase("CrashTest", [SKIP]), 30).start);

Done.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/Timeout...
File tests/standalone/src/TimeoutTest.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/src/Timeout...
tests/standalone/src/TimeoutTest.dart:7: void main() {
On 2011/11/04 09:05:34, Søren Gjesse wrote:
> I don't like busy-looping just to test timeout. How about making a test which
> misses closing a port to cause the timeout.

Done.

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/standalone....
File tests/standalone/standalone.status (right):

http://codereview.chromium.org/8440066/diff/1008/tests/standalone/standalone....
tests/standalone/standalone.status:16: CrashTest: Skip
On 2011/11/04 09:06:13, Mads Ager wrote:
> Crash, but I think we need a better test for this so just remove for now?

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:9: // #import("test_configuration.dart");
On 2011/11/04 09:06:13, Mads Ager wrote:
> Remove commented out code.

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:47: 
On 2011/11/04 09:05:34, Søren Gjesse wrote:
> Add empty line.

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:49: TestCase testCase;  // The TestCase this
is the output for.
On 2011/11/04 09:06:13, Mads Ager wrote:
> I would add the comment above the line instead.

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:61: 
On 2011/11/04 09:06:13, Mads Ager wrote:
> Remove extra blank?

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:68: bool get hasCrashed() =>
On 2011/11/04 09:06:13, Mads Ager wrote:
> This will fit on one line I think?

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:83: String executablePath;
On 2011/11/04 09:06:13, Mads Ager wrote:
> Isn't the executablePath and the arguments also in the testCase? Do you need
> them here?

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:87: int timerKey;  // Running processes have
a timeout check callback in a map.
Why?
I have gotten rid of this anyway, by creating a timer per process.

On 2011/11/04 09:06:13, Mads Ager wrote:
> Move comment above the field instead?

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:89: List<String> output;
On 2011/11/04 09:05:34, Søren Gjesse wrote:
> Use names stdout and stderr as on TestOutput class above?

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:95: RunningProcess(this.testCase,
On 2011/11/04 09:06:13, Mads Ager wrote:
> One line?

Done.

http://codereview.chromium.org/8440066/diff/1008/tools/testing/dart/test_runn...
tools/testing/dart/test_runner.dart:106: 
On 2011/11/04 09:05:34, Søren Gjesse wrote:
> Only one empty line between methods for consistency.

Done.

[Bill Hesse, 2011-11-07 10:32:30]

Added fix to string_stream.dart.

[Søren Gjesse, 2011-11-07 10:38:43]

lgtm