Updated testrunner:

Updated testrunner:

- works with latest SDK
- understand and works with pub and pubspecs
- tested on Linux, Mac and Windows
- has its own tests

There is still as issue on Windows, where using Process.start to run a .bat file is not terminating until the timeout kills the cmd.exe process. My suspicion is that this is am issue with Process, not testrunner. I have contacted whesse et al about this.

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

[Siggi Cherem (dart-lang), 2013-04-19 21:39:39]

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/layout_te...
File utils/testrunner/layout_test_controller.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/layout_te...
utils/testrunner/layout_test_controller.dart:157: completer.complete(exitCode);
can we get rid of using completer?

I think it should be possible to return exitCode here and refactor things a bit:

_processHelper(...) {
...

return processFuture.then((process) {
  timer = ...;
  return process.exitCode;
}).then((exitCode) {
  if (timer != null) { ... }
  if (raw) ...
  return exitCode;
});

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/layout_te...
utils/testrunner/layout_test_controller.dart:172: return completer.future;
wouldn't this crash when procId != 0 (since completer is null)?

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pipeline_...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pipeline_...
utils/testrunner/pipeline_utils.dart:59: Future _processHelper(String command,
List<String> args,
is this duplicate code with the other _processHelper? If not, can we fix this
one also to avoid using a completer?

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pubspec.yaml
File utils/testrunner/pubspec.yaml (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pubspec.y...
utils/testrunner/pubspec.yaml:4: unittest: any
do you need unittest for testing or for the actual testrunner? if it's only for
testing, consider making this a dev_dependency

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
File utils/testrunner/testrunner.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:163: Future doPubConfig(Path sourcePath, String
sourceDir,
why do we need to run pub? 

when users run it, pub already had created the packages/ directory within
'test/'

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/utils.dart
File utils/testrunner/utils.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/utils.dar...
utils/testrunner/utils.dart:107: var completer = new Completer();
likewise, we can get rid of this completer too.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/utils.dar...
utils/testrunner/utils.dart:108: var timer = null;
unused

[Siggi Cherem (dart-lang), 2013-04-19 21:48:38]

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
File utils/testrunner/testrunner.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:163: Future doPubConfig(Path sourcePath, String
sourceDir,
On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> why do we need to run pub? 
> 
> when users run it, pub already had created the packages/ directory within
> 'test/'

after our discussion, I now understand that you are running the code in a tmp
directory where the packages symlinks don't exist.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:175: var sourcePubSpecName = new
Path(sourceDir).append("pubspec.yaml").
one option here is that you could simply simlink the packages/ folder on the
temp directory to the packages/ folder of the test/ directory. Then everything
should "just work".

Alternatively, if you want to run pub, to make sure that pub installs the same
thing, make sure you copy over 'pubspec.lock'. That file should have the exact
versions that the user has been developing with. 

Given that they depend on testrunner in pub, pubspec.lock will include all the
transitive dependencies you need (including unittest).

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:183: if (targetPubSpec.existsSync()) {
is this branch possible? (won't they use pub to get testrunner in the first
place?)

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:192: "name: testrunner\ndependencies:\n 
unittest: any\n  browser: any\n");
make sure you include 'browser' as a testrunner dependency too (consider adding
a comment on your pubspec.yaml to explain why).

[gram, 2013-04-22 23:54:27]

PTAL

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/layout_te...
File utils/testrunner/layout_test_controller.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/layout_te...
utils/testrunner/layout_test_controller.dart:157: completer.complete(exitCode);
On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> can we get rid of using completer?
> 
> I think it should be possible to return exitCode here and refactor things a
bit:
> 
> _processHelper(...) {
> ...
> 
> return processFuture.then((process) {
>   timer = ...;
>   return process.exitCode;
> }).then((exitCode) {
>   if (timer != null) { ... }
>   if (raw) ...
>   return exitCode;
> });

Done.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/layout_te...
utils/testrunner/layout_test_controller.dart:172: return completer.future;
On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> wouldn't this crash when procId != 0 (since completer is null)?

Yes - but thankfully there is no completer now :-)

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pipeline_...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pipeline_...
utils/testrunner/pipeline_utils.dart:59: Future _processHelper(String command,
List<String> args,
On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> is this duplicate code with the other _processHelper? If not, can we fix this
> one also to avoid using a completer?

No, it is not a dup; it has some subtle differences. And in this case I think we
need the completer as we are not actually complete until the streams have
drained as well as the process exiting.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pubspec.yaml
File utils/testrunner/pubspec.yaml (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pubspec.y...
utils/testrunner/pubspec.yaml:4: unittest: any
On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> do you need unittest for testing or for the actual testrunner? if it's only
for
> testing, consider making this a dev_dependency

Removed.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
File utils/testrunner/testrunner.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:163: Future doPubConfig(Path sourcePath, String
sourceDir,
On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> why do we need to run pub? 
> 
> when users run it, pub already had created the packages/ directory within
> 'test/'

Done.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:163: Future doPubConfig(Path sourcePath, String
sourceDir,
On 2013/04/19 21:48:38, Siggi Cherem (dart-lang) wrote:
> On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> > why do we need to run pub? 
> > 
> > when users run it, pub already had created the packages/ directory within
> > 'test/'
> 
> after our discussion, I now understand that you are running the code in a tmp
> directory where the packages symlinks don't exist.

Done.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:175: var sourcePubSpecName = new
Path(sourceDir).append("pubspec.yaml").
On 2013/04/19 21:48:38, Siggi Cherem (dart-lang) wrote:
> one option here is that you could simply simlink the packages/ folder on the
> temp directory to the packages/ folder of the test/ directory. Then everything
> should "just work".
> 
> Alternatively, if you want to run pub, to make sure that pub installs the same
> thing, make sure you copy over 'pubspec.lock'. That file should have the exact
> versions that the user has been developing with. 
> 
> Given that they depend on testrunner in pub, pubspec.lock will include all the
> transitive dependencies you need (including unittest).

Done.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:183: if (targetPubSpec.existsSync()) {
On 2013/04/19 21:48:38, Siggi Cherem (dart-lang) wrote:
> is this branch possible? (won't they use pub to get testrunner in the first
> place?)

It's possible if there is no source pub, but they have runtestrunner before, and
it created a pubspec in the temp directory.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:192: "name: testrunner\ndependencies:\n 
unittest: any\n  browser: any\n");
On 2013/04/19 21:48:38, Siggi Cherem (dart-lang) wrote:
> make sure you include 'browser' as a testrunner dependency too (consider
adding
> a comment on your pubspec.yaml to explain why).

Browser is already there at the end of the line :-)

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/utils.dart
File utils/testrunner/utils.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/utils.dar...
utils/testrunner/utils.dart:107: var completer = new Completer();
On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> likewise, we can get rid of this completer too. 

Done.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/utils.dar...
utils/testrunner/utils.dart:108: var timer = null;
On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> unused

Done.

[Siggi Cherem (dart-lang), 2013-04-23 01:10:04]

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pipeline_...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pipeline_...
utils/testrunner/pipeline_utils.dart:59: Future _processHelper(String command,
List<String> args,
On 2013/04/22 23:54:27, gram wrote:
> On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> > is this duplicate code with the other _processHelper? If not, can we fix
this
> > one also to avoid using a completer?
> 
> No, it is not a dup; it has some subtle differences. And in this case I think
we
> need the completer as we are not actually complete until the streams have
> drained as well as the process exiting.
> 

I think you can use Future.wait instead, using stream.asFuture() as a future
that completes when the pipe has drained as well. Otherwise, you'll need to
propagate errors through the completer.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
File utils/testrunner/testrunner.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:192: "name: testrunner\ndependencies:\n 
unittest: any\n  browser: any\n");
On 2013/04/22 23:54:27, gram wrote:
> On 2013/04/19 21:48:38, Siggi Cherem (dart-lang) wrote:
> > make sure you include 'browser' as a testrunner dependency too (consider
> adding
> > a comment on your pubspec.yaml to explain why).
> 
> Browser is already there at the end of the line :-) 

Sorry, what I trying to say was that if you use the configuration from the user
(sourcePubSpec.existsSync()), then you should ensure that their pubspec.lock
file includes both unittest and browser. One way to guarantee that is to include
them in testrunner's pubspec file so they get pulled transitively.

https://codereview.chromium.org/14247033/diff/11001/utils/testrunner/utils.dart
File utils/testrunner/utils.dart (right):

https://codereview.chromium.org/14247033/diff/11001/utils/testrunner/utils.da...
utils/testrunner/utils.dart:113: .then((result) {
nits:
- indent +4
- use 1-line callback when it fits:

    .then((res) => res.exitCode))
    .catchError((e) { });

https://codereview.chromium.org/14247033/diff/11001/utils/testrunner/utils.da...
utils/testrunner/utils.dart:116: .catchError((e) {
do we really want to swallow errors?

[gram, 2013-04-23 22:53:39]

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pipeline_...
File utils/testrunner/pipeline_utils.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/pipeline_...
utils/testrunner/pipeline_utils.dart:59: Future _processHelper(String command,
List<String> args,
On 2013/04/23 01:10:04, Siggi Cherem (dart-lang) wrote:
> On 2013/04/22 23:54:27, gram wrote:
> > On 2013/04/19 21:39:39, Siggi Cherem (dart-lang) wrote:
> > > is this duplicate code with the other _processHelper? If not, can we fix
> this
> > > one also to avoid using a completer?
> > 
> > No, it is not a dup; it has some subtle differences. And in this case I
think
> we
> > need the completer as we are not actually complete until the streams have
> > drained as well as the process exiting.
> > 
> 
> I think you can use Future.wait instead, using stream.asFuture() as a future
> that completes when the pipe has drained as well. Otherwise, you'll need to
> propagate errors through the completer. 

Done.

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
File utils/testrunner/testrunner.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:192: "name: testrunner\ndependencies:\n 
unittest: any\n  browser: any\n");
On 2013/04/23 01:10:04, Siggi Cherem (dart-lang) wrote:
> On 2013/04/22 23:54:27, gram wrote:
> > On 2013/04/19 21:48:38, Siggi Cherem (dart-lang) wrote:
> > > make sure you include 'browser' as a testrunner dependency too (consider
> > adding
> > > a comment on your pubspec.yaml to explain why).
> > 
> > Browser is already there at the end of the line :-) 
> 
> Sorry, what I trying to say was that if you use the configuration from the
user
> (sourcePubSpec.existsSync()), then you should ensure that their pubspec.lock
> file includes both unittest and browser. One way to guarantee that is to
include
> them in testrunner's pubspec file so they get pulled transitively.

The tests are run directly; testrunner just generates the wrappers and then
spawns VM or DRT instances to run the tests, so I don't think transitive
inclusion would work, and I am reluctant to poke into the contents of the user's
pubspec file. The tests would presumably fail if the user had a pubspec that
didn't include the dependencies, so wouldn't that be sufficient?

https://codereview.chromium.org/14247033/diff/11001/utils/testrunner/utils.dart
File utils/testrunner/utils.dart (right):

https://codereview.chromium.org/14247033/diff/11001/utils/testrunner/utils.da...
utils/testrunner/utils.dart:113: .then((result) {
On 2013/04/23 01:10:04, Siggi Cherem (dart-lang) wrote:
> nits:
> - indent +4
> - use 1-line callback when it fits:
> 
>     .then((res) => res.exitCode))
>     .catchError((e) { });

Done.

https://codereview.chromium.org/14247033/diff/11001/utils/testrunner/utils.da...
utils/testrunner/utils.dart:116: .catchError((e) {
On 2013/04/23 01:10:04, Siggi Cherem (dart-lang) wrote:
> do we really want to swallow errors?

I changed it to print them out. We only use this for running pub install.

[Siggi Cherem (dart-lang), 2013-04-24 16:34:15]

lgtm

Minor nits below

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
File utils/testrunner/testrunner.dart (right):

https://codereview.chromium.org/14247033/diff/2001/utils/testrunner/testrunne...
utils/testrunner/testrunner.dart:192: "name: testrunner\ndependencies:\n 
unittest: any\n  browser: any\n");
On 2013/04/23 22:53:40, gram wrote:
> On 2013/04/23 01:10:04, Siggi Cherem (dart-lang) wrote:
> > On 2013/04/22 23:54:27, gram wrote:
> > > On 2013/04/19 21:48:38, Siggi Cherem (dart-lang) wrote:
> > > > make sure you include 'browser' as a testrunner dependency too (consider
> > > adding
> > > > a comment on your pubspec.yaml to explain why).
> > > 
> > > Browser is already there at the end of the line :-) 
> > 
> > Sorry, what I trying to say was that if you use the configuration from the
> user
> > (sourcePubSpec.existsSync()), then you should ensure that their pubspec.lock
> > file includes both unittest and browser. One way to guarantee that is to
> include
> > them in testrunner's pubspec file so they get pulled transitively.
> 
> The tests are run directly; testrunner just generates the wrappers and then
> spawns VM or DRT instances to run the tests, so I don't think transitive
> inclusion would work, and I am reluctant to poke into the contents of the
user's
> pubspec file. The tests would presumably fail if the user had a pubspec that
> didn't include the dependencies, so wouldn't that be sufficient?
> 

Just to summarize our offline discussion - users do not depend on testrunner on
their pubspec, so we prefer that they depend directly on unittest and browser if
they need them for their test. 

Let's add a TODO indicating that we should inspect the pubspec.lock file and
give useful errors to users:
- if the lock file doesn't exit, then run pub install
- if it exists and it doesn't have the required packages (unittest or browser)
ask them to add them and run pub install again.

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

https://codereview.chromium.org/14247033/diff/26001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:85: return Future.wait( [process.exitCode,
stdoutFuture, stderrFuture ])
style nit: fix spaces - no space after '(', no space before ']':

wait([process.exitCode, stdoutFuture, stderrFuture])

https://codereview.chromium.org/14247033/diff/26001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:86: .then((l) {
nit: "l" => "values"

https://codereview.chromium.org/14247033/diff/26001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:90: return new Future.value(l[0]);
you can remove the 'new Future' here.

return values[0];

[gram, 2013-04-24 18:40:02]

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

[gram, 2013-04-24 18:42:06]

I should have updated the change description; the Windows .bat file issue was
resolved.

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

https://codereview.chromium.org/14247033/diff/26001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:85: return Future.wait( [process.exitCode,
stdoutFuture, stderrFuture ])
On 2013/04/24 16:34:15, Siggi Cherem (dart-lang) wrote:
> style nit: fix spaces - no space after '(', no space before ']':
> 
> wait([process.exitCode, stdoutFuture, stderrFuture])
> 

Done.

https://codereview.chromium.org/14247033/diff/26001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:86: .then((l) {
On 2013/04/24 16:34:15, Siggi Cherem (dart-lang) wrote:
> nit: "l" => "values"

Done.

https://codereview.chromium.org/14247033/diff/26001/utils/testrunner/pipeline...
utils/testrunner/pipeline_utils.dart:90: return new Future.value(l[0]);
On 2013/04/24 16:34:15, Siggi Cherem (dart-lang) wrote:
> you can remove the 'new Future' here.
> 
> return values[0];

Done.