Android Add _incremental targets for instrumentation tests

Android Add _incremental targets for instrumentation tests

Switches gtests to explicitly passing path to install script rather than
--incremental-install + heuristics.

BUG=575975
Committed: https://crrev.com/4931af8c9d77f7d2b76423405b20e1f8abc9bce7
Cr-Commit-Position: refs/heads/master@{#374699}

[agrieve, 2016-02-09 18:55:47]

agrieve@chromium.org changed reviewers:
+ dpranke@chromium.org, jbudorick@chromium.org

[agrieve, 2016-02-09 18:55:47]

☔☔☔

dpranke: please look at //testing

jbudorick: please review.

[Dirk Pranke, 2016-02-09 19:21:56]

//testing lgtm.

[jbudorick, 2016-02-09 20:08:27]

I'm a bit uneasy about this, but it is definitely an improvement in that it
provides explicit paths instead of hard-coded guesses.

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/gtest/g...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/gtest/g...
build/android/pylib/gtest/gtest_test_instance.py:262: def
test_apk_install_script(self):
Hm. These don't really belong in the test_instance, as they're not generic to
gtests but instead are specific to (gtest + local) (and similarly for the
instrumentation ones)

Unfortunately, in my limited wisdom, I didn't give the arguments to the
test_runs at construction time, so I guess I'm ok with this for now.

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/instrumentation_test_instance.py:258:
self._suite = self._suite[:-12]
nit: self._suite[:-len('_incremental')]

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_environment.py (left):

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_environment.py:94: def
incremental_install(self):
So incremental installation is now implicit based on whether or not the caller
provides installer scripts...? I'm not crazy about that as-is but would be ok
with it if "incremental" is somewhere in the argument name, as mentioned over in
test_runner.py.

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_test_run.py (right):

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_test_run.py:12: from
incremental_install import installer
this is going to sound unusual, but please move this inside IncrementalInstall.

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

https://codereview.chromium.org/1680233002/diff/1/build/android/test_runner.p...
build/android/test_runner.py:251:
group.add_argument('--test-apk-install-script',
If these arguments are going to trigger incremental mode, they should have
incremental somewhere in the name.

Also, I'm going to try to figure out how to hide these in the help text at some
point, because I imagine it'll be nearly impossible for someone to use these
outside the scope of a generated script.

[agrieve, 2016-02-09 21:43:50]

didn't see any easy way to hide the flags. Might have to use a 2nd pass arg
parse.

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/instrum...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/instrum...
build/android/pylib/instrumentation/instrumentation_test_instance.py:258:
self._suite = self._suite[:-12]
On 2016/02/09 20:08:26, jbudorick wrote:
> nit: self._suite[:-len('_incremental')]

Done.

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_environment.py (left):

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_environment.py:94: def
incremental_install(self):
On 2016/02/09 20:08:26, jbudorick wrote:
> So incremental installation is now implicit based on whether or not the caller
> provides installer scripts...? I'm not crazy about that as-is but would be ok
> with it if "incremental" is somewhere in the argument name, as mentioned over
in
> test_runner.py.

Done.

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/local/d...
File build/android/pylib/local/device/local_device_test_run.py (right):

https://codereview.chromium.org/1680233002/diff/1/build/android/pylib/local/d...
build/android/pylib/local/device/local_device_test_run.py:12: from
incremental_install import installer
On 2016/02/09 20:08:27, jbudorick wrote:
> this is going to sound unusual, but please move this inside
IncrementalInstall.

Done.

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

https://codereview.chromium.org/1680233002/diff/1/build/android/test_runner.p...
build/android/test_runner.py:251:
group.add_argument('--test-apk-install-script',
On 2016/02/09 20:08:27, jbudorick wrote:
> If these arguments are going to trigger incremental mode, they should have
> incremental somewhere in the name.
> 
> Also, I'm going to try to figure out how to hide these in the help text at
some
> point, because I imagine it'll be nearly impossible for someone to use these
> outside the scope of a generated script.

Done.

[jbudorick, 2016-02-09 23:32:32]

lgtm w/ nits

I didn't mean to suggest that you should do the argument hiding in this CL.
test_runner.py has quite a few arguments in general and could use some cleaning
overall. I intend to do that after switching instrumentation tests over to the
new implementation.

https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
build/android/gyp/write_build_config.py:220: help='Path to the target\'s
incremental apk output.')
nit: use double quotes around strings containing an apostrophe s.t. you don't
have to escape.

https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
build/android/gyp/write_build_config.py:222: help='Path to the target\'s
generated incremental install '
nit: same

https://codereview.chromium.org/1680233002/diff/20001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1680233002/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:71:
def install_apk():
nit through line 86: either
 - no parameters on the first line of a function call w/ a 4-space indent on
subsequent lines, or
 - subsequent lines should be indented to the depth of the first parameter

(this is a bit of a departure from our java style)

[agrieve, 2016-02-10 01:56:56]

Thanks for speedy reviews!

https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
build/android/gyp/write_build_config.py:220: help='Path to the target\'s
incremental apk output.')
On 2016/02/09 23:32:32, jbudorick wrote:
> nit: use double quotes around strings containing an apostrophe s.t. you don't
> have to escape.

Done.

https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
build/android/gyp/write_build_config.py:222: help='Path to the target\'s
generated incremental install '
On 2016/02/09 23:32:32, jbudorick wrote:
> nit: same

Done.

https://codereview.chromium.org/1680233002/diff/20001/build/android/pylib/loc...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1680233002/diff/20001/build/android/pylib/loc...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:71:
def install_apk():
On 2016/02/09 23:32:32, jbudorick wrote:
> nit through line 86: either
>  - no parameters on the first line of a function call w/ a 4-space indent on
> subsequent lines, or
>  - subsequent lines should be indented to the depth of the first parameter
> 
> (this is a bit of a departure from our java style)

Done.

[jbudorick, 2016-02-10 02:01:09]

On 2016/02/10 01:56:56, agrieve wrote:
> Thanks for speedy reviews!

Ha, I'm trying to get better about them.

> 
>
https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
> File build/android/gyp/write_build_config.py (right):
> 
>
https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
> build/android/gyp/write_build_config.py:220: help='Path to the target\'s
> incremental apk output.')
> On 2016/02/09 23:32:32, jbudorick wrote:
> > nit: use double quotes around strings containing an apostrophe s.t. you
don't
> > have to escape.
> 
> Done.
> 
>
https://codereview.chromium.org/1680233002/diff/20001/build/android/gyp/write...
> build/android/gyp/write_build_config.py:222: help='Path to the target\'s
> generated incremental install '
> On 2016/02/09 23:32:32, jbudorick wrote:
> > nit: same
> 
> Done.
> 
>
https://codereview.chromium.org/1680233002/diff/20001/build/android/pylib/loc...
> File build/android/pylib/local/device/local_device_instrumentation_test_run.py
> (right):
> 
>
https://codereview.chromium.org/1680233002/diff/20001/build/android/pylib/loc...
> build/android/pylib/local/device/local_device_instrumentation_test_run.py:71:
> def install_apk():
> On 2016/02/09 23:32:32, jbudorick wrote:
> > nit through line 86: either
> >  - no parameters on the first line of a function call w/ a 4-space indent on
> > subsequent lines, or
> >  - subsequent lines should be indented to the depth of the first parameter
> > 
> > (this is a bit of a departure from our java style)
> 
> Done.

[agrieve, 2016-02-10 02:03:00]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-02-10 02:03:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1680233002/#ps40001
(title: "review comments")

[commit-bot: I haz the power, 2016-02-10 02:06:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1680233002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1680233002/40001

[commit-bot: I haz the power, 2016-02-10 02:51:11]

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

[commit-bot: I haz the power, 2016-02-10 02:51:12]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[agrieve, 2016-02-10 15:58:35]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-02-10 15:58:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1680233002/#ps60001
(title: "add missing dep")

[commit-bot: I haz the power, 2016-02-10 15:59:42]

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

[commit-bot: I haz the power, 2016-02-10 18:07:54]

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

[commit-bot: I haz the power, 2016-02-10 18:07:55]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

Transient error: No JSON object could be decoded

[commit-bot: I haz the power, 2016-02-10 18:11:07]

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

[agrieve, 2016-02-10 19:11:56]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-02-10 19:14:53]

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

[commit-bot: I haz the power, 2016-02-10 19:26:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-16 22:30:25]

Description was changed from

==========
Android Add _incremental targets for instrumentation tests

Switches gtests to explicitly passing path to install script rather than
--incremental-install + heuristics.

BUG=575975
==========

to

==========
Android Add _incremental targets for instrumentation tests

Switches gtests to explicitly passing path to install script rather than
--incremental-install + heuristics.

BUG=575975

Committed: https://crrev.com/4931af8c9d77f7d2b76423405b20e1f8abc9bce7
Cr-Commit-Position: refs/heads/master@{#374699}
==========

[commit-bot: I haz the power, 2016-02-16 22:30:26]

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