test.dart sometimes associates the wrong stderr with a task.

test.dart sometimes associates the wrong stderr with a task.

Problem is due to stdout status being processed before
all of stderr is read.

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

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

[Siggi Cherem (dart-lang), 2012-02-29 19:13:32]

lgtm with my partial understanding of this code. I'm adding Emily and John in
case they have more insight since they've been doing some additional work on
test.dart to run the browser tests.

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

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:488: _process.stderr.readInto(buf, 0,
available);;
extra ;

[Emily Fortuna, 2012-02-29 19:59:03]

Just a few comments -- the Stream APIs are a bit confusing in their current
state. :-)

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

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:469: // TODO(zundel): This is pretty bad -
streams need flush()
Add issue id # (1407) in comment
(also possibly let sgjesse (owner of 1407) know about this in test dart -- it
may raise the priority of flush getting implemented in streams sooner)

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:485: result = _stderrStream.read();
Perhaps I'm misunderstanding the API, but would the case actually occur that
_stderrStream.read() does not return null but there are still more lines to be
read using readLine or read? Shouldn't the call to read() be the end?

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:488: _process.stderr.readInto(buf, 0,
available);;
_stderrStream was instantiated from new StringInputStream(_process.stderr), so
shouldn't _process.stderr be empty already?

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:541: return () {
Check out makeReadHandler on line 334. 
whesse also has the line: 
if (source.closed) return;  // TODO(whesse): Remove when bug is fixed.
I don't know what the particular bug is, but you may want to have a similar test
in this function, too. Or, better yet, share code between makeReadHandler and
this method (and maybe _readStdout).

[zundel, 2012-02-29 20:19:08]

Thank you for your feedback Siggi and Emily.

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

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:469: // TODO(zundel): This is pretty bad -
streams need flush()
On 2012/02/29 19:59:03, Emily Fortuna wrote:
> Add issue id # (1407) in comment
> (also possibly let sgjesse (owner of 1407) know about this in test dart -- it
> may raise the priority of flush getting implemented in streams sooner)

Done.

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:485: result = _stderrStream.read();
On 2012/02/29 19:59:03, Emily Fortuna wrote:
> Perhaps I'm misunderstanding the API, but would the case actually occur that
> _stderrStream.read() does not return null but there are still more lines to be
> read using readLine or read? Shouldn't the call to read() be the end?

(see next comment).

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:488: _process.stderr.readInto(buf, 0,
available);;
On 2012/02/29 19:59:03, Emily Fortuna wrote:
> _stderrStream was instantiated from new StringInputStream(_process.stderr), so
> shouldn't _process.stderr be empty already?

I didn't know about 1407 and filed this bug to try to explain why I felt the
need to work around this way.

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

You'd think you could never get to this point, but it remains that
_process.stderr.available() returns a positive number after the call to both
read() and readLine().  In fact, a call to StringInputStream.read() or
.readLine() does not actually read from the underlying stream at all.  It just
delivers up data that's already be read in and processes it through the decoder
to turn it into a string.

Its great that this means there's no chance this call will block, but then your
call to read() is possibly leaving unread data waiting to be read.  Seems like a
possible inefficiency.

https://chromiumcodereview.appspot.com/9538009/diff/1003/tools/testing/dart/t...
tools/testing/dart/test_runner.dart:541: return () {
On 2012/02/29 19:59:03, Emily Fortuna wrote:
> Check out makeReadHandler on line 334. 
> whesse also has the line: 
> if (source.closed) return;  // TODO(whesse): Remove when bug is fixed.
> I don't know what the particular bug is, but you may want to have a similar
test
> in this function, too. Or, better yet, share code between makeReadHandler and
> this method (and maybe _readStdout).

I just got rid of _readStdout() and replaced it with _makeReadHandler() and put
the  if stream.closed() logic
in _readStdout()

[Emily Fortuna, 2012-02-29 20:23:41]

lgtm! And hopefully we get these APIs fixed soon!

[Mads Ager (google), 2012-03-01 07:02:59]

I don't understand this change. This should not be needed. All of the output
should become available for reading with the normal streaming behavior on
stderr. Can't you put an end token in the stderr in the same way there is an end
token in stdout. That way you can use the normal streaming API until you have
received both and only then signal that you are ready for another test?

Having a flush method will not work. stderr is an InputStream. You cannot flush
input.