Make device serial required in AdbWrapper/DeviceUtils

build/android/pylib/device/device_utils.py

Index: build/android/pylib/device/device_utils.py
diff --git a/build/android/pylib/device/device_utils.py b/build/android/pylib/device/device_utils.py
index 272e613b6817f915a2cee039caf6ff53de13bea7..f23e1eaee5b5c2b62d42cba656149f2d74bff8b2 100644
--- a/build/android/pylib/device/device_utils.py
+++ b/build/android/pylib/device/device_utils.py
@@ -65,8 +65,8 @@ class DeviceUtils(object):
     """DeviceUtils constructor.
 
     Args:
-      device: Either a device serial, an existing AdbWrapper instance, an
-              an existing AndroidCommands instance, or nothing.
+      device: Either a device serial, an existing AdbWrapper instance, or an
+              an existing AndroidCommands instance.
       default_timeout: An integer containing the default number of seconds to
                        wait for an operation to complete if no explicit value
                        is provided.
@@ -85,11 +85,8 @@ class DeviceUtils(object):
     elif isinstance(device, pylib.android_commands.AndroidCommands):
       self.adb = adb_wrapper.AdbWrapper(device.GetDevice())
       self.old_interface = device
-    elif not device:
-      self.adb = adb_wrapper.AdbWrapper('')
-      self.old_interface = pylib.android_commands.AndroidCommands()
     else:
-      raise ValueError('Unsupported type passed for argument "device"')
+      raise ValueError('Unsupported device value: %r' % device)

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

a bit more informative when passing the wrong value

     self._commands_installed = None
     self._default_timeout = default_timeout
     self._default_retries = default_retries
@@ -1132,12 +1129,12 @@ class DeviceUtils(object):
 
   def __str__(self):
     """Returns the device serial."""
-    s = self.old_interface.GetDevice()
-    if not s:
-      s = self.old_interface.Adb().GetSerialNumber()
-      if s == 'unknown':
-        raise device_errors.NoDevicesError()
-    return s
+    return str(self.adb)

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

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]

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

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

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

[jbudorick, 2014/12/01 15:14:23]

I can agree with this.
If we don't care about whether adb.GetDeviceSerial is called...

+
+  @property
+  def device_serial(self):

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

This is entirely unnecessary between __str__ and AdbWrapper.GetDeviceSerial()

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

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:23]

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.

+    """Returns the device serial."""
+    return str(self.adb)
 
   @staticmethod
   def parallel(devices=None, async=False):