1050896 - [mozdevice.adb] Make ADBDevice's device_serial parameter more intelligent

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[James Graham [:jgraham]]

+++ This bug was initially created as a clone of Bug #1048881 +++

The current thinking seems to be:

* Allow a string or a dict of properties
* Detect known-brokenness e.g. inaris with : in the serial
* Make the behaviour with None safer so it checks for a specific device

Comment 1

[James Graham [:jgraham]]

Attachment: Improve specification of device serial in mozdevice.adb

Comment 2

[James Graham [:jgraham]]

Attachment: Improve specification of device serial in mozdevice.adb

Updated with ContextManager patch removed.

Comment 3

[Bob Clary [:bc] (inactive)]

Comment on attachment 8472407 [details] [diff] [review]
Improve specification of device serial in mozdevice.adb

Review of attachment 8472407 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +463,5 @@
> +        :param device: Either a dictionary as returned from ADBHost.devices()
> +            or a string representing the adb serial number of the device.
> +            In the case that the serial number contains a ":" character the
> +            string form won't work. If None is passed and there is exactly
> +            one device connected to the host, that device will be used.

This doesn't quite detail the behavior. Can we work all of the following into the comment? It doesn't have to be identical or word for word, just all of the details.

In the event only a single device is attached, device can be None in which case the single dictionary item from the list returned by ADBHost.devices() will be used. It is an error to specify device is None if more than one device is attached.

If device is a string, it must be either be the adb serial number of the device without ':' or the usb path. If your device serial number naturally contains a ':', use the dictionary form of device instead of a string.

If device is a dictionary, it must be one item for a single device of the form from the list returned by ADBHost().devices() instead of the full output of devices(). The dictionary should have at least one of 'usb' or 'device_serial' attributes. If the dictionary does not contain a device_serial attribute, the usb path "usb:%s" device['usb'] will be used.

@@ +473,5 @@
>          :param device_ready_retry_attempts: number of attempts when
>              checking if a device is ready.
>  
>          :raises: * ADBError
>                   * ADBTimeoutError

* ValueError if the serial is not in the correct format or if no device can be found.

Comment 4

[Bob Clary [:bc] (inactive)]

Oh. forgot to mention the whitespace error on line 70 in the patch.

Comment 5

[Bob Clary [:bc] (inactive)]

Comment on attachment 8472407 [details] [diff] [review]
Improve specification of device serial in mozdevice.adb

Review of attachment 8472407 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +529,5 @@
>  
>          self._logger.debug("ADBDevice: %s" % self.__dict__)
>  
>          self._setup_test_root()
> + 

please clean trailing whitespace.

Comment 6

[James Graham [:jgraham]]

Attachment: Improve specification of device serial in mozdevice.adb

It seems like this bug had an old version of the patch attached.

Comment 7

[Bob Clary [:bc] (inactive)]

Comment on attachment 8475314 [details] [diff] [review]
Improve specification of device serial in mozdevice.adb

Review of attachment 8475314 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +460,5 @@
>                   device_ready_retry_attempts=3):
>          """Initializes the ADBDevice object.
>  
> +        :param device: can be either a dictionary, a string or None.
> +            Wehn a string is passed, it is interpreted as the

When

@@ +547,5 @@
> +    def _get_device_serial(self, device):
> +        if device is None:
> +            devices = ADBHost().devices()
> +            if len(devices) > 1:
> +                raise ValueError("ADBDevice called with multiple devices attached and no device specified")

please split this to be no longer than 80.

                raise ValueError("ADBDevice called with multiple devices "
                                 "attached and no device specified")

@@ +560,5 @@
> +
> +        if isinstance(device, (str, unicode)):
> +            # Treat this as a device serial
> +            if not is_valid_serial(device):
> +                raise ValueError("Device serials containing ':' characters are invalid. Pass the output from ADBHost.devices() for the device instead")

ditto

                raise ValueError("Device serials containing ':' characters are "
                                 "invalid. Pass the output from "
                                 "ADBHost.devices() for the device instead")

Comment 8

[James Graham [:jgraham]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f9db3aa3c64

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f9db3aa3c64