[Android] Remove more uses of android_commands from build/android/pylib.

[Android] Remove more uses of android_commands from build/android/pylib.

BUG=267773
Committed: https://crrev.com/166b709fc6c8fefad72520473850ce15ffe45898
Cr-Commit-Position: refs/heads/master@{#327402}

[jbudorick, 2015-04-28 02:16:42]

jbudorick@chromium.org changed reviewers:
+ mikecase@chromium.org, perezju@chromium.org, rnephew@chromium.org

[perezju, 2015-04-28 10:33:51]

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/base/te...
File build/android/pylib/base/test_dispatcher_unittest.py (right):

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/base/te...
build/android/pylib/base/test_dispatcher_unittest.py:31: def
_CreateMockDevice(serial):
wait, this is creating a _real_ DeviceUtils. Should it better return a mock with
spec based on the DeviceUtils class? Also maybe name it just _MockDevice or
_MockDeviceUtils.

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/base/te...
build/android/pylib/base/test_dispatcher_unittest.py:39: def __init__(self,
device=_CreateMockDevice('0'), shard_index=0):
I think this should be device=None, and then self.device = device or
_CreateMockDevice('0')

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/base/te...
build/android/pylib/base/test_dispatcher_unittest.py:68: def __init__(self,
device=_CreateMockDevice('0'), shard_index=0):
same here

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/device/...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/device/...
build/android/pylib/device/adb_wrapper.py:545: def Emulator(self, cmd,
timeout=_DEFAULT_TIMEOUT,
not sure if I like my own suggestion, but we've been naming these matching the
name of the corresponding adb command, so should be adb.Emu?

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/utils/e...
File build/android/pylib/utils/emulator.py (right):

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/utils/e...
build/android/pylib/utils/emulator.py:99: if not [d for d in
device_utils.HealthyDevices() if d.adb.is_emulator]:
if not any(d.adb.is_emulator for d in device_utils.HealthyDevices()):

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

https://codereview.chromium.org/1105323002/diff/1/build/android/test_runner.p...
build/android/test_runner.py:880: % (test_device, ', '.join(attached_devices)))
Hmm... Just a thought. By virtue of our definitions of __eq__ and __str__ in
AdbWrapper, an adb_wrapper instance also behaves as it's own string serial when
appropriate, so all of the following tests do work as expected:

- str_serial == adb_instance
- adb_instance == str_serial
- adb_instance_1 == adb_instance_2
- str_serial in list_of_adb_instances
- adb_instance in list_of_str_serials

We may want to do something similar for DeviceUtils (basically adding an __eq__
that compares with str of other), so the inner block here could just become:

if test_device in attached_devices:
  attached_devices = [device_utils.DeviceUtils(test_device)]
else:
  raise ...

wdyt?

https://codereview.chromium.org/1105323002/diff/1/build/android/test_runner.p...
build/android/test_runner.py:885: return sorted(attached_devices, key=lambda d:
d.adb.GetDeviceSerial())
... and if we define __lt__, we also get |sorted| working out of the box.

[jbudorick, 2015-04-28 13:44:59]

ptal

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/base/te...
File build/android/pylib/base/test_dispatcher_unittest.py (right):

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/base/te...
build/android/pylib/base/test_dispatcher_unittest.py:31: def
_CreateMockDevice(serial):
On 2015/04/28 10:33:50, perezju wrote:
> wait, this is creating a _real_ DeviceUtils. Should it better return a mock
with
> spec based on the DeviceUtils class? Also maybe name it just _MockDevice or
> _MockDeviceUtils.

Done.

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/base/te...
build/android/pylib/base/test_dispatcher_unittest.py:39: def __init__(self,
device=_CreateMockDevice('0'), shard_index=0):
On 2015/04/28 10:33:50, perezju wrote:
> I think this should be device=None, and then self.device = device or
> _CreateMockDevice('0')

Done.

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/base/te...
build/android/pylib/base/test_dispatcher_unittest.py:68: def __init__(self,
device=_CreateMockDevice('0'), shard_index=0):
On 2015/04/28 10:33:50, perezju wrote:
> same here

Done.

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/device/...
File build/android/pylib/device/adb_wrapper.py (right):

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/device/...
build/android/pylib/device/adb_wrapper.py:545: def Emulator(self, cmd,
timeout=_DEFAULT_TIMEOUT,
On 2015/04/28 10:33:50, perezju wrote:
> not sure if I like my own suggestion, but we've been naming these matching the
> name of the corresponding adb command, so should be adb.Emu?

I don't like the name, but you're right that we've been naming these to match
... consistency wins. Emu it is.

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/utils/e...
File build/android/pylib/utils/emulator.py (right):

https://codereview.chromium.org/1105323002/diff/1/build/android/pylib/utils/e...
build/android/pylib/utils/emulator.py:99: if not [d for d in
device_utils.HealthyDevices() if d.adb.is_emulator]:
On 2015/04/28 10:33:51, perezju wrote:
> if not any(d.adb.is_emulator for d in device_utils.HealthyDevices()):

Done.

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

https://codereview.chromium.org/1105323002/diff/1/build/android/test_runner.p...
build/android/test_runner.py:880: % (test_device, ', '.join(attached_devices)))
On 2015/04/28 10:33:51, perezju wrote:
> Hmm... Just a thought. By virtue of our definitions of __eq__ and __str__ in
> AdbWrapper, an adb_wrapper instance also behaves as it's own string serial
when
> appropriate, so all of the following tests do work as expected:
> 
> - str_serial == adb_instance
> - adb_instance == str_serial
> - adb_instance_1 == adb_instance_2
> - str_serial in list_of_adb_instances
> - adb_instance in list_of_str_serials
> 
> We may want to do something similar for DeviceUtils (basically adding an
__eq__
> that compares with str of other), so the inner block here could just become:
> 
> if test_device in attached_devices:
>   attached_devices = [device_utils.DeviceUtils(test_device)]
> else:
>   raise ...
> 
> wdyt?

I implemented __eq__ to support this behavior, but it winds up a little weird...
in order to get the 'serial in list_of_devices' functionality, we have to
compare a basestring and a DeviceUtils for equality.

I wound up implementing the part in here a little differently.

https://codereview.chromium.org/1105323002/diff/1/build/android/test_runner.p...
build/android/test_runner.py:885: return sorted(attached_devices, key=lambda d:
d.adb.GetDeviceSerial())
On 2015/04/28 10:33:51, perezju wrote:
> ... and if we define __lt__, we also get |sorted| working out of the box.

Done.

[perezju, 2015-04-28 14:36:19]

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

https://codereview.chromium.org/1105323002/diff/1/build/android/test_runner.p...
build/android/test_runner.py:880: % (test_device, ', '.join(attached_devices)))
On 2015/04/28 13:44:59, jbudorick wrote:
> On 2015/04/28 10:33:51, perezju wrote:
> > Hmm... Just a thought. By virtue of our definitions of __eq__ and __str__ in
> > AdbWrapper, an adb_wrapper instance also behaves as it's own string serial
> when
> > appropriate, so all of the following tests do work as expected:
> > 
> > - str_serial == adb_instance
> > - adb_instance == str_serial
> > - adb_instance_1 == adb_instance_2
> > - str_serial in list_of_adb_instances
> > - adb_instance in list_of_str_serials
> > 
> > We may want to do something similar for DeviceUtils (basically adding an
> __eq__
> > that compares with str of other), so the inner block here could just become:
> > 
> > if test_device in attached_devices:
> >   attached_devices = [device_utils.DeviceUtils(test_device)]
> > else:
> >   raise ...
> > 
> > wdyt?
> 
> I implemented __eq__ to support this behavior, but it winds up a little
weird...
> in order to get the 'serial in list_of_devices' functionality, we have to
> compare a basestring and a DeviceUtils for equality.

I think that should "just work". I did a bit of testing and when doing
string_serial == device (or when doing string_serial in [device]) this is
eventually turned into device.__eq__(string_serial).

From what I gather from the documentation: if the lhs of the equality is a
built-in, and the rhs is a class; the __eq__ of the class is tried first.
https://docs.python.org/2/reference/datamodel.html#coercion-rules

https://codereview.chromium.org/1105323002/diff/20001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1105323002/diff/20001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:187: if isinstance(other,
basestring):
why not just do like we do in AdbWrapper, i.e.:

  return self.adb.GetDeviceSerial() == str(other)

https://codereview.chromium.org/1105323002/diff/20001/build/android/pylib/per...
File build/android/pylib/perf/setup.py (right):

https://codereview.chromium.org/1105323002/diff/20001/build/android/pylib/per...
build/android/pylib/perf/setup.py:30: return sorted(devices, key=lambda d:
d.adb.GetDeviceSerial())
we can also drop the sorting key from here. :)

[jbudorick, 2015-04-28 15:52:53]

https://codereview.chromium.org/1105323002/diff/20001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/1105323002/diff/20001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:187: if isinstance(other,
basestring):
On 2015/04/28 14:36:19, perezju wrote:
> why not just do like we do in AdbWrapper, i.e.:
> 
>   return self.adb.GetDeviceSerial() == str(other)

Wow, that's much better. Done.

https://codereview.chromium.org/1105323002/diff/20001/build/android/pylib/per...
File build/android/pylib/perf/setup.py (right):

https://codereview.chromium.org/1105323002/diff/20001/build/android/pylib/per...
build/android/pylib/perf/setup.py:30: return sorted(devices, key=lambda d:
d.adb.GetDeviceSerial())
On 2015/04/28 14:36:19, perezju wrote:
> we can also drop the sorting key from here. :)

oops. done.

[perezju, 2015-04-28 16:32:37]

lgtm

[jbudorick, 2015-04-28 22:18:03]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-04-28 22:19:50]

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

[commit-bot: I haz the power, 2015-04-29 00:26:14]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-29 00:27:12]

Patchset 3 (id:??) landed as
https://crrev.com/166b709fc6c8fefad72520473850ce15ffe45898
Cr-Commit-Position: refs/heads/master@{#327402}

[aberent, 2015-04-29 11:06:55]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1116493003/ by aberent@chromium.org.

The reason for reverting is: Causes HostDriven_SigninTest.testManagedSignin to
fail, and hence prevents rolls into Android downstream repository

BUG=482406.