New testrunner that runs the test pipleine in an isolate.

New testrunner that runs the test pipleine in an isolate.
Committed: https://code.google.com/p/dart/source/detail?r=13155

[Siggi Cherem (dart-lang), 2012-09-28 01:53:51]

this is very cool. I'd love to see how much faster did it become (e.g. 100 tests
in processes vs isolates)?

lgtm after addressing the comments below.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:16: var p = new Path(basis);
fix indentation, hopefully that will make the next line fit in 80 col

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:34: * will be put in a map with key
[procId] and [procId] will be
why not always return the Future? 

If I passed the procId as not-null, I already have it, so I don't need to get it
returned back to me.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:42: completer = new Completer();
nit: un-indent

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:104: return runCommand(command, args, 3000,
'${_procnum++}');
ditto

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:121: if (f.existsSync()) {
ditto

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
File utils/testrunner/run_pipeline.dart (right):

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:6: #library('pipeline');
FYI - you might want to start switching to the new library + import syntax. It
is now supported both in the vm and dart2js.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:54: tempHtmlFile =
createTempName(config["tempdir"], testFile, '.html');
<< fix indent - here and several other places below

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:59: if (config["runtime"] == 'drt-js') {
<<

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:82: notifyDone = (e) => exit(e);
this one surprised me (I was expecting the postMessage also for layout tests...)

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:197: if (window.testRunner) {
should we move this to a .js file that is always in this directory?

[gram, 2012-10-01 21:10:51]

PTAL. Ignore my comments about the whitespace; it turned out it was a tab/space
issue. That's what comes from mixing three editors.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:16: var p = new Path(basis);
On 2012/09/28 01:53:51, sigmund wrote:
> fix indentation, hopefully that will make the next line fit in 80 col

Weird, I don't see this indentation issue, although I do have an overrun on the
next line I will fix.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:34: * will be put in a map with key
[procId] and [procId] will be
On 2012/09/28 01:53:51, sigmund wrote:
> why not always return the Future? 
> 
> If I passed the procId as not-null, I already have it, so I don't need to get
it
> returned back to me.
> 

I see your point, but it is a usage issue. There are two ways this API gets
used, and in one we want a Future because we want to run to completion before
continuing, and in the other we don't. To make that clearer I will make this a
private function and have a public wrapper that calls it for the Future case.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:42: completer = new Completer();
On 2012/09/28 01:53:51, sigmund wrote:
> nit: un-indent

Again, I'm not seeing this indent issue. Very strange. I would have guessed it
was a tab/space issue but that doesn't seem to explain it.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:104: return runCommand(command, args, 3000,
'${_procnum++}');
On 2012/09/28 01:53:51, sigmund wrote:
> ditto

ditto

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:121: if (f.existsSync()) {
On 2012/09/28 01:53:51, sigmund wrote:
> ditto

ditto!

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
File utils/testrunner/run_pipeline.dart (right):

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:6: #library('pipeline');
On 2012/09/28 01:53:51, sigmund wrote:
> FYI - you might want to start switching to the new library + import syntax. It
> is now supported both in the vm and dart2js.

I'd like to punt on that for now and do that when I move this to pub. Partly
because I don't understand the new syntax yet.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:54: tempHtmlFile =
createTempName(config["tempdir"], testFile, '.html');
On 2012/09/28 01:53:51, sigmund wrote:
> << fix indent - here and several other places below

Again, I don't see this.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:59: if (config["runtime"] == 'drt-js') {
On 2012/09/28 01:53:51, sigmund wrote:
> <<

ditto

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:82: notifyDone = (e) => exit(e);
On 2012/09/28 01:53:51, sigmund wrote:
> this one surprised me (I was expecting the postMessage also for layout
tests...)

No, the layout tests are split between a controller running in the VM and a
child test running in DRT. This is the controller's part.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/run_pipeline...
utils/testrunner/run_pipeline.dart:197: if (window.testRunner) {
On 2012/09/28 01:53:51, sigmund wrote:
> should we move this to a .js file that is always in this directory?

There is some string interpolation here so no; we would need several files.

[Siggi Cherem (dart-lang), 2012-10-02 17:45:20]

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:34: * will be put in a map with key
[procId] and [procId] will be
On 2012/10/01 21:10:51, gram wrote:
> On 2012/09/28 01:53:51, sigmund wrote:
> > why not always return the Future? 
> > 
> > If I passed the procId as not-null, I already have it, so I don't need to
get
> it
> > returned back to me.
> > 
> 
> I see your point, but it is a usage issue. There are two ways this API gets
> used, and in one we want a Future because we want to run to completion before
> continuing, and in the other we don't. To make that clearer I will make this a
> private function and have a public wrapper that calls it for the Future case.

I really find it confusing when functions return some union of 2 types. Now that
you have the wrappers why not do move the specialized functionality there?

Future _processHelper(...) {
  // always return the future always
}

Future runCommand(...) {
  return _processHelper(...);
}

String startProcess(...) {
  var pid = '${_procnum++}';
  _processHelper(..., pid);
  // maybe move _procs[id] = process here.
  return pid;
}

At that point, you can probably get rid of _processHelper, since it would the
same that startProcess has:

Future runCommand(...) {
  // ... 
}

String startProcess(...) {
  var pid = '${_procnum++}';
  runCommand(..., pid);
  return pid;
}

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:8: Map _procs = {};
it should be perfectly valid to use the int values as keys in the map:
_procs = new Map<int, Process>();

so you shouldn't need to convert pids into strings.

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:72: StringInputStream stdoutStringStream =
nits (here and below): 
- remove type on the left hand side (according to our style guide)
- this var names might be over-specific, we don't need to know the type on the
variable name, how about: 'outStream' and 'errStream'?

  var outStream = new StringInputStream(process.stdout);
  var errStream = new StringInputStream(process.stdout);
  outStream.onLine = _makeReadHandler(outStream, stdout);
  errStream.onLine = _makeReadHandler(errStream, stderr);

- I just noticed that you are not even using these variables anywhere else, you
could hide this part of the logic under _makeReadHandler. E.g. rename it to
something like 'pipeStream'?

void _pipeStream(IntputStream stream, List<String> destination) {
  var source = new StringInputStream(stream);
  source.onLine = () {
    ...
  };
}

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:74: StringInputStream stderrStringStream =
ditto

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:107: runCommand(String command,
List<String> args, [int timeout = 300]) {
add return type

[gram, 2012-10-02 23:01:59]

PTAL. 

BTW I have noticed an issue which I don't have a fix for yet. It seems that it
is possible that the code to spawn a process and capture the stdout/stderr
streams can drop content. I have a test file with three tests, and if I run that
and include --summarize, sometimes I see all three test results and the summary,
and sometimes I only see two test results. If I run the generated test wrapper
though I always see all the output, so it seems the output is there, but maybe
in a race with the process closing we are losing some content. I tried a couple
of things but nothing has fixed this yet. But that can be handled as a separate
issue.

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/10977069/diff/1/utils/testrunner/pipeline_uti...
utils/testrunner/pipeline_utils.dart:34: * will be put in a map with key
[procId] and [procId] will be
On 2012/10/02 17:45:20, sigmund wrote:
> On 2012/10/01 21:10:51, gram wrote:
> > On 2012/09/28 01:53:51, sigmund wrote:
> > > why not always return the Future? 
> > > 
> > > If I passed the procId as not-null, I already have it, so I don't need to
> get
> > it
> > > returned back to me.
> > > 
> > 
> > I see your point, but it is a usage issue. There are two ways this API gets
> > used, and in one we want a Future because we want to run to completion
before
> > continuing, and in the other we don't. To make that clearer I will make this
a
> > private function and have a public wrapper that calls it for the Future
case.
> 
> I really find it confusing when functions return some union of 2 types. Now
that
> you have the wrappers why not do move the specialized functionality there?
> 
> Future _processHelper(...) {
>   // always return the future always
> }
> 
> Future runCommand(...) {
>   return _processHelper(...);
> }
> 
> String startProcess(...) {
>   var pid = '${_procnum++}';
>   _processHelper(..., pid);
>   // maybe move _procs[id] = process here.
>   return pid;
> }
> 
> At that point, you can probably get rid of _processHelper, since it would the
> same that startProcess has:
> 
> Future runCommand(...) {
>   // ... 
> }
> 
> String startProcess(...) {
>   var pid = '${_procnum++}';
>   runCommand(..., pid);
>   return pid;
> }

That's somewhat like going back to the way I had it, except that instead of
returning the pid we could return a null if there is a pid (we don't want to
return a Future in this case so null seems the best option).

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:8: Map _procs = {};
On 2012/10/02 17:45:20, sigmund wrote:
> it should be perfectly valid to use the int values as keys in the map:
> _procs = new Map<int, Process>();
> 
> so you shouldn't need to convert pids into strings.

Done.

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:72: StringInputStream stdoutStringStream =
On 2012/10/02 17:45:20, sigmund wrote:
> nits (here and below): 
> - remove type on the left hand side (according to our style guide)
> - this var names might be over-specific, we don't need to know the type on the
> variable name, how about: 'outStream' and 'errStream'?
> 
>   var outStream = new StringInputStream(process.stdout);
>   var errStream = new StringInputStream(process.stdout);
>   outStream.onLine = _makeReadHandler(outStream, stdout);
>   errStream.onLine = _makeReadHandler(errStream, stderr);
> 
> - I just noticed that you are not even using these variables anywhere else,
you
> could hide this part of the logic under _makeReadHandler. E.g. rename it to
> something like 'pipeStream'?
> 
> void _pipeStream(IntputStream stream, List<String> destination) {
>   var source = new StringInputStream(stream);
>   source.onLine = () {
>     ...
>   };
> }

Done.

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:74: StringInputStream stderrStringStream =
On 2012/10/02 17:45:20, sigmund wrote:
> ditto

Done.

https://codereview.chromium.org/10977069/diff/12001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:107: runCommand(String command,
List<String> args, [int timeout = 300]) {
On 2012/10/02 17:45:20, sigmund wrote:
> add return type

Done.

[Siggi Cherem (dart-lang), 2012-10-02 23:42:55]

lgtm

[Siggi Cherem (dart-lang), 2012-10-02 23:43:56]

https://codereview.chromium.org/10977069/diff/20001/utils/testrunner/pipeline...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/10977069/diff/20001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:38: * passing in the [procId].
we should update the comment though...