[Telemetry] Switch RunShellCommand clients to check_return=True

[Telemetry] Switch RunShellCommand clients to check_return=True

In all files where check_return was missing within /telemetry:
- enable check_return=True,
- pass the command as a list (rather than a string) where appropriate,
- replace calls to RunShellCommand with higher level commands (e.g.
  RemovePath) where appropriate.

BUG=chromium:628617
Review-Url: https://codereview.chromium.org/2709523005
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/a75c463e8416e25de8c1d70ec210a1de89ea19a5

[perezju, 2017-02-21 12:04:53]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 12:05:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[perezju, 2017-02-21 12:11:54]

perezju@chromium.org changed reviewers:
+ jbudorick@chromium.org, nednguyen@google.com

[commit-bot: I haz the power, 2017-02-21 12:41:29]

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

[commit-bot: I haz the power, 2017-02-21 12:41:30]

Dry run: Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)

[nednguyen, 2017-02-21 13:41:01]

lgtm

[perezju, 2017-02-21 15:40:30]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 15:40:34]

Dry run: 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-02-21 15:41:45]

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

[commit-bot: I haz the power, 2017-02-21 15:41:46]

Dry run: Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
  Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)
  Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)

[perezju, 2017-02-21 15:56:44]

check_return already proving useful to catch bugs in the existing code base.

PTAL

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:609: ['chown',
'-R', owner, filepath], check_return=True, as_root=True)
The previous version actually failed with:

    - chown: /data/data/com.google.android.apps.chrome/app_tabs/*/*/*: No such
file or directory

but we never noticed due to the lack of check_return. Not sure why we never used
-R to begin with (traced that code back to over 3 years ago at
https://codereview.chromium.org/50493010). I tested the new version and it and
it works at least on L and M.

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:618: ['sh',
self._device_copy_script, source, dest], check_return=True)
This hasn't actually worked for a long time (it was trying to use the host path
on the device!). Again, check_return to the rescue.

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:674:
logging.warning('- %s', filepath)
As stated in the comment, this fails pretty consistently on some paths; not sure
why. The changes are just to reduce the log spam, eliminate the unnecessary call
stack, and make it easier to see the list of paths that failed to be retrieved.

[nednguyen, 2017-02-21 16:00:14]

Fail sooner & harder FTW!

Stil lgtm

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:674:
logging.warning('- %s', filepath)
On 2017/02/21 15:56:44, perezju wrote:
> As stated in the comment, this fails pretty consistently on some paths; not
sure
> why. The changes are just to reduce the log spam, eliminate the unnecessary
call
> stack, and make it easier to see the list of paths that failed to be
retrieved.

Hmhh, maybe the files are too big?

[perezju, 2017-02-21 16:05:25]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 16:05:35]

Dry run: 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-02-21 16:10:25]

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

[commit-bot: I haz the power, 2017-02-21 16:10:26]

Dry run: Try jobs failed on following builders:
  Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)

[jbudorick, 2017-02-21 16:11:18]

There appear to be quite a few device path manipulations using os.path rather
than posixpath. That shouldn't make a difference in our current setup, but I
think it's something we could address in a subsequent CL.

lgtm w/ nits

https://codereview.chromium.org/2709523005/diff/1/telemetry/telemetry/core/an...
File telemetry/telemetry/core/android_action_runner.py (right):

https://codereview.chromium.org/2709523005/diff/1/telemetry/telemetry/core/an...
telemetry/telemetry/core/android_action_runner.py:82:
self._platform_backend.device.RunShellCommand(
DeviceUtils has SendKeyEvent, though perhaps that's better deferred for another
CL.

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:76:
self.device.RunShellCommand(['logcat', '-c'], check_return=True)
device.adb.Logcat(clear=True), though again, maybe for a future CL.

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:665:
self._device.PullFile(source, dest, retries=0, timeout=240)
No retries?

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:674:
logging.warning('- %s', filepath)
On 2017/02/21 15:56:44, perezju wrote:
> As stated in the comment, this fails pretty consistently on some paths; not
sure
> why. The changes are just to reduce the log spam, eliminate the unnecessary
call
> stack, and make it easier to see the list of paths that failed to be
retrieved.

Does the exception text contain anything helpful?

[perezju, 2017-02-21 16:12:33]

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:674:
logging.warning('- %s', filepath)
On 2017/02/21 16:00:14, nednguyen wrote:
> On 2017/02/21 15:56:44, perezju wrote:
> > As stated in the comment, this fails pretty consistently on some paths; not
> sure
> > why. The changes are just to reduce the log spam, eliminate the unnecessary
> call
> > stack, and make it easier to see the list of paths that failed to be
> retrieved.
> 
> Hmhh, maybe the files are too big?

I don't think so, it fails immediately so it doesn't look like it's trying to
copy a lot of things and then fails because of that. I'm suspecting there is
some sort of race condition, since I cannot find the failing paths on the device
after the test has finished, maybe they are being removed as we try to pull
them?

[perezju, 2017-02-21 16:33:08]

From the latest try job:

    command: chown -R 10074.10074
/data/data/com.google.android.apps.chrome/app_chrome
    exit status: 10
    output:
    - No such user '-R'


:'-(

There's my answer. It doesn't work on K.

Ideas?

- Do a recursive list of all the files, then chown each invididually. (i.e. 2 or
more RunShellCommands per iteration)
- Write a single shell script to the listing + chown'ing on one go.

Either way fixing that should probably be a separate CL.

https://codereview.chromium.org/2709523005/diff/1/telemetry/telemetry/core/an...
File telemetry/telemetry/core/android_action_runner.py (right):

https://codereview.chromium.org/2709523005/diff/1/telemetry/telemetry/core/an...
telemetry/telemetry/core/android_action_runner.py:82:
self._platform_backend.device.RunShellCommand(
On 2017/02/21 16:11:14, jbudorick wrote:
> DeviceUtils has SendKeyEvent, though perhaps that's better deferred for
another
> CL.

The fewer calls to RunShellCommand the happier I am. Done.

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:76:
self.device.RunShellCommand(['logcat', '-c'], check_return=True)
On 2017/02/21 16:11:14, jbudorick wrote:
> device.adb.Logcat(clear=True), though again, maybe for a future CL.

Done.

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:665:
self._device.PullFile(source, dest, retries=0, timeout=240)
On 2017/02/21 16:11:16, jbudorick wrote:
> No retries?

I removed them because of the log spam, but sure, maybe it doesn't hurt to
retry.

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:674:
logging.warning('- %s', filepath)
On 2017/02/21 16:11:15, jbudorick wrote:
> On 2017/02/21 15:56:44, perezju wrote:
> > As stated in the comment, this fails pretty consistently on some paths; not
> sure
> > why. The changes are just to reduce the log spam, eliminate the unnecessary
> call
> > stack, and make it easier to see the list of paths that failed to be
> retrieved.
> 
> Does the exception text contain anything helpful?

Nope, it was actually kind of misleading:

  AdbCommandFailedError: (device: 06ab259b003be726) adb pull
/data/data/com.google.android.apps.chrome/files /tmp/tmpaRp3zm/files: failed and
output:
  - File not found on host: /tmp/tmpaRp3zm/files

Which sounds like it failed _because_ it couldn't find something on the host.
What it actually means is that it tried to Pull, and that seemed to work fine
(adb didn't complain), but the expected copy that should be now found on the
host is not there.

[jbudorick, 2017-02-21 16:38:07]

Re chown: I'd probably use -R on versions that support it & fall back to a
hand-rolled script implementation in other cases. You definitely would not want
one or more RunShellCommand invocations per individual chowned file.

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:674:
logging.warning('- %s', filepath)
On 2017/02/21 16:33:08, perezju wrote:
> On 2017/02/21 16:11:15, jbudorick wrote:
> > On 2017/02/21 15:56:44, perezju wrote:
> > > As stated in the comment, this fails pretty consistently on some paths;
not
> > sure
> > > why. The changes are just to reduce the log spam, eliminate the
unnecessary
> > call
> > > stack, and make it easier to see the list of paths that failed to be
> > retrieved.
> > 
> > Does the exception text contain anything helpful?
> 
> Nope, it was actually kind of misleading:
> 
>   AdbCommandFailedError: (device: 06ab259b003be726) adb pull
> /data/data/com.google.android.apps.chrome/files /tmp/tmpaRp3zm/files: failed
and
> output:
>   - File not found on host: /tmp/tmpaRp3zm/files
> 
> Which sounds like it failed _because_ it couldn't find something on the host.
> What it actually means is that it tried to Pull, and that seemed to work fine
> (adb didn't complain), but the expected copy that should be now found on the
> host is not there.

Hmm, good to know. Maybe a sign that we should rewrite the exception text for
that case.

[perezju, 2017-02-21 16:48:49]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 16:48:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[perezju, 2017-02-21 16:50:03]

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
File telemetry/telemetry/internal/platform/android_platform_backend.py (right):

https://codereview.chromium.org/2709523005/diff/20001/telemetry/telemetry/int...
telemetry/telemetry/internal/platform/android_platform_backend.py:674:
logging.warning('- %s', filepath)
On 2017/02/21 16:38:07, jbudorick wrote:
> On 2017/02/21 16:33:08, perezju wrote:
> > On 2017/02/21 16:11:15, jbudorick wrote:
> > > On 2017/02/21 15:56:44, perezju wrote:
> > > > As stated in the comment, this fails pretty consistently on some paths;
> not
> > > sure
> > > > why. The changes are just to reduce the log spam, eliminate the
> unnecessary
> > > call
> > > > stack, and make it easier to see the list of paths that failed to be
> > > retrieved.
> > > 
> > > Does the exception text contain anything helpful?
> > 
> > Nope, it was actually kind of misleading:
> > 
> >   AdbCommandFailedError: (device: 06ab259b003be726) adb pull
> > /data/data/com.google.android.apps.chrome/files /tmp/tmpaRp3zm/files: failed
> and
> > output:
> >   - File not found on host: /tmp/tmpaRp3zm/files
> > 
> > Which sounds like it failed _because_ it couldn't find something on the
host.
> > What it actually means is that it tried to Pull, and that seemed to work
fine
> > (adb didn't complain), but the expected copy that should be now found on the
> > host is not there.
> 
> Hmm, good to know. Maybe a sign that we should rewrite the exception text for
> that case.

Agreed.

[commit-bot: I haz the power, 2017-02-21 16:54:45]

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

[commit-bot: I haz the power, 2017-02-21 16:54:47]

Dry run: Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[perezju, 2017-02-21 17:37:40]

The CQ bit was checked by perezju@chromium.org

[perezju, 2017-02-21 17:37:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/2709523005/#ps80001
(title: "revert change in chown command")

[commit-bot: I haz the power, 2017-02-21 17:37:46]

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-02-21 17:42:12]

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

[commit-bot: I haz the power, 2017-02-21 17:42:13]

Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[perezju, 2017-02-27 09:09:51]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2017-02-27 09:10:02]

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-02-27 09:47:07]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488186591552980,
"parent_rev": "456b3a55214ef9a6b38f834d43d85501a8b9df92", "commit_rev":
"a75c463e8416e25de8c1d70ec210a1de89ea19a5"}

[commit-bot: I haz the power, 2017-02-27 09:47:11]

Description was changed from

==========
[Telemetry] Switch RunShellCommand clients to check_return=True

In all files where check_return was missing within /telemetry:
- enable check_return=True,
- pass the command as a list (rather than a string) where appropriate,
- replace calls to RunShellCommand with higher level commands (e.g.
  RemovePath) where appropriate.

BUG=chromium:628617
==========

to

==========
[Telemetry] Switch RunShellCommand clients to check_return=True

In all files where check_return was missing within /telemetry:
- enable check_return=True,
- pass the command as a list (rather than a string) where appropriate,
- replace calls to RunShellCommand with higher level commands (e.g.
  RemovePath) where appropriate.

BUG=chromium:628617

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

[commit-bot: I haz the power, 2017-02-27 09:47:12]

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