Update the test runner to use the new dart:io API

Update the test runner to use the new dart:io API

The new binaries are from r19850.

R=whesse@google.com, ager@google.com, ricow@google.com, kustermann@google.com

BUG=

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

[Mads Ager (google), 2013-03-13 11:28:55]

LGTM

https://codereview.chromium.org/12417004/diff/3001/tests/standalone/standalon...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/12417004/diff/3001/tests/standalone/standalon...
tests/standalone/standalone.status:10: io/skipping_dart2js_compilations_test:
skip # Non-fatal process error.
Shouldn't we get this one going as well. This is a test of the test scripts I
believe.

[Bill Hesse, 2013-03-13 12:56:41]

LGTM.

The batch test runner had very tricky behavior, because the _process member and
the _testCase member were updated or set to null at different points, and the
exit handler, output streams, and ignore_streams were synchronized with that. 
But the new changes seem to be right, and to make it simpler.

https://codereview.chromium.org/12417004/diff/3001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/test.dart#newcode224
tools/test.dart:224: server.startServers('127.0.0.1').then((_) {
Could this take long enough that we would prefer to start them in parallel, and
use Future.wait?

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/http_se...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:132: (HttpRequest request) {
Doesn't indentation look nicer (smaller) if we put the argument on the same line
as .listen, and use 2 space indent for the function body?

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:317: DebugLogger.warning('HttpServer: error
while closing the response '
Put the entire string on the next line?

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/multite...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:221: var bytes = tests[key].codeUnits;
.codeUnits returns 16-bit code units.  Don't we need to write this with an
encoding?

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:222: openedFile.writeListSync(bytes, 0,
bytes.length);
writeStringSync?

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_pr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_pr...
tools/testing/dart/test_progress.dart:290: var tempDirs = [];
Why not just use a counter instead of a list for tempDirs?
(Can be left as is, though).

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_ru...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:679: var bytesContentLength =
"Content-Length:".codeUnits;
Misleading?  But I see there is no easy way of getting a list of bytes from a
string.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1053: DateTime _startTime;
Did we just remove the only use of the MutableValue class?  If so, remove the
class.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1236: _processExitHandler(exitCode);
I'm not sure that _processExitHandler will work correctly, since I think we will
start a new process, and set _processExitHandler for that process, before the
previous process has exited and called the old _processExitHandler.

It worked before because the handler was on the process object, not on the
BatchRunner instance.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:51: Future asynchronously(function()) {
Shouldn't this be replaced at its call sites with new Future.delayed(new
Duration(seconds: 0), ___)?

Or at least its body should be that?

[kustermann, 2013-03-13 13:37:56]

lgtm

https://codereview.chromium.org/12417004/diff/1/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/test.dart#newcode119
tools/test.dart:119: List<TestingServers> servers = [];
Why not make it a "List<Future> serverFutures", start them here (as we did
before) and do "Future.wait(serverFutures).then((_) => startProcessQueue)" at
the bottom of the file instead of the recursive call of "startTesting".

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/http_serve...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/http_serve...
tools/testing/dart/http_server.dart:318: 'stream: $e');
Indentation.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/multitest....
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/multitest....
tools/testing/dart/multitest.dart:119: // Add the template, with no
mulpedBytitest lines, as a test with key 'none'.
What is this?

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/multitest....
tools/testing/dart/multitest.dart:155: // Matches #import( or #source(
pedByfollowed by " or ' followed by anything
What is this?

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_runne...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:1029: class MutableValue<T> {
Looks like this class is no longer used.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:1041: Stream<String> _stderrStream;
I think we don't need these instance variables anymore.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_suite...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_suite...
tools/testing/dart/test_suite.dart:55: new Timer(new Duration(seconds: 0), () =>
completer.complete(function()));
Why not use 'Timer.run' (is there a difference to 'new Timer(0, ..)'?

[ricow1, 2013-03-13 13:38:41]

LGTM

https://codereview.chromium.org/12417004/diff/3001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/test.dart#newcode119
tools/test.dart:119: List<TestingServers> servers = [];
how do we make sure that these are shut down?

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_pr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_pr...
tools/testing/dart/test_progress.dart:298: DebugLogger.warning("There are
${tempDirs.length} directories "
indentation

[Søren Gjesse, 2013-03-13 15:17:18]

Thanks for the review.

I will try a commit then the tree is green and quiet.

https://codereview.chromium.org/12417004/diff/1/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/test.dart#newcode119
tools/test.dart:119: List<TestingServers> servers = [];
On 2013/03/13 13:37:56, kustermann wrote:
> Why not make it a "List<Future> serverFutures", start them here (as we did
> before) and do "Future.wait(serverFutures).then((_) => startProcessQueue)" at
> the bottom of the file instead of the recursive call of "startTesting".

Done.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/http_serve...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/http_serve...
tools/testing/dart/http_server.dart:318: 'stream: $e');
On 2013/03/13 13:37:56, kustermann wrote:
> Indentation.

Done.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/multitest....
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/multitest....
tools/testing/dart/multitest.dart:119: // Add the template, with no
mulpedBytitest lines, as a test with key 'none'.
On 2013/03/13 13:37:56, kustermann wrote:
> What is this?

Done.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/multitest....
tools/testing/dart/multitest.dart:155: // Matches #import( or #source(
pedByfollowed by " or ' followed by anything
On 2013/03/13 13:37:56, kustermann wrote:
> What is this?

Done.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_runne...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:1029: class MutableValue<T> {
On 2013/03/13 13:37:56, kustermann wrote:
> Looks like this class is no longer used.

Done.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_runne...
tools/testing/dart/test_runner.dart:1041: Stream<String> _stderrStream;
On 2013/03/13 13:37:56, kustermann wrote:
> I think we don't need these instance variables anymore.

Done.

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_suite...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/test_suite...
tools/testing/dart/test_suite.dart:55: new Timer(new Duration(seconds: 0), () =>
completer.complete(function()));
On 2013/03/13 13:37:56, kustermann wrote:
> Why not use 'Timer.run' (is there a difference to 'new Timer(0, ..)'?

Done.

https://codereview.chromium.org/12417004/diff/3001/tests/standalone/standalon...
File tests/standalone/standalone.status (right):

https://codereview.chromium.org/12417004/diff/3001/tests/standalone/standalon...
tests/standalone/standalone.status:10: io/skipping_dart2js_compilations_test:
skip # Non-fatal process error.
On 2013/03/13 11:28:55, Mads Ager wrote:
> Shouldn't we get this one going as well. This is a test of the test scripts I
> believe.

Forgot to remove that - it passes.

https://codereview.chromium.org/12417004/diff/3001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/test.dart#newcode119
tools/test.dart:119: List<TestingServers> servers = [];
On 2013/03/13 13:38:41, ricow1 wrote:
> how do we make sure that these are shut down?

It is handled in allTestsFinished().

https://codereview.chromium.org/12417004/diff/3001/tools/test.dart#newcode224
tools/test.dart:224: server.startServers('127.0.0.1').then((_) {
On 2013/03/13 12:56:41, Bill Hesse wrote:
> Could this take long enough that we would prefer to start them in parallel,
and
> use Future.wait?

Changed to what kustermann@ suggested.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/http_se...
File tools/testing/dart/http_server.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:132: (HttpRequest request) {
On 2013/03/13 12:56:41, Bill Hesse wrote:
> Doesn't indentation look nicer (smaller) if we put the argument on the same
line
> as .listen, and use 2 space indent for the function body?

Done.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/http_se...
tools/testing/dart/http_server.dart:317: DebugLogger.warning('HttpServer: error
while closing the response '
On 2013/03/13 12:56:41, Bill Hesse wrote:
> Put the entire string on the next line?

Done.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/multite...
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:221: var bytes = tests[key].codeUnits;
On 2013/03/13 12:56:41, Bill Hesse wrote:
> .codeUnits returns 16-bit code units.  Don't we need to write this with an
> encoding?

Used writeStringSync as suggested below. Then reading and writing should default
to UTF-8 encoding.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/multite...
tools/testing/dart/multitest.dart:222: openedFile.writeListSync(bytes, 0,
bytes.length);
On 2013/03/13 12:56:41, Bill Hesse wrote:
> writeStringSync?

Done.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_pr...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_pr...
tools/testing/dart/test_progress.dart:290: var tempDirs = [];
On 2013/03/13 12:56:41, Bill Hesse wrote:
> Why not just use a counter instead of a list for tempDirs?
> (Can be left as is, though).

Done.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_pr...
tools/testing/dart/test_progress.dart:298: DebugLogger.warning("There are
${tempDirs.length} directories "
On 2013/03/13 13:38:41, ricow1 wrote:
> indentation

Done.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_ru...
File tools/testing/dart/test_runner.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:679: var bytesContentLength =
"Content-Length:".codeUnits;
On 2013/03/13 12:56:41, Bill Hesse wrote:
> Misleading?  But I see there is no easy way of getting a list of bytes from a
> string.

As long as we have ASCII only this should be fine.

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1053: DateTime _startTime;
On 2013/03/13 12:56:41, Bill Hesse wrote:
> Did we just remove the only use of the MutableValue class?  If so, remove the
> class.

MutableValue removed

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_ru...
tools/testing/dart/test_runner.dart:1236: _processExitHandler(exitCode);
On 2013/03/13 12:56:41, Bill Hesse wrote:
> I'm not sure that _processExitHandler will work correctly, since I think we
will
> start a new process, and set _processExitHandler for that process, before the
> previous process has exited and called the old _processExitHandler.
> 
> It worked before because the handler was on the process object, not on the
> BatchRunner instance.

I don't think that is the case. We are only starting a new process through a
call to _processExitHandler (except for the first process).

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_su...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/12417004/diff/3001/tools/testing/dart/test_su...
tools/testing/dart/test_suite.dart:51: Future asynchronously(function()) {
On 2013/03/13 12:56:41, Bill Hesse wrote:
> Shouldn't this be replaced at its call sites with new Future.delayed(new
> Duration(seconds: 0), ___)?
> 
> Or at least its body should be that?

It was changed to use Timer.run.

[Bill Hesse, 2013-03-13 16:07:00]

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/multitest....
File tools/testing/dart/multitest.dart (right):

https://codereview.chromium.org/12417004/diff/1/tools/testing/dart/multitest....
tools/testing/dart/multitest.dart:155: // Matches #import( or #source(
pedByfollowed by " or ' followed by anything
On 2013/03/13 13:37:56, kustermann wrote:
> What is this?

This must be stray changes when someone converted .map to .mappedBy, using a
regexp.

[Søren Gjesse, 2013-03-14 07:54:38]

Committed patchset #5 manually as r19987 (presubmit successful).