Make device serial required in AdbWrapper/DeviceUtils

Make device serial required in AdbWrapper/DeviceUtils

As a side effect, we also:
- help migrate DeviceUtils.__str__ to AdbWrapper
- fix bug with the presubmit pylib.instrumentation.tests_runner_test
  when no devices are attached.

BUG=267773, 430731
Committed: https://crrev.com/d10fd290b1ea2091e9fbaf0e2785b09313e0a7af
Cr-Commit-Position: refs/heads/master@{#306585}

[perezju, 2014-11-26 17:53:54]

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

[perezju, 2014-11-26 17:53:55]

For when you have some time to take a look.

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:89: raise ValueError('Unsupported
device value: %r' % device)
a bit more informative when passing the wrong value

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1132: return str(self.adb)
note: maybe self.GetDeviceSerial() might look a bit better here, but that messes
up lots of test because now an unexpected method call is made, e.g., just when
raising an exception. Private methods are not watched, so we won't need to
change any tests.

[jbudorick, 2014-11-27 17:01:58]

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1132: return str(self.adb)
On 2014/11/26 17:53:55, perezju wrote:
> note: maybe self.GetDeviceSerial() might look a bit better here, but that
messes
> up lots of test because now an unexpected method call is made, e.g., just when

FWIW, "messing up a lot of tests" isn't a great motivation for a implementation
decision.

> raising an exception. Private methods are not watched, so we won't need to
> change any tests.

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1135: def device_serial(self):
This is entirely unnecessary between __str__ and AdbWrapper.GetDeviceSerial()

[perezju, 2014-11-28 16:06:22]

also just replying to comments here

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1132: return str(self.adb)
On 2014/11/27 17:01:58, jbudorick wrote:
> On 2014/11/26 17:53:55, perezju wrote:
> > note: maybe self.GetDeviceSerial() might look a bit better here, but that
> messes
> > up lots of test because now an unexpected method call is made, e.g., just
when
> 
> FWIW, "messing up a lot of tests" isn't a great motivation for a
implementation
> decision.
> 
> > raising an exception. Private methods are not watched, so we won't need to
> > change any tests.
> 

Let me rephrase that. I don't think we normally want str(self) to introduce
calls to expect when we do assertCalls in tests. Because in most cases the
correctness of a test does not depend on how many times we had to retrieve the
string with the device serial.

So, there are two alternatives here: use str(self.adb) as I did here, or somehow
add an exception to device_utils_test and make adb.GetDeviceSerial unwatched.
What would you think is better?

In either case, if we really want to watch any of these methods for a particular
test, we can always easily add it "on the fly" because mock_calls is great. :)
:P

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1135: def device_serial(self):
On 2014/11/27 17:01:58, jbudorick wrote:
> This is entirely unnecessary between __str__ and AdbWrapper.GetDeviceSerial()

Unnecessary but more readable and explicit, for when we want the code to state
"I really want the device serial (and no other string representation which you
might think is useful)". I thought that's the reason why AdbWrapper *also* has
both a __str__ and a GetDeviceSerial method that do the same thing.

It's also more concise to write device.device_serial, rather than
device.adb.GetDeviceSerial() [which also feels like trying to reach into the low
level details of a device].

Anyway, (as you see) I have lots of opinions here, but not a really strong
preference. So let me know what you think.

[jbudorick, 2014-12-01 15:14:24]

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1132: return str(self.adb)
On 2014/11/28 16:06:22, perezju wrote:
> On 2014/11/27 17:01:58, jbudorick wrote:
> > On 2014/11/26 17:53:55, perezju wrote:
> > > note: maybe self.GetDeviceSerial() might look a bit better here, but that
> > messes
> > > up lots of test because now an unexpected method call is made, e.g., just
> when
> > 
> > FWIW, "messing up a lot of tests" isn't a great motivation for a
> implementation
> > decision.
> > 
> > > raising an exception. Private methods are not watched, so we won't need to
> > > change any tests.
> > 
> 
> Let me rephrase that. I don't think we normally want str(self) to introduce
> calls to expect when we do assertCalls in tests. Because in most cases the
> correctness of a test does not depend on how many times we had to retrieve the
> string with the device serial.

I can agree with this.

> 
> So, there are two alternatives here: use str(self.adb) as I did here, or
somehow
> add an exception to device_utils_test and make adb.GetDeviceSerial unwatched.
> What would you think is better?

If we don't care about whether adb.GetDeviceSerial is called...

> 
> In either case, if we really want to watch any of these methods for a
particular
> test, we can always easily add it "on the fly" because mock_calls is great. :)
> :P

https://codereview.chromium.org/762883002/diff/1/build/android/pylib/device/d...
build/android/pylib/device/device_utils.py:1135: def device_serial(self):
On 2014/11/28 16:06:22, perezju wrote:
> On 2014/11/27 17:01:58, jbudorick wrote:
> > This is entirely unnecessary between __str__ and
AdbWrapper.GetDeviceSerial()
> 
> Unnecessary but more readable and explicit, for when we want the code to state
> "I really want the device serial (and no other string representation which you
> might think is useful)". I thought that's the reason why AdbWrapper *also* has
> both a __str__ and a GetDeviceSerial method that do the same thing.
> 
> It's also more concise to write device.device_serial, rather than
> device.adb.GetDeviceSerial() [which also feels like trying to reach into the
low
> level details of a device].
> 
> Anyway, (as you see) I have lots of opinions here, but not a really strong
> preference. So let me know what you think.

I really don't like duplicating functionality between DeviceUtils and
AdbWrapper. We need to provide __str__ for both, but I would prefer not adding
this property.

[perezju, 2014-12-02 11:13:18]

ptal

https://codereview.chromium.org/762883002/diff/40001/build/android/pylib/devi...
File build/android/pylib/device/device_utils_test.py (right):

https://codereview.chromium.org/762883002/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils_test.py:251:
self.watchMethodCalls(self.call.adb, exclude=['GetDeviceSerial'])
GetDeviceSerial is now excluded from the methods watched during assertCalls

https://codereview.chromium.org/762883002/diff/40001/build/android/pylib/devi...
build/android/pylib/device/device_utils_test.py:1561:
(self.call.adb.GetDeviceSerial(), '0123456789abcdef')):
... except if we really want to watch it

https://codereview.chromium.org/762883002/diff/40001/build/android/pylib/inst...
File build/android/pylib/instrumentation/test_runner_test.py (right):

https://codereview.chromium.org/762883002/diff/40001/build/android/pylib/inst...
build/android/pylib/instrumentation/test_runner_test.py:231: return_value=None)
This line is what fixes BUG 430731

[jbudorick, 2014-12-02 15:31:25]

lgtm w/ nit

https://codereview.chromium.org/762883002/diff/40001/build/android/pylib/util...
File build/android/pylib/utils/mock_calls.py (right):

https://codereview.chromium.org/762883002/diff/40001/build/android/pylib/util...
build/android/pylib/utils/mock_calls.py:125: def watchMethodCalls(self, call,
exclude=None):
nit: "exclude" -> "ignore"

[perezju, 2014-12-03 10:15:15]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2014-12-03 10:15:35]

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

[commit-bot: I haz the power, 2014-12-03 11:22:55]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-03 11:23:46]

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