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

RESOLVED FIXED in mozilla35

Status

Testing
Mozbase
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jgraham, Assigned: jgraham)

Tracking

unspecified
mozilla35
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
+++ 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
(Assignee)

Comment 1

3 years ago
Created attachment 8470174 [details] [diff] [review]
Improve specification of device serial in mozdevice.adb
Attachment #8470174 - Flags: review?(bclary)
(Assignee)

Comment 2

3 years ago
Created attachment 8472407 [details] [diff] [review]
Improve specification of device serial in mozdevice.adb

Updated with ContextManager patch removed.
(Assignee)

Updated

3 years ago
Attachment #8470174 - Attachment is obsolete: true
Attachment #8470174 - Flags: review?(bclary)
(Assignee)

Updated

3 years ago
Attachment #8472407 - Flags: review?(bclary)

Comment 3

3 years ago
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.
Attachment #8472407 - Flags: review?(bclary) → review+

Comment 4

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

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8475314 [details] [diff] [review]
Improve specification of device serial in mozdevice.adb

It seems like this bug had an old version of the patch attached.
Attachment #8472407 - Attachment is obsolete: true
Attachment #8475314 - Flags: review?(bclary)

Comment 7

3 years ago
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")
Attachment #8475314 - Flags: review?(bclary) → review+
(Assignee)

Comment 8

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

Updated

3 years ago
Blocks: 1062371
https://hg.mozilla.org/mozilla-central/rev/9f9db3aa3c64
Assignee: nobody → james
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.