Add Instrumentation test support to remote device

Add Instrumentation test support to remote device

This is the minimum to trigger/collect instrumentation tests.
There is still more to be done before this is working 100%.

BUG=448627
Committed: https://crrev.com/e416dff2dcefa69fc5c2d5ae928376364e4926f3
Cr-Commit-Position: refs/heads/master@{#312457}

[rnephew (Wrong account), 2015-01-16 20:26:50]

rnephew@google.com changed reviewers:
+ jbudorick@chromium.org, klundberg@chromium.org, rnephew@google.com

[jbudorick, 2015-01-16 20:44:15]

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/base/tes...
File build/android/pylib/base/test_run_factory.py (right):

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/base/tes...
build/android/pylib/base/test_run_factory.py:30: if isinstance(test_instance,
style: either:
 1) have a parameter on the first line and align subsequent lines to the start
of that first parameter, or
 2) have no parameters on the first line and indent subsequent lines four spaces

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/remote/d...
File build/android/pylib/remote/device/remote_device_instrumentation_run.py
(right):

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/remote/d...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:17:
_EXTRA_COMMAND_LINE_FILE = (
This should not be in here. Instrumentation tests don't use
ChromeNativeTestActivity.

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/remote/d...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:38: if
self._test_instance.flags:
This entire block should be removed for now.

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/remote/d...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:65: for
status_code, bundle in parsed_output:
This logic (or something along these lines) should not be limited to remote
devices. It's handling a general case: running more than one instrumentation
test in a single am instrument call. As such, this should be in the test
instance.

I'm not sure whether this should be combined with GenerateTestResult or should
be the basis of another function, though.

https://codereview.chromium.org/832493005/diff/1/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/832493005/diff/1/build/android/test_runner.py...
build/android/test_runner.py:145: group.add_argument('--app-under-test',
default='apks/Chrome.apk',
This default should be removed.

[rnephew (Wrong account), 2015-01-16 21:39:27]

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/base/tes...
File build/android/pylib/base/test_run_factory.py (right):

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/base/tes...
build/android/pylib/base/test_run_factory.py:30: if isinstance(test_instance,
On 2015/01/16 20:44:14, jbudorick wrote:
> style: either:
>  1) have a parameter on the first line and align subsequent lines to the start
> of that first parameter, or
>  2) have no parameters on the first line and indent subsequent lines four
spaces

Done.

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/remote/d...
File build/android/pylib/remote/device/remote_device_instrumentation_run.py
(right):

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/remote/d...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:17:
_EXTRA_COMMAND_LINE_FILE = (
On 2015/01/16 20:44:14, jbudorick wrote:
> This should not be in here. Instrumentation tests don't use
> ChromeNativeTestActivity.

Done.

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/remote/d...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:38: if
self._test_instance.flags:
On 2015/01/16 20:44:14, jbudorick wrote:
> This entire block should be removed for now.

Done.

https://codereview.chromium.org/832493005/diff/1/build/android/pylib/remote/d...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:65: for
status_code, bundle in parsed_output:
On 2015/01/16 20:44:14, jbudorick wrote:
> This logic (or something along these lines) should not be limited to remote
> devices. It's handling a general case: running more than one instrumentation
> test in a single am instrument call. As such, this should be in the test
> instance.
> 
> I'm not sure whether this should be combined with GenerateTestResult or should
> be the basis of another function, though.

Moved it to its own function in instrumentation_test_instance for now.

https://codereview.chromium.org/832493005/diff/1/build/android/test_runner.py
File build/android/test_runner.py (right):

https://codereview.chromium.org/832493005/diff/1/build/android/test_runner.py...
build/android/test_runner.py:145: group.add_argument('--app-under-test',
default='apks/Chrome.apk',
On 2015/01/16 20:44:14, jbudorick wrote:
> This default should be removed.

Done.

[jbudorick, 2015-01-16 22:24:48]

I need to look at the output format a bit more, but I can't help but feel like
we could have a cleaner implementation of GenerateTestResult /
GenerateMultiTestResult.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:143: def
GenerateMultiTestResult(errors, statuses):
This implementation can just be in the test instance. The other two functions
are outside because they're still used in old code.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:149: #
TODO(rnephew): Make skipped tests still output test name. This is only
What's preventing you from doing this now?

Also, nit: this should be indented two more spaces.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:152:
test_name = "%s" % skip_counter
should just be

  test_name = str(skip_counter)

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:162: if
len(errors) and errors.pop(0) == 'shortMsg=Native crash':
I'm not crazy about attempting to match an exact string here.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/remo...
File build/android/pylib/remote/device/remote_device_instrumentation_run.py
(right):

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:30:
runner_package = self._test_instance.test_runner
what's the point of doing this outside of the function call?

[rnephew (Wrong account), 2015-01-16 22:36:38]

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:143: def
GenerateMultiTestResult(errors, statuses):
On 2015/01/16 22:24:48, jbudorick wrote:
> This implementation can just be in the test instance. The other two functions
> are outside because they're still used in old code.

Done.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:149: #
TODO(rnephew): Make skipped tests still output test name. This is only
On 2015/01/16 22:24:48, jbudorick wrote:
> What's preventing you from doing this now?
> 
> Also, nit: this should be indented two more spaces.

I was planning on doing that in its own CL, as to not have large CLs touching
too many parts of the code. I also haven't started looking at how the tests
output when they run.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:152:
test_name = "%s" % skip_counter
On 2015/01/16 22:24:48, jbudorick wrote:
> should just be
> 
>   test_name = str(skip_counter)

Done.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:162: if
len(errors) and errors.pop(0) == 'shortMsg=Native crash':
On 2015/01/16 22:24:48, jbudorick wrote:
> I'm not crazy about attempting to match an exact string here.

Either am I. But this and longMsg='long version of this crash message' are the
only ones I have seen. I can't make it more robust until I know what else will
return here. I will add a logging statement that outputs all the errors.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/remo...
File build/android/pylib/remote/device/remote_device_instrumentation_run.py
(right):

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:30:
runner_package = self._test_instance.test_runner
On 2015/01/16 22:24:48, jbudorick wrote:
> what's the point of doing this outside of the function call?

I was originally doing other stuff, but got rid of it so this no longer makes
sense by itself. Moved.

[jbudorick, 2015-01-16 22:39:06]

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:149: #
TODO(rnephew): Make skipped tests still output test name. This is only
On 2015/01/16 22:36:38, rnephew wrote:
> On 2015/01/16 22:24:48, jbudorick wrote:
> > What's preventing you from doing this now?
> > 
> > Also, nit: this should be indented two more spaces.
> 
> I was planning on doing that in its own CL, as to not have large CLs touching
> too many parts of the code. I also haven't started looking at how the tests
> output when they run.

Fair enough.

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:162: if
len(errors) and errors.pop(0) == 'shortMsg=Native crash':
On 2015/01/16 22:36:38, rnephew wrote:
> On 2015/01/16 22:24:48, jbudorick wrote:
> > I'm not crazy about attempting to match an exact string here.
> 
> Either am I. But this and longMsg='long version of this crash message' are the
> only ones I have seen. I can't make it more robust until I know what else will
> return here. I will add a logging statement that outputs all the errors.

What if we just look for "Native crash" (maybe w/ case insensitivity)?

[rnephew (Wrong account), 2015-01-16 22:51:02]

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/20001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:162: if
len(errors) and errors.pop(0) == 'shortMsg=Native crash':
On 2015/01/16 22:39:06, jbudorick wrote:
> On 2015/01/16 22:36:38, rnephew wrote:
> > On 2015/01/16 22:24:48, jbudorick wrote:
> > > I'm not crazy about attempting to match an exact string here.
> > 
> > Either am I. But this and longMsg='long version of this crash message' are
the
> > only ones I have seen. I can't make it more robust until I know what else
will
> > return here. I will add a logging statement that outputs all the errors.
> 
> What if we just look for "Native crash" (maybe w/ case insensitivity)?

Done.

[jbudorick, 2015-01-20 15:39:04]

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/inst...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:466: def
GenerateMultiTestResult(self, errors, statuses):
Does this need |self|? It looks like it can be a @staticmethod.

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:484: print
'batman'
the debugging statement we deserve, but not the one we need right now.

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
File build/android/pylib/remote/device/remote_device_instrumentation_run.py
(right):

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:1: #
Copyright 2014 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:12: 
nit: no empty line here

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:17: class
RemoteDeviceInstrumentationRun(
nit: RemoteDeviceInstrumentationRun -> RemoteDeviceInstrumentationTestRun (and
rename the file accordingly)

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:31:
self._test_instance.runner_runner, environment_variables={})
runner_runner...?

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:46:
logging.info(errors)
This should either be:
 - removed
 - logged at debug level
 - better formatted & explained

As is, this is liable to confuse people reading the logs on the bots.

[rnephew (Wrong account), 2015-01-20 16:45:47]

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/inst...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:466: def
GenerateMultiTestResult(self, errors, statuses):
On 2015/01/20 15:39:03, jbudorick wrote:
> Does this need |self|? It looks like it can be a @staticmethod.

Done.

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:484: print
'batman'
On 2015/01/20 15:39:03, jbudorick wrote:
> the debugging statement we deserve, but not the one we need right now.

Done.

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
File build/android/pylib/remote/device/remote_device_instrumentation_run.py
(right):

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:1: #
Copyright 2014 The Chromium Authors. All rights reserved.
On 2015/01/20 15:39:04, jbudorick wrote:
> 2015

Done.

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:12: 
On 2015/01/20 15:39:04, jbudorick wrote:
> nit: no empty line here

Done.

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:17: class
RemoteDeviceInstrumentationRun(
On 2015/01/20 15:39:04, jbudorick wrote:
> nit: RemoteDeviceInstrumentationRun -> RemoteDeviceInstrumentationTestRun (and
> rename the file accordingly)

Done.

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:31:
self._test_instance.runner_runner, environment_variables={})
On 2015/01/20 15:39:03, jbudorick wrote:
> runner_runner...?

Done.

https://codereview.chromium.org/832493005/diff/60001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_instrumentation_run.py:46:
logging.info(errors)
On 2015/01/20 15:39:04, jbudorick wrote:
> This should either be:
>  - removed
>  - logged at debug level
>  - better formatted & explained
> 
> As is, this is liable to confuse people reading the logs on the bots.

Done.

[jbudorick, 2015-01-21 02:39:34]

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/inst...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:486: if
re.search('Native Crash', error, re.IGNORECASE):
Compile the regex and save it in the constant in class or module scope, e.g.,
something like this should work:

  NATIVE_CRASH_RE = re.compile('native crash', re.IGNORECASE)

  ...

    if NATIVE_CRASH_RE.search(error):

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/remo...
File build/android/pylib/remote/device/remote_device_gtest_run.py (left):

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_gtest_run.py:1: # Copyright 2014
The Chromium Authors. All rights reserved.
Leave this one as-is for now. "gtest_test_run" is a bit repetitive, it's not
related to this CL, and it's not consistent with the local version.

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/remo...
File build/android/pylib/remote/device/remote_device_uirobot_run.py (left):

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_uirobot_run.py:1: # Copyright
2014 The Chromium Authors. All rights reserved.
I guess I'm ok with changing this one.

[rnephew (Wrong account), 2015-01-21 16:13:12]

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/inst...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:486: if
re.search('Native Crash', error, re.IGNORECASE):
On 2015/01/21 02:39:34, jbudorick wrote:
> Compile the regex and save it in the constant in class or module scope, e.g.,
> something like this should work:
> 
>   NATIVE_CRASH_RE = re.compile('native crash', re.IGNORECASE)
> 
>   ...
> 
>     if NATIVE_CRASH_RE.search(error):

Done.

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/remo...
File build/android/pylib/remote/device/remote_device_gtest_run.py (left):

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_gtest_run.py:1: # Copyright 2014
The Chromium Authors. All rights reserved.
On 2015/01/21 02:39:34, jbudorick wrote:
> Leave this one as-is for now. "gtest_test_run" is a bit repetitive, it's not
> related to this CL, and it's not consistent with the local version.

Done.

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/remo...
File build/android/pylib/remote/device/remote_device_uirobot_run.py (left):

https://codereview.chromium.org/832493005/diff/80001/build/android/pylib/remo...
build/android/pylib/remote/device/remote_device_uirobot_run.py:1: # Copyright
2014 The Chromium Authors. All rights reserved.
On 2015/01/21 02:39:34, jbudorick wrote:
> I guess I'm ok with changing this one.

Done.

[jbudorick, 2015-01-21 16:18:14]

lgtm w/ nit

https://codereview.chromium.org/832493005/diff/120001/build/android/pylib/ins...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/120001/build/android/pylib/ins...
build/android/pylib/instrumentation/instrumentation_test_instance.py:29:
_NATIVE_CRASH_RE = re.compile('native crash', re.IGNORECASE)
nit: put this before _PICKLE_FORMAT_VERSION

[rnephew (Wrong account), 2015-01-21 16:22:34]

https://codereview.chromium.org/832493005/diff/120001/build/android/pylib/ins...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/832493005/diff/120001/build/android/pylib/ins...
build/android/pylib/instrumentation/instrumentation_test_instance.py:29:
_NATIVE_CRASH_RE = re.compile('native crash', re.IGNORECASE)
On 2015/01/21 16:18:14, jbudorick wrote:
> nit: put this before _PICKLE_FORMAT_VERSION

Done.

[rnephew (Wrong account), 2015-01-21 16:22:37]

The CQ bit was checked by rnephew@google.com

[commit-bot: I haz the power, 2015-01-21 16:22:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832493005/100002

[commit-bot: I haz the power, 2015-01-21 18:06:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 18:06:54]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rnephew (Wrong account), 2015-01-21 20:36:27]

The CQ bit was checked by rnephew@google.com

[commit-bot: I haz the power, 2015-01-21 20:37:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832493005/100002

[commit-bot: I haz the power, 2015-01-21 20:39:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-21 20:39:02]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rnephew (Wrong account), 2015-01-21 21:23:05]

The CQ bit was checked by rnephew@google.com

[commit-bot: I haz the power, 2015-01-21 21:24:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/832493005/100002

[commit-bot: I haz the power, 2015-01-21 21:27:08]

Committed patchset #8 (id:100002)

[commit-bot: I haz the power, 2015-01-21 21:28:54]

Patchset 8 (id:??) landed as
https://crrev.com/e416dff2dcefa69fc5c2d5ae928376364e4926f3
Cr-Commit-Position: refs/heads/master@{#312457}