Fix coverage test

Fix coverage test

Do not rely on order of Future callbacks. There may still be
data on stdout when the process exit event is called (i.e. when
the process Future completes).

R=ricow@google.com

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

[hausner, 2013-06-03 18:53:50]

Thanks for taking a look Rico.

[hausner, 2013-06-03 18:56:42]

Just saw your response. I'll change this if you think it's not good.

[ricow1, 2013-06-03 19:05:57]

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
tests/standalone/coverage_test.dart:26: if (nextLineToMatch <
sourceLines.length) {
don't we want an exact match? what should happen if nextLineToMatch >
sourceLines.length?

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
tests/standalone/coverage_test.dart:35: print("Coverage tool process (pid $pid)
terminated with exit code $exitCode.");
long line and I assume that this is for debugging purposes only?

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
tests/standalone/coverage_test.dart:81:
stderrStringStream.listen(onCoverageOutput);
you can potentially get output on stderr after you have "determined" that you
where successful, i.e., the expected output is on stdout and you got it all, so
checkSuccess is called from the callback above. Now you get the bad output here,
which will not be considered since we just ignore it in onCoverageOutput

[hausner, 2013-06-03 20:48:08]

This version uses the 3 futures solution as you suggested.

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
tests/standalone/coverage_test.dart:26: if (nextLineToMatch <
sourceLines.length) {
On 2013/06/03 19:05:57, ricow1 wrote:
> don't we want an exact match? what should happen if nextLineToMatch >
> sourceLines.length?
A simple empty line from the coverage tool output could accidentally match
an empty line in the source text. I made the match a bit more stringent
and expect an exact match. It's still not completely accurate, but good enough
for what i had in mind. I just want to test that the coverage tool compiles and
runs so it does not bit-rot.

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
tests/standalone/coverage_test.dart:35: print("Coverage tool process (pid $pid)
terminated with exit code $exitCode.");
On 2013/06/03 19:05:57, ricow1 wrote:
> long line and I assume that this is for debugging purposes only?

Yes, for debugging. But it doesn't hurt to leave it, right? Shortened line.

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
tests/standalone/coverage_test.dart:81:
stderrStringStream.listen(onCoverageOutput);
On 2013/06/03 19:05:57, ricow1 wrote:
> you can potentially get output on stderr after you have "determined" that you
> where successful, i.e., the expected output is on stdout and you got it all,
so
> checkSuccess is called from the callback above. Now you get the bad output
here,
> which will not be considered since we just ignore it in onCoverageOutput

Fair point. I changed this to use 3 futures, checking the match after all 3
futures complete.

[ricow1, 2013-06-04 05:49:00]

LGTM

https://codereview.chromium.org/16305013/diff/5002/tests/standalone/coverage_...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/16305013/diff/5002/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:85: checkSuccess();
you don't really use the return values for anything, consider doing something
like:
if (results.contains(false)) .... exit(-1);
(both futures from the StreamSubscriptions will actually throws if something
goes wrong there - but I doubt that will ever happen, so this is actually only
to handle the exitCode != 0 case)

[ricow1, 2013-06-04 05:49:43]

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/16305013/diff/1/tests/standalone/coverage_tes...
tests/standalone/coverage_test.dart:35: print("Coverage tool process (pid $pid)
terminated with exit code $exitCode.");
On 2013/06/03 20:48:08, hausner wrote:
> On 2013/06/03 19:05:57, ricow1 wrote:
> > long line and I assume that this is for debugging purposes only?
> 
> Yes, for debugging. But it doesn't hurt to leave it, right? Shortened line.
Nope, that is perfectly fine, just wanted to make sure :-)

[hausner, 2013-06-04 16:03:24]

Committed patchset #4 manually as r23601 (presubmit successful).

[hausner, 2013-06-04 16:03:41]

Thank you.

https://codereview.chromium.org/16305013/diff/5002/tests/standalone/coverage_...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/16305013/diff/5002/tests/standalone/coverage_...
tests/standalone/coverage_test.dart:85: checkSuccess();
On 2013/06/04 05:49:00, ricow1 wrote:
> you don't really use the return values for anything, consider doing something
> like:
> if (results.contains(false)) .... exit(-1);
> (both futures from the StreamSubscriptions will actually throws if something
> goes wrong there - but I doubt that will ever happen, so this is actually only
> to handle the exitCode != 0 case)

Done.

[kustermann, 2013-06-04 16:21:01]

https://codereview.chromium.org/16305013/diff/10001/tests/standalone/coverage...
File tests/standalone/coverage_test.dart (right):

https://codereview.chromium.org/16305013/diff/10001/tests/standalone/coverage...
tests/standalone/coverage_test.dart:65: Process.start(options.executable,
processOpts).then((Process process) {
DBC: As ricow mentioned on the last review, you could've simplified this a lot,
by just doing something like:

  Process.run(options.executable, processOpts).then((ProcessResult result) {
    if (result.exitCode != 0) {
      throw new Exception("Coverage tool exited with nonzero exit code.");
    }
    verifyOutput(result.stdout.split("\n"));
  });