[devil] Add system app replacement wrapper script.

[devil] Add system app replacement wrapper script.

BUG=chromium:728404
Review-Url: https://codereview.chromium.org/2923443002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/36487978c43b55ab38f8d0ba4d698cae2aefed85

[jbudorick, 2017-06-12 17:58:41]

Description was changed from

==========
Add system app replacement wrapper script.

BUG=chromium:728404
==========

to

==========
[devil] Add system app replacement wrapper script.

BUG=chromium:728404
==========

[jbudorick, 2017-06-12 17:58:58]

jbudorick@chromium.org changed reviewers:
+ bsheedy@chromium.org, martiniss@chromium.org

[bsheedy, 2017-06-12 18:31:07]

LGTM w/ question.

https://codereview.chromium.org/2923443002/diff/40001/devil/devil/android/too...
File devil/devil/android/tools/system_app.py (right):

https://codereview.chromium.org/2923443002/diff/40001/devil/devil/android/too...
devil/devil/android/tools/system_app.py:73: def
EnableSystemAppModification(device):
Should this be a nop if this happens to be called again while already in scope
of another EnableSystemAppModification? Or is this already handled by being a
contextmanager/are we assuming that this will never be called while already in
scope?

[jbudorick, 2017-06-12 20:55:45]

https://codereview.chromium.org/2923443002/diff/40001/devil/devil/android/too...
File devil/devil/android/tools/system_app.py (right):

https://codereview.chromium.org/2923443002/diff/40001/devil/devil/android/too...
devil/devil/android/tools/system_app.py:73: def
EnableSystemAppModification(device):
On 2017/06/12 18:31:07, bsheedy wrote:
> Should this be a nop if this happens to be called again while already in scope
> of another EnableSystemAppModification? Or is this already handled by being a
> contextmanager/are we assuming that this will never be called while already in
> scope?

Good question. It should, though I had been assuming that it'd never be called
while already in scope. Revised to use a property on the device to manage
modification state & added a test.

[bsheedy, 2017-06-12 21:01:57]

On 2017/06/12 20:55:45, jbudorick wrote:
>
https://codereview.chromium.org/2923443002/diff/40001/devil/devil/android/too...
> File devil/devil/android/tools/system_app.py (right):
> 
>
https://codereview.chromium.org/2923443002/diff/40001/devil/devil/android/too...
> devil/devil/android/tools/system_app.py:73: def
> EnableSystemAppModification(device):
> On 2017/06/12 18:31:07, bsheedy wrote:
> > Should this be a nop if this happens to be called again while already in
scope
> > of another EnableSystemAppModification? Or is this already handled by being
a
> > contextmanager/are we assuming that this will never be called while already
in
> > scope?
> 
> Good question. It should, though I had been assuming that it'd never be called
> while already in scope. Revised to use a property on the device to manage
> modification state & added a test.

LGTM. Once this makes it in, I'll want to discuss the possibility of getting VR
tests running on on Pixel devices on something other than our FYI bot with you
or another appropriate infra person.

[jbudorick, 2017-06-13 04:51:15]

Description was changed from

==========
[devil] Add system app replacement wrapper script.

BUG=chromium:728404
==========

to

==========
[devil] Add system app replacement wrapper script.

BUG=chromium:728404
==========

[jbudorick, 2017-06-13 14:07:11]

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

[jbudorick, 2017-06-13 14:07:11]

+mikecase for device temp directory & the parallelizer changes

[mikecase (-- gone --), 2017-06-13 21:20:16]

lgtm parallalizer + temp_dir

https://codereview.chromium.org/2923443002/diff/100001/devil/devil/android/de...
File devil/devil/android/device_temp_file.py (right):

https://codereview.chromium.org/2923443002/diff/100001/devil/devil/android/de...
devil/devil/android/device_temp_file.py:70: class
NamedDeviceTemporaryDirectory(object):
oooh! Going to use this in a few places in test_runner.py

[jbudorick, 2017-06-13 21:30:43]

https://codereview.chromium.org/2923443002/diff/100001/devil/devil/android/de...
File devil/devil/android/device_temp_file.py (right):

https://codereview.chromium.org/2923443002/diff/100001/devil/devil/android/de...
devil/devil/android/device_temp_file.py:70: class
NamedDeviceTemporaryDirectory(object):
On 2017/06/13 21:20:16, mikecase wrote:
> oooh! Going to use this in a few places in test_runner.py

Figured you might like this part :)

Added a few docstrings below.

[jbudorick, 2017-06-13 21:31:07]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2017-06-13 21:31:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
bsheedy@chromium.org, mikecase@chromium.org
Link to the patchset: https://codereview.chromium.org/2923443002/#ps120001
(title: "docstrings for device_temp_file")

[commit-bot: I haz the power, 2017-06-13 21:31:18]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-13 21:59:12]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497389467804970,
"parent_rev": "f6be5fc6cf65875b7e076ccb2c720b39331875f1", "commit_rev":
"36487978c43b55ab38f8d0ba4d698cae2aefed85"}

[commit-bot: I haz the power, 2017-06-13 21:59:16]

Description was changed from

==========
[devil] Add system app replacement wrapper script.

BUG=chromium:728404
==========

to

==========
[devil] Add system app replacement wrapper script.

BUG=chromium:728404

Review-Url: https://codereview.chromium.org/2923443002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-06-13 21:59:17]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...