Changes to run tests on Dartium content shell on Android.

Changes to run tests on Dartium content shell on Android.

- In browser_controller.dart, adds a drtOnAndroid Browser.
- Adds flags for starting test servers at fixed ports.
  - This is needed so that android reverse port forwarding
    can be set up in advance.
- Adds setProp to android.dart to be used in lieu of
  environment variables when testing on Android.

R=kustermann@google.com

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

[kustermann, 2013-10-24 09:36:00]

First round of comments.

A few thoughts
- we should use Chrome.apk instead of ContentShell.apk
   * our users will use the clank/chrome and not ContentShell
   * spending time on getting ContentShell stable might be a waste (there might
be subtle differences, when tests work on chrome but not on CS)
- we should wait until a) building chrome.apk works b) it works reliably on the
bots
   * e.g.: when you copied the "AndroidChrome" class, you removed some code
(installing the apk + resetting the profile + ...)
- the port numbers we pass in is not so pretty -- it is actually beneficial to
have different port numbers every time
   * for now we actually have WLAN in the lab, we currently use that

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
tools/testing/dart/browser_controller.dart:535: class drtOnAndroid extends
Browser {
drtOnAndroid => DrtOnAndroid

Since drt has been replaced by content_shell, we may want to rename it to
ContentShellOnAndroid or something like that?

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
tools/testing/dart/browser_controller.dart:539: AndroidEmulator _emulator;
Where is this _emulator set / used ?

Did you try running it on an emulator?

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
tools/testing/dart/browser_controller.dart:553: return
_adbDevice.setProp("DART_FORWARDING_PRINT", "1");
Are you sure that a 'adb shell setprop' is setting environment variables?

I'm not sure, but I think "system properties != environment variables".

(Anton has added support for this a while ago in the dartium code, but it
expects it to be a normal environment variable)

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
tools/testing/dart/browser_controller.dart:778: } else if (browserName ==
'drtOnAndroid') {
There is no reason to duplicate this code. You could do something like:

var androidBrowserCreationMapping = {
  'chromeOnAndroid' : (AdbDevice device) => new AndroidChrome(device),
  'drtOnAndroid' : (AdbDevice device) => new DrtOnAndroid(device),
}

if (androidBrowserCreationMapping.containsKey(browserName)) {
  ...
  var browser = androidBrowserCreationMapping[browserName](device);
  ....
}

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/test_optio...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/test_optio...
tools/testing/dart/test_options.dart:330: 0),
You've the 'int.parse()' all over the place. Please specify an integer type as
with the '--timeout' or '--tasks' option.

[zra, 2013-10-24 15:34:49]

This change is to help us test locally in preparation for moving to the bots. It
is early days, and we'd like to make incremental changes instead of making a
giant change and leaping onto the bots all at once.

- I can add a test target for chrome_apk in a subsequent CL.

- I removed stuff from AndroidChrome because
'third_party/android_testing_resources' doesn't appear to exist, and because we
are just installing the apk manually using the Clank scripts before testing. We
can add back the 'TurnScreenOn.apk' and installation to the test script prior to
moving to the bots.

Random port numbers won't work locally without a much larger change since our
devices aren't on corp wifi, and must connect through reverse port forwarding
over usb, which must be set up in advance.

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
File tools/testing/dart/browser_controller.dart (right):

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
tools/testing/dart/browser_controller.dart:535: class drtOnAndroid extends
Browser {
On 2013/10/24 09:36:00, kustermann wrote:
> drtOnAndroid => DrtOnAndroid
> 
> Since drt has been replaced by content_shell, we may want to rename it to
> ContentShellOnAndroid or something like that?

Renamed ContentShellOnAndroid

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
tools/testing/dart/browser_controller.dart:539: AndroidEmulator _emulator;
On 2013/10/24 09:36:00, kustermann wrote:
> Where is this _emulator set / used ?
> 
> Did you try running it on an emulator?

Removed

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
tools/testing/dart/browser_controller.dart:553: return
_adbDevice.setProp("DART_FORWARDING_PRINT", "1");
On 2013/10/24 09:36:00, kustermann wrote:
> Are you sure that a 'adb shell setprop' is setting environment variables?
> 
> I'm not sure, but I think "system properties != environment variables".
> 
> (Anton has added support for this a while ago in the dartium code, but it
> expects it to be a normal environment variable)

In a separate CL (https://codereview.chromium.org/38323002/) I modify to use
system properties.

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/browser_co...
tools/testing/dart/browser_controller.dart:778: } else if (browserName ==
'drtOnAndroid') {
On 2013/10/24 09:36:00, kustermann wrote:
> There is no reason to duplicate this code. You could do something like:
> 
> var androidBrowserCreationMapping = {
>   'chromeOnAndroid' : (AdbDevice device) => new AndroidChrome(device),
>   'drtOnAndroid' : (AdbDevice device) => new DrtOnAndroid(device),
> }
> 
> if (androidBrowserCreationMapping.containsKey(browserName)) {
>   ...
>   var browser = androidBrowserCreationMapping[browserName](device);
>   ....
> }

Done.

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/test_optio...
File tools/testing/dart/test_options.dart (right):

https://codereview.chromium.org/29773006/diff/1/tools/testing/dart/test_optio...
tools/testing/dart/test_options.dart:330: 0),
On 2013/10/24 09:36:00, kustermann wrote:
> You've the 'int.parse()' all over the place. Please specify an integer type as
> with the '--timeout' or '--tasks' option.

Done.

[kustermann, 2013-10-25 15:35:01]

LGTM

(But I think you focus on *the real* browser (in terms of building + testing)
instead of ContentShell.)

https://codereview.chromium.org/29773006/diff/60001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/29773006/diff/60001/tools/test.dart#newcode135
tools/test.dart:135: for (var conf in configurations) {
Please add an assertion before this loop, that if 'test_server_port' or
'test_server_cross_origin_port' is 0 in the first configuration, then the
"configurations.length" has to be 1.

(We start two http servers per configuration. Having a fixed port means we can
have only one configuration!)

[zra, 2013-10-25 16:19:35]

https://codereview.chromium.org/29773006/diff/60001/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/29773006/diff/60001/tools/test.dart#newcode135
tools/test.dart:135: for (var conf in configurations) {
On 2013/10/25 15:35:01, kustermann wrote:
> Please add an assertion before this loop, that if 'test_server_port' or
> 'test_server_cross_origin_port' is 0 in the first configuration, then the
> "configurations.length" has to be 1.
> 
> (We start two http servers per configuration. Having a fixed port means we can
> have only one configuration!)

Done.

[zra, 2013-10-25 16:39:54]

Committed patchset #3 manually as r29259 (presubmit successful).