Add asynchronous test suite runner to Dart rewrite of test scripts.

Add asynchronous test suite runner to Dart rewrite of test scripts.

BUG=
TEST=

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

[Bill Hesse, 2011-11-07 17:18:31]

Test runner that sends TestCases to the process queue asynchronously as it gets
the file names from the directory!

Currently streaming standalone and corelib tests simultaneously.

[Mads Ager (google), 2011-11-08 08:26:06]

This will be really cool. A number of comments and suggestions.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart
File tests/corelib/test_config.dart (right):

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:10: final String VM_PATH =
Need something else here.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:14: Function doneCallback;
Can we make the naming more uniform here? 

handleTest, handleDone? 

testCallback, doneCallback?

onTest, onDone?

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:15: final String directoryPath =
"tests/corelib/src";
This only works in the top-level directory, right?

Also, it would be nice to group the final members at the top.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:19: void getTests(Function callback ,[Function
done = null]) {
Maybe some other name that indicates that we do not get a collection of tests
here? forEachTest?

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:27: if (!dir.existsSync()) return null;
Just 'return' instead of 'return null'?

Could you add a "TODO(ager): Use dir.errorHandler instead when directory
implementation correctly uses the error callback."

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:34: if (filename.endsWith("Test.dart")) {
Let's not check for "Test.dart" but only for ".dart". We really should rename
all the tests to lower_case_underscore.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:36: handleTest(new TestCase(filename, VM_PATH,
I would have one argument per line to make it more readable.

TestCase(filename,
         VM_PATH,
         ...);

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:46: print("Exit code: ${testCase.output.exitCode}
   Time: ${testCase.output.time}");
Long line.

http://codereview.chromium.org/8492013/diff/1/tests/standalone/standalone.status
File tests/standalone/standalone.status (right):

http://codereview.chromium.org/8492013/diff/1/tests/standalone/standalone.sta...
tests/standalone/standalone.status:30: TestRunnerTest: Skip # Uses Process. 
Dart issue 363.
Why add TestRunnerTest here?

http://codereview.chromium.org/8492013/diff/1/tests/standalone/test_config.dart
File tests/standalone/test_config.dart (right):

http://codereview.chromium.org/8492013/diff/1/tests/standalone/test_config.da...
tests/standalone/test_config.dart:12:
"/usr/local/google/home/whesse/dart/out/Debug_ia32/dart_bin";
All the same comments as for the other config file.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_control...
File tools/testing/dart/test_controller.dart (right):

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_control...
tools/testing/dart/test_controller.dart:1: // Copyright (c) 2011, the Dart
project authors.  Please see the AUTHORS file
This is the actual test script. Can you move it to tools/test.dart?

Also, could you make the first line

#!/usr/bin/env dart_bin

That way users just have to put dart_bin in their path and they can run the
script directly from the command line.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_control...
tools/testing/dart/test_controller.dart:6: 
I would remove one new line here (or add another one after the imports).

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:161: final int maxProcesses = 4;
Let's do this right and take this as an argument? We have

var platform = new Platform();
platform.numberOfProcessors();

to give the actual number of available processors (and we should implement the
-j option to the script).

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:167: print("           ***");
Remove.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:170: print("running ${test.displayName}");
Add a TODO here to refactor this into various test output methods.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:171: Function old_callback =
test.completedHandler;
Why do you extract old_callback here? Why not just use it below?

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:180: new RunningProcess(test, 60).start();
Add TODO here to take the actual timeout information from the test case?

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:187: class AsyncProcessQueue {
Let's switch the names. This should be the ProcessQueue and the other one should
be SyncProcessQueue or ProcessQueueSync.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:189: final int maxProcesses = 4;
Let's pass in the actual number of processors?

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:195: print("           ***");
Remove.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:196: while (numProcesses < maxProcesses &&
!tests.isEmpty()) {
I don't think you need the while loop here. You are always spinning up one test
at a time and getting one completed callback at a time. So just start the next
test in the queue? So change the while to an if?

[Bill Hesse, 2011-11-08 14:38:56]

All comments addressed.  The result is smaller and cleaner.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart
File tests/corelib/test_config.dart (right):

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:10: final String VM_PATH =
On 2011/11/08 08:26:06, Mads Ager wrote:
> Need something else here.

Done.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:14: Function doneCallback;
On 2011/11/08 08:26:06, Mads Ager wrote:
> Can we make the naming more uniform here? 
> 
> handleTest, handleDone? 
> 
> testCallback, doneCallback?
> 
> onTest, onDone?

Done.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:15: final String directoryPath =
"tests/corelib/src";
Now it also works in the runtime directory.

On 2011/11/08 08:26:06, Mads Ager wrote:
> This only works in the top-level directory, right?
> 
> Also, it would be nice to group the final members at the top.
Done.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:19: void getTests(Function callback ,[Function
done = null]) {
On 2011/11/08 08:26:06, Mads Ager wrote:
> Maybe some other name that indicates that we do not get a collection of tests
> here? forEachTest?

Done.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:27: if (!dir.existsSync()) return null;
On 2011/11/08 08:26:06, Mads Ager wrote:
> Just 'return' instead of 'return null'?
> 
> Could you add a "TODO(ager): Use dir.errorHandler instead when directory
> implementation correctly uses the error callback."

Done.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:34: if (filename.endsWith("Test.dart")) {
On 2011/11/08 08:26:06, Mads Ager wrote:
> Let's not check for "Test.dart" but only for ".dart". We really should rename
> all the tests to lower_case_underscore.

Done.

http://codereview.chromium.org/8492013/diff/1/tests/corelib/test_config.dart#...
tests/corelib/test_config.dart:36: handleTest(new TestCase(filename, VM_PATH,
On 2011/11/08 08:26:06, Mads Ager wrote:
> I would have one argument per line to make it more readable.
> 
> TestCase(filename,
>          VM_PATH,
>          ...);
> 

Done.

http://codereview.chromium.org/8492013/diff/1/tests/standalone/standalone.status
File tests/standalone/standalone.status (right):

http://codereview.chromium.org/8492013/diff/1/tests/standalone/standalone.sta...
tests/standalone/standalone.status:30: TestRunnerTest: Skip # Uses Process. 
Dart issue 363.
On 2011/11/08 08:26:06, Mads Ager wrote:
> Why add TestRunnerTest here?

Because it is failing due to flakiness in the Process implementation, we think. 
This has actually already been committed.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_control...
File tools/testing/dart/test_controller.dart (right):

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_control...
tools/testing/dart/test_controller.dart:1: // Copyright (c) 2011, the Dart
project authors.  Please see the AUTHORS file
On 2011/11/08 08:26:06, Mads Ager wrote:
> This is the actual test script. Can you move it to tools/test.dart?
> 
> Also, could you make the first line
> 
> #!/usr/bin/env dart_bin
> 
> That way users just have to put dart_bin in their path and they can run the
> script directly from the command line.

Done.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_control...
tools/testing/dart/test_controller.dart:6: 
On 2011/11/08 08:26:06, Mads Ager wrote:
> I would remove one new line here (or add another one after the imports).

Done.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
File tools/testing/dart/test_runner.dart (right):

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:167: print("           ***");
On 2011/11/08 08:26:06, Mads Ager wrote:
> Remove.

Done.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:170: print("running ${test.displayName}");
On 2011/11/08 08:26:06, Mads Ager wrote:
> Add a TODO here to refactor this into various test output methods.

Done.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:171: Function old_callback =
test.completedHandler;
On 2011/11/08 08:26:06, Mads Ager wrote:
> Why do you extract old_callback here? Why not just use it below?

I think that won't work.  We will call the current value of
test.completedHandler, which will be the Function wrapper, and we will have an
infinite regress.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:180: new RunningProcess(test, 60).start();
On 2011/11/08 08:26:06, Mads Ager wrote:
> Add TODO here to take the actual timeout information from the test case?

Done.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:187: class AsyncProcessQueue {
On 2011/11/08 08:26:06, Mads Ager wrote:
> Let's switch the names. This should be the ProcessQueue and the other one
should
> be SyncProcessQueue or ProcessQueueSync.

Only Async version provided, called ProcessQueue.
Done.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:189: final int maxProcesses = 4;
On 2011/11/08 08:26:06, Mads Ager wrote:
> Let's pass in the actual number of processors?

Done.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:195: print("           ***");
On 2011/11/08 08:26:06, Mads Ager wrote:
> Remove.

Done.

http://codereview.chromium.org/8492013/diff/1/tools/testing/dart/test_runner....
tools/testing/dart/test_runner.dart:196: while (numProcesses < maxProcesses &&
!tests.isEmpty()) {
On 2011/11/08 08:26:06, Mads Ager wrote:
> I don't think you need the while loop here. You are always spinning up one
test
> at a time and getting one completed callback at a time. So just start the next
> test in the queue? So change the while to an if?

Done.

[Mads Ager (google), 2011-11-08 14:58:21]

LGTM with these comments addressed.

http://codereview.chromium.org/8492013/diff/3004/tests/corelib/test_config.dart
File tests/corelib/test_config.dart (right):

http://codereview.chromium.org/8492013/diff/3004/tests/corelib/test_config.da...
tests/corelib/test_config.dart:21: void forEachTest(Function onTest ,[Function
onDone = null]) {
Move comma to right after onTest.

http://codereview.chromium.org/8492013/diff/3004/tests/corelib/test_config.da...
tests/corelib/test_config.dart:31: if (!dir.existsSync()) return;
Instead of just returning here, shouldn't we throw an exception? If we do
nothing it will look like the tests are all passing when in fact we did not find
them.

http://codereview.chromium.org/8492013/diff/3004/tests/corelib/test_config.da...
tests/corelib/test_config.dart:40: if (filename.endsWith(".dart")) {
Sorry, my mistake. I think currently there might be other files than test files
in the directories. The right thing is probably to keep the test for 'Test.dart'
for now.

http://codereview.chromium.org/8492013/diff/3004/tests/corelib/test_config.da...
tests/corelib/test_config.dart:47: "--ignore-unrecognized-flags",
Indentation. I would do:

["--...",
 "--...",
 filename],

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

http://codereview.chromium.org/8492013/diff/3004/tests/standalone/src/TestRun...
tests/standalone/src/TestRunnerTest.dart:47: return new TestCase(testName,
getDartShellFileName(),
I would move argument two to a separate line.

http://codereview.chromium.org/8492013/diff/3004/tests/standalone/src/TestRun...
tests/standalone/src/TestRunnerTest.dart:76: new RunningProcess(new
TestCase("CrashTest", getProcessTestFileName(),
Ditto.

http://codereview.chromium.org/8492013/diff/3004/tests/standalone/test_config...
File tests/standalone/test_config.dart (right):

http://codereview.chromium.org/8492013/diff/3004/tests/standalone/test_config...
tests/standalone/test_config.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
Same comments as other config file.

http://codereview.chromium.org/8492013/diff/3004/tools/testing/dart/test_cont...
File tools/testing/dart/test_controller.dart (right):

http://codereview.chromium.org/8492013/diff/3004/tools/testing/dart/test_cont...
tools/testing/dart/test_controller.dart:1: // Copyright (c) 2011, the Dart
project authors.  Please see the AUTHORS file
Delete file, please.