Reland of Migrate DeviceUtils.ReadFile to adb_wrapper (try 3)

Reland of Migrate DeviceUtils.ReadFile to adb_wrapper (try 3)

Original description:

The implementation is based on a simple 'cat' run by shell on the
device (with optional as_root).

Interface changes:
- The return value is a string (instead of a list of lines).
- An exception is raised if the file cannot be read
  (instead of returning an empty list).

Previous CLs:
- https://codereview.chromium.org/794583004/
- https://codereview.chromium.org/775333002/

BUG=267773
Committed: https://crrev.com/7138148b7108a41bf44a39e8aa52e5954d2c0f52
Cr-Commit-Position: refs/heads/master@{#313055}

[perezju, 2014-12-15 14:57:10]

perezju@chromium.org changed reviewers:
+ jbudorick@chromium.org, tonyg@chromium.org

[perezju, 2014-12-15 14:57:11]

Here it goes the third attempt at landing the interface changes in
DeviceUtils.ReadFile.

The problem last time was caused because, when telemetry tried to read and save
the old contents of the Chrome command file, the file may not exist and under
the new interface this causes a CommandFailedError.

This error was confused with a potential error when _writing_ to the file, and
thus telemetry would abort running the test. This is now fixed in my latest
patch.

tonyg@chromium.org: skyostil@ reviewed the changes in the previous attempted
CL's, but he is now OOO for the holiday season. Could you please review the
changes in telemetry or suggest someone who could?

jbudorick@chromium.org: No new changes in build/android

https://codereview.chromium.org/806843002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/806843002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:236:
logging.critical('Cannot set Chrome command line: %s', str(exc))
Hopefully we won't need this again. But, just in case, gives a more useful error
message to diagnose any failures. For example, the breakage in the previous CL
would be reported as:

CRITICAL:root:Cannot set Chrome command line: (device: 02e584be08e628cb) adb
shell command 'cat /data/local/tmp/chrome-shell-command-line; echo %$?;' failed
with exit status 1 and output:
> /system/bin/sh: cat: /data/local/tmp/chrome-shell-command-line: No such file
or directory

CRITICAL:root:May be fixed by flashing to a userdebug build.

[tonyg, 2014-12-15 17:59:50]

Did the previous change break a unittest? If not, can you update this CL to
include a unittest that would have broken?

[jbudorick, 2014-12-15 23:09:54]

build/android/ lgtm

[perezju, 2014-12-30 13:10:15]

added the unittest requested

[tonyg, 2015-01-05 17:28:43]

https://codereview.chromium.org/806843002/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend_unittest.py
(right):

https://codereview.chromium.org/806843002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend_unittest.py:51:
device.WriteFile.assert_called_once_with(mock.ANY, mock.ANY, as_root=True)
I think these mock-style unittests are just closely coupling telemetry to the
underlying implementation without providing actual functional coverage. In other
words, this test verifies that certain commands were called, but doesn't verify
that they worked as intended. At this level, we should be testing behavior. For
instance: start a browser with a given command line, then use `ps` to verify
that the process has that command line. Or start a browser with a given command
line, then inspect the file system afterwards to verify the file was deleted.

[jbudorick, 2015-01-05 17:50:42]

https://codereview.chromium.org/806843002/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend_unittest.py
(right):

https://codereview.chromium.org/806843002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend_unittest.py:51:
device.WriteFile.assert_called_once_with(mock.ANY, mock.ANY, as_root=True)
On 2015/01/05 17:28:43, tonyg wrote:
> I think these mock-style unittests are just closely coupling telemetry to the
> underlying implementation without providing actual functional coverage. In
other
> words, this test verifies that certain commands were called, but doesn't
verify
> that they worked as intended. At this level, we should be testing behavior.
For
> instance: start a browser with a given command line, then use `ps` to verify
> that the process has that command line. Or start a browser with a given
command
> line, then inspect the file system afterwards to verify the file was deleted.

We've generally been switching to this style of test in our unit tests in
build/android/pylib/* to avoid having higher-level unit tests implicitly test
lower-level functionality (i.e., we verify the adb calls we make instead of the
results of those adb calls on an actual device). Our goals may be different,
though -- are telemetry's Android unit tests generally designed to run using a
real device? Should these be closer to functional tests than unit tests?

[tonyg, 2015-01-05 17:53:17]

On 2015/01/05 17:50:42, jbudorick wrote:
>
https://codereview.chromium.org/806843002/diff/40001/tools/telemetry/telemetr...
> File
>
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend_unittest.py
> (right):
> 
>
https://codereview.chromium.org/806843002/diff/40001/tools/telemetry/telemetr...
>
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend_unittest.py:51:
> device.WriteFile.assert_called_once_with(mock.ANY, mock.ANY, as_root=True)
> On 2015/01/05 17:28:43, tonyg wrote:
> > I think these mock-style unittests are just closely coupling telemetry to
the
> > underlying implementation without providing actual functional coverage. In
> other
> > words, this test verifies that certain commands were called, but doesn't
> verify
> > that they worked as intended. At this level, we should be testing behavior.
> For
> > instance: start a browser with a given command line, then use `ps` to verify
> > that the process has that command line. Or start a browser with a given
> command
> > line, then inspect the file system afterwards to verify the file was
deleted.
> 
> We've generally been switching to this style of test in our unit tests in
> build/android/pylib/* to avoid having higher-level unit tests implicitly test
> lower-level functionality (i.e., we verify the adb calls we make instead of
the
> results of those adb calls on an actual device). Our goals may be different,
> though -- are telemetry's Android unit tests generally designed to run using a
> real device? Should these be closer to functional tests than unit tests?

Right, I agree this style of test makes sense for build/android/pylib. But
Telemetry should be agnostic of pylib's implementation. Telemetry's tests are
designed to run against a real device.

[perezju, 2015-01-20 09:36:42]

Thanks both for the feedback.

After @ariblue's CL [1], separating the functionality to set/restore the command
line onto its own class, writing the required unittest got much easier.

[1]: https://codereview.chromium.org/811703007

[jbudorick, 2015-01-20 14:30:31]

https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py
(right):

https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py:49:
serial = adb_commands.GetAttachedDevices()[0]
Looks like this failed on the bot because it can't find adb?

https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py:69:
serial = adb_commands.GetAttachedDevices()[0]
(same)

[tonyg, 2015-01-20 17:14:26]

lgtm when the trybots are happy

https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py
(right):

https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py:49:
serial = adb_commands.GetAttachedDevices()[0]
On 2015/01/20 14:30:31, jbudorick wrote:
> Looks like this failed on the bot because it can't find adb?

I'd expect that we already have an adb_commands context somehow.

[perezju, 2015-01-22 16:41:53]

On 2015/01/20 17:14:26, tonyg wrote:
> lgtm when the trybots are happy
> 
>
https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
> File
>
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py
> (right):
> 
>
https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
>
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py:49:
> serial = adb_commands.GetAttachedDevices()[0]
> On 2015/01/20 14:30:31, jbudorick wrote:
> > Looks like this failed on the bot because it can't find adb?
> 
> I'd expect that we already have an adb_commands context somehow.

@tony: I've changed the unittest to be skipped if it fails to find an attached
device (which seems to be the case on some of the linux trybots). Do you think
this is reasonable?

Also, more or less randomly I picked some trybots which looked relevant and the
test is passing in many of them (except for linux_android_rel_ng [1] which seems
to be having issues recently). Do you have any suggestions for some more
reasonable trybots to check before trying to land?

[1]:
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...

[tonyg, 2015-01-25 19:18:47]

On 2015/01/22 16:41:53, perezju wrote:
> On 2015/01/20 17:14:26, tonyg wrote:
> > lgtm when the trybots are happy
> > 
> >
>
https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
> > File
> >
>
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/806843002/diff/80001/tools/telemetry/telemetr...
> >
>
tools/telemetry/telemetry/core/backends/android_command_line_backend_unittest.py:49:
> > serial = adb_commands.GetAttachedDevices()[0]
> > On 2015/01/20 14:30:31, jbudorick wrote:
> > > Looks like this failed on the bot because it can't find adb?
> > 
> > I'd expect that we already have an adb_commands context somehow.
> 
> @tony: I've changed the unittest to be skipped if it fails to find an attached
> device (which seems to be the case on some of the linux trybots). Do you think
> this is reasonable?

Instead, just add @benchmark.Enabled('android') to those tests. W/ that this
still lgtm.

> 
> Also, more or less randomly I picked some trybots which looked relevant and
the
> test is passing in many of them (except for linux_android_rel_ng [1] which
seems
> to be having issues recently). Do you have any suggestions for some more
> reasonable trybots to check before trying to land?

Just check the commit box and let it go through the default set.

> 
> [1]:
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...

[perezju, 2015-01-26 09:10:04]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2015-01-26 09:11:03]

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

[commit-bot: I haz the power, 2015-01-26 10:10:28]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-26 10:11:24]

Patchset 7 (id:??) landed as
https://crrev.com/7138148b7108a41bf44a39e8aa52e5954d2c0f52
Cr-Commit-Position: refs/heads/master@{#313055}