Always fail if a process failed to launch.

Always fail if a process failed to launch.

This reveals a problem with the implementation of extra commands, which is also fixed.

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

[Bill Hesse, 2012-09-26 22:45:07]

LGTM.

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

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_r...
dart/tools/testing/dart/test_runner.dart:634: testComplete(-1, true);
Doesn't testComplete take a single argument, exitCode?  It looks like all other
instance fields used by testComplete have been initialized at this point, so
this should work.

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_r...
dart/tools/testing/dart/test_runner.dart:892: // TODO(ahe): How to report this
as a test failure?
I think
_stdoutStream = null;
_stderrStream = null;
_process = null;
_status = ">> TEST CRASH";
_testStderr = [];
_testStdout = [];
_startTime = new Date.now();
_reportResult();

should work, reporting a crash of the test, even though this was a crash of the
batch runner, even before a test was sent to it.  The batch runner should be
restarted by the next test sent to it by .startTest being run on it, triggered
by test.completed() being run by _reportResult().  This is different than what
happens on _process.onExit, but it shouldn't result in the permanent loss of a
runner from the pool (but we should check).

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_r...
dart/tools/testing/dart/test_runner.dart:1046: exit(1);
I don't know as much about this case.  I'm surprised there is no p.onExit()
handler registered.

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_s...
File dart/tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_s...
dart/tools/testing/dart/test_suite.dart:512: void addExtraCommand(String
command, List<String> arguments, List commands) {
This functionality just seems like a magnet for additional hacks and special
cases to me.  We should consider eliminating it in the future, rather than
letting it become more generally used.

[ahe, 2012-09-27 07:29:48]

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

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_r...
dart/tools/testing/dart/test_runner.dart:634: testComplete(-1, true);
On 2012/09/26 22:45:07, Bill Hesse wrote:
> Doesn't testComplete take a single argument, exitCode?  It looks like all
other
> instance fields used by testComplete have been initialized at this point, so
> this should work.

Good catch. This is from another CL I'm working on.

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_r...
dart/tools/testing/dart/test_runner.dart:892: // TODO(ahe): How to report this
as a test failure?
On 2012/09/26 22:45:07, Bill Hesse wrote:
> I think
> _stdoutStream = null;
> _stderrStream = null;
> _process = null;
> _status = ">> TEST CRASH";
> _testStderr = [];
> _testStdout = [];
> _startTime = new Date.now();
> _reportResult();
> 
> should work, reporting a crash of the test, even though this was a crash of
the
> batch runner, even before a test was sent to it.  The batch runner should be
> restarted by the next test sent to it by .startTest being run on it, triggered
> by test.completed() being run by _reportResult().  This is different than what
> happens on _process.onExit, but it shouldn't result in the permanent loss of a
> runner from the pool (but we should check).

Actually, I think chances are that this will happen for each test run. So I'll
update the comment to explain why we're exiting at this point.

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_s...
File dart/tools/testing/dart/test_suite.dart (right):

http://codereview.chromium.org/10993048/diff/1/dart/tools/testing/dart/test_s...
dart/tools/testing/dart/test_suite.dart:512: void addExtraCommand(String
command, List<String> arguments, List commands) {
On 2012/09/26 22:45:07, Bill Hesse wrote:
> This functionality just seems like a magnet for additional hacks and special
> cases to me.  We should consider eliminating it in the future, rather than
> letting it become more generally used.

I agree, but that just because I don't like adding complexity. As far as I'm
concerned, the root cause of this problem was not the ExtraCommand feature, but
poor error handling when running processes.

By itself, addExtraCommand is reasonable. I think the problem it is being used
in an extremely brittle way. A web server is started listening on port 8000. I'm
sorry, I'm already using this port.

Furthermore, it is a launching a process using "nohup" and "&" that simply
doesn't work on Windows.

If an error occurs, the webserver will be left running for 60 seconds. This
webserver will also prevent you from testing multiple configurations on the same
machine.

As far as I'm concerned, pkg/intl/test/start_web_server.dart should not use
"nohup" and "&". Instead the ExtraCommand feature should include a tag that
tells test_suite that it is a background process (and that it must be terminated
when the test is complete—also it is an error if the process didn't die by
itself).

Unfortunately, this doesn't solve the port number issue. That should also be
solved, but it is a bit more involved than what is reasonable to include here.

[ahe, 2012-09-27 07:39:11]

I have filed http://dartbug.com/5512.

[Alan Knight, 2012-09-27 19:44:12]

On 2012/09/27 07:39:11, ahe wrote:
> I have filed http://dartbug.com/5512.

I am out of office until Monday, so if this is time-sensitive, don't wait on me
for a review.
 
Alan

[ahe, 2012-09-27 19:52:52]

On 2012/09/27 19:44:12, Alan Knight wrote:
> On 2012/09/27 07:39:11, ahe wrote:
> > I have filed http://dartbug.com/5512.
> 
> I am out of office until Monday, so if this is time-sensitive, don't wait on
me
> for a review.
>  
> Alan

No problem, I already submitted this CL. It was time sensitive as we saw that
failures on the dart2js/d8 build bot went unreported.