Added support for running dart2js tests on android devices

Added support for running dart2js tests on android devices

R=ricow@google.com

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

[ricow1, 2013-05-21 18:30:43]

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
File tools/testing/dart/android.dart (right):

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:40: buf.write(data);
indentation seems off here, hard to follow flow

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:59: stdout: \n ${results[0]}
this may look sort of strange due to the whitespace being added here

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:65: return [results[0], results[1]];
how about a ProcessResult like class for returning this?

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:117: AndroidEmulator(this._port,
this._adbDevice, this._emulatorProcess) {
since you are using private methods and instance varibles you could make this
private as well

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:158: return _executeCommand("android", args,
"\n\n\n\n");
add a comment stating why we add 4 new lines to stdin?

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:307: return
_deviceLineRegexp.allMatches(result.stdout).map((Match m) =>
m.group(1)).toList();
long line

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/test_op...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/test_op...
tools/testing/dart/test_options.dart:320: '127.0.0.1'),];
you actually default to 0.0.0.0 in the http server

[kustermann, 2013-05-22 09:30:58]

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
File tools/testing/dart/android.dart (right):

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:40: buf.write(data);
On 2013/05/21 18:30:43, ricow1 wrote:
> indentation seems off here, hard to follow flow

Done.

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:59: stdout: \n ${results[0]}
On 2013/05/21 18:30:43, ricow1 wrote:
> this may look sort of strange due to the whitespace being added here

Done.

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:65: return [results[0], results[1]];
On 2013/05/21 18:30:43, ricow1 wrote:
> how about a ProcessResult like class for returning this?
I think there is no public ProcessResult constructor:
http://api.dartlang.org/docs/releases/latest/dart_io/ProcessResult.html

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:117: AndroidEmulator(this._port,
this._adbDevice, this._emulatorProcess) {
On 2013/05/21 18:30:43, ricow1 wrote:
> since you are using private methods and instance varibles you could make this
> private as well

Done.

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:158: return _executeCommand("android", args,
"\n\n\n\n");
On 2013/05/21 18:30:43, ricow1 wrote:
> add a comment stating why we add 4 new lines to stdin?

Done. I don't remember if we actually need 4 of them, but I added a comment why
I do it.

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:307: return
_deviceLineRegexp.allMatches(result.stdout).map((Match m) =>
m.group(1)).toList();
On 2013/05/21 18:30:43, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/test_op...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/test_op...
tools/testing/dart/test_options.dart:320: '127.0.0.1'),];
On 2013/05/21 18:30:43, ricow1 wrote:
> you actually default to 0.0.0.0 in the http server

I'm aware and there is a reason for it:
- We don't want to expose (part of) the filesystem to all other people on the
network by default.

Here's why I did it in http_server.dart:
- If people get a failure message they get a list of commands they've to
execute. The http server command currently doesn't specify the ip to use (so it
takes 127.0.0.1) but the phone can't connect there.

Tough, it's not particularly nice.

[kustermann, 2013-05-22 09:41:40]

PTAL

[ricow1, 2013-05-22 13:13:36]

LGTM with suggestion for encapsulating the adb stuff

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
File tools/testing/dart/android.dart (right):

https://codereview.chromium.org/15567002/diff/2001/tools/testing/dart/android...
tools/testing/dart/android.dart:65: return [results[0], results[1]];
On 2013/05/22 09:30:58, kustermann wrote:
> On 2013/05/21 18:30:43, ricow1 wrote:
> > how about a ProcessResult like class for returning this?
> I think there is no public ProcessResult constructor:
> http://api.dartlang.org/docs/releases/latest/dart_io/ProcessResult.html
That is why I wrote ProcessResult _like_ class :-)

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
File tools/testing/dart/android.dart (right):

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
tools/testing/dart/android.dart:37: Future<String> getOutput(Stream<List<int>>
stream) {
we should file a feature request that allows you to specify stdin as part of
ProcessOptions, would clean all of this up

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
tools/testing/dart/android.dart:64: return [results[0], results[1]];
you never actually use stderr for anything, you just discard it

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
tools/testing/dart/android.dart:136: completer.complete(true);
shouldn't we use the exitCode for something here, don't we want to know if it
failed?

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
tools/testing/dart/android.dart:211: new Timer(_adbServerStartupTime, () =>
adbRootCompleter.complete(true));
are we guaranteed that this will only take 3 seconds?

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/browse...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:408: if (success) {
indentation

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:422: Future<List<Browser>>
getBrowsers() {
I think this is rather hackish, how about this:
The browser class has a method that returns the max number of instances (normal
ff/chrome=unlimited). The browser constructor  takes an id (I think it should
anyway, makes sense). Now in the AdndroidChrome class we have a static Map from
id to deviceid. That way we encapsulate all browser specific stuff in the
browser classes instead of having it leak in here?

[kustermann, 2013-05-22 15:32:41]

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
File tools/testing/dart/android.dart (right):

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
tools/testing/dart/android.dart:37: Future<String> getOutput(Stream<List<int>>
stream) {
On 2013/05/22 13:13:37, ricow1 wrote:
> we should file a feature request that allows you to specify stdin as part of
> ProcessOptions, would clean all of this up
I don't know if ProcessOptions is the right place. I'd probably do something
similar to pythons subprocess module. They have for example
subprocess.check_call, subprocess.check_output, ...
But I don't know if the library guys want to put such wrappers into 'dart:io'
(instead of making a pub package).

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
tools/testing/dart/android.dart:64: return [results[0], results[1]];
On 2013/05/22 13:13:37, ricow1 wrote:
> you never actually use stderr for anything, you just discard it

I removed it now. Normal programs only write to stderr if they also exit with
nonzero exitCode (and we throw if that is the case with an appropriate message).

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
tools/testing/dart/android.dart:136: completer.complete(true);
On 2013/05/22 13:13:37, ricow1 wrote:
> shouldn't we use the exitCode for something here, don't we want to know if it
> failed?
If we kill a browser or an emulator, we just want it to be gone. I don't know if
we could do something smart here.
I'll add a TODO. It's already written to the debug log on line 128.
(I don't think we'll use this code in the near future anyway, because chrome is
not working in the emulator).

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/androi...
tools/testing/dart/android.dart:211: new Timer(_adbServerStartupTime, () =>
adbRootCompleter.complete(true));
On 2013/05/22 13:13:37, ricow1 wrote:
> are we guaranteed that this will only take 3 seconds?
No. Normally it takes << 1 sec.
I don't know how we can wait for this to finish (it's not a blocking operation).
I'll add a TODO Here as well.

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/browse...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:408: if (success) {
On 2013/05/22 13:13:37, ricow1 wrote:
> indentation

Done.

https://codereview.chromium.org/15567002/diff/13001/tools/testing/dart/browse...
tools/testing/dart/browser_controller.dart:422: Future<List<Browser>>
getBrowsers() {
On 2013/05/22 13:13:37, ricow1 wrote:
> I think this is rather hackish, how about this:
> The browser class has a method that returns the max number of instances
(normal
> ff/chrome=unlimited). The browser constructor  takes an id (I think it should
> anyway, makes sense). Now in the AdndroidChrome class we have a static Map
from
> id to deviceid. That way we encapsulate all browser specific stuff in the
> browser classes instead of having it leak in here?
We've to determine the number of browsers before we actually create instances of
them.
This would mean the 'maxBrowsersSupported' property has to be static ...
I've added a TODO for now and will fix it later.

[kustermann, 2013-05-22 15:34:53]

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