[Android] Implement instrumentation tests in platform mode.

[Android] Implement instrumentation tests in platform mode.

BUG=428729
Committed: https://crrev.com/911be58db834f53799c84ffaeb9abeb87f643ce7
Cr-Commit-Position: refs/heads/master@{#311189}

[jbudorick, 2015-01-07 20:35:48]

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

[klundberg, 2015-01-07 22:56:30]

I might need to do another run through this CL as it is quite big but for now I
have these comments/questions.

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

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:1: 
Copyright?

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:131:
status_code)
Maybe log this as an error instead? 
Seems like a pretty big deal if we get an unknown status code...I was even
considering that we should throw an exception but I think that might be too
disruptive :-)

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:136: print
'test: %r' % test_name
Why print and not logging.info?
Is this just leftover from debugging?

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:146: # APKs
This comment isn't very descriptive. Maybe remove or make it more descriptive?

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:148:
self._apk_under_test = args.apk_under_test
For the test apk, you check whether the apk exists when the test_apk endswith
.apk and if it doesn't, create the full path. 
But here you assume the apk under test is a full path when it ends with .apk. 
Why not be consistent/defensive and perform the same check here?

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:169:
'%s.apk' % args.test_apk)
nit: Would be nice with an empty line here.

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:202: if
args.test_data:
Should we have a TODO to deprecate this?
I noticed you have a TODO below but might be worth to have one here too.

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/loca...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:1: 
Copyright?

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:55:
def individual_device_set_up(dev, host_device_tuples):
Does this mean that the test runner will always install both APKs?
On the existing instrumentation bots, the "deploy on devices" step installs
chrome.apk and the "install APK" step installs the test APK. Very confusing :-(

I would love to see the test runner make this easier for the bots and it seems
like this could be part of that(?).

[jbudorick, 2015-01-12 16:32:16]

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

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:1: 
On 2015/01/07 22:56:30, klundberg wrote:
> Copyright?

oops. done

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:131:
status_code)
On 2015/01/07 22:56:30, klundberg wrote:
> Maybe log this as an error instead? 
> Seems like a pretty big deal if we get an unknown status code...I was even
> considering that we should throw an exception but I think that might be too
> disruptive :-)

Done.

I do think an exception would be a bit much.

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:136: print
'test: %r' % test_name
On 2015/01/07 22:56:30, klundberg wrote:
> Why print and not logging.info?
> Is this just leftover from debugging?

Yep. Removed.

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:146: # APKs
On 2015/01/07 22:56:30, klundberg wrote:
> This comment isn't very descriptive. Maybe remove or make it more descriptive?

This comment (along with "Data dependencies", "Test filters", and "Flags") was
just here to note the beginning of a section of __init__. In the new patchset, I
removed these comments and separated the various parts of __init__ into private
member functions. WDYT?

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:148:
self._apk_under_test = args.apk_under_test
On 2015/01/07 22:56:30, klundberg wrote:
> For the test apk, you check whether the apk exists when the test_apk endswith
> .apk and if it doesn't, create the full path. 
> But here you assume the apk under test is a full path when it ends with .apk. 
> Why not be consistent/defensive and perform the same check here?

I made the two consistent by removing what I think was an overzealous
implementation from the test_apk section. That added support for a behavior that
I don't think we need to have.

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:169:
'%s.apk' % args.test_apk)
On 2015/01/07 22:56:30, klundberg wrote:
> nit: Would be nice with an empty line here.

Done.

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:202: if
args.test_data:
On 2015/01/07 22:56:30, klundberg wrote:
> Should we have a TODO to deprecate this?
> I noticed you have a TODO below but might be worth to have one here too.

Done.

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/loca...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:1: 
On 2015/01/07 22:56:30, klundberg wrote:
> Copyright?

Done.

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/loca...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:55:
def individual_device_set_up(dev, host_device_tuples):
On 2015/01/07 22:56:30, klundberg wrote:
> Does this mean that the test runner will always install both APKs?

Yes. It's ok if they're already installed, though.

> On the existing instrumentation bots, the "deploy on devices" step installs
> chrome.apk and the "install APK" step installs the test APK. Very confusing
:-(

I think that's only on the downstream bots. The upstream bots do "Install
ChromeShell.apk" and "Instrumentation test ChromeShellTest", where the latter
handles test apk installation.

> 
> I would love to see the test runner make this easier for the bots and it seems
> like this could be part of that(?).

This could eliminate the step that installs the APK under test.

[klundberg, 2015-01-12 17:16:18]

lgtm

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

https://codereview.chromium.org/794923003/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/instrumentation_test_instance.py:146: # APKs
On 2015/01/12 16:32:16, jbudorick wrote:
> On 2015/01/07 22:56:30, klundberg wrote:
> > This comment isn't very descriptive. Maybe remove or make it more
descriptive?
> 
> This comment (along with "Data dependencies", "Test filters", and "Flags") was
> just here to note the beginning of a section of __init__. In the new patchset,
I
> removed these comments and separated the various parts of __init__ into
private
> member functions. WDYT?

I think this is much better. Much easier to read.

[jbudorick, 2015-01-12 23:35:49]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-01-12 23:37:06]

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

[commit-bot: I haz the power, 2015-01-12 23:45:32]

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

[commit-bot: I haz the power, 2015-01-12 23:45:33]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jbudorick, 2015-01-12 23:54:03]

jbudorick@chromium.org changed reviewers:
+ maruel@chromium.org

[jbudorick, 2015-01-12 23:54:03]

+maruel for *.isolate

[M-A Ruel, 2015-01-13 00:03:26]

lgtm, feel free to TBR simple changes like this.

[jbudorick, 2015-01-13 00:04:24]

On 2015/01/13 00:03:26, M-A Ruel wrote:
> lgtm, feel free to TBR simple changes like this.

sgtm, thanks

[jbudorick, 2015-01-13 00:04:31]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-01-13 00:04:43]

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

[commit-bot: I haz the power, 2015-01-13 02:51:48]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-13 02:52:40]

Patchset 4 (id:??) landed as
https://crrev.com/911be58db834f53799c84ffaeb9abeb87f643ce7
Cr-Commit-Position: refs/heads/master@{#311189}