[devil] Raise DeviceUnreachableError on device not found

[devil] Raise DeviceUnreachableError on device not found

Raise a device_errors.DeviceUnreachableError when running an adb
command fails with: "error: device '(serial)' not found".

BUG=catapult:#3476

Review-Url: https://codereview.chromium.org/2808763004
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/2f8fce497d967c866ae866d36cdc08d2bfa30ff7

[perezju, 2017-04-11 13:38:16]

Not ready yet (still need to check some more of the try-except blocks), but
wanted to send out for early feedback.

Also a chromium-side CL will be needed to be prepared for these changes.

https://codereview.chromium.org/2808763004/diff/1/devil/devil/android/device_...
File devil/devil/android/device_test_case.py (right):

https://codereview.chromium.org/2808763004/diff/1/devil/devil/android/device_...
devil/devil/android/device_test_case.py:26:
device_errors.DeviceUnreachableError):
this seems to be a pretty common set of exceptions to catch, wondering whether
to add a common "DeviceError" as a superclass for all errors related to a
specific device.

However not all CommandFailedError's actually refer to a specific device (e.g.
adb start/kill-server), so these are not always strictly speaking "device"
errors?

Maybe the suggestion still works with a name other than "DeviceError" for the
superclass?

https://codereview.chromium.org/2808763004/diff/1/devil/devil/android/forward...
File devil/devil/android/forwarder.py (right):

https://codereview.chromium.org/2808763004/diff/1/devil/devil/android/forward...
devil/devil/android/forwarder.py:50: device_errors.DeviceUnreachableError):
these should also probably be catching CommandTimeoutError, so another +1 to my
suggestion above.

https://codereview.chromium.org/2808763004/diff/1/devil/devil/android/tools/d...
File devil/devil/android/tools/device_recovery.py (right):

https://codereview.chromium.org/2808763004/diff/1/devil/devil/android/tools/d...
devil/devil/android/tools/device_recovery.py:88:
device_errors.DeviceUnreachableError):
Here not sure if we want to add different reasons to the black list for each of
these two?

(and so on in the next couple of changes...)

[perezju, 2017-04-11 14:33:25]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 14:33:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[perezju, 2017-04-11 14:34:50]

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

[perezju, 2017-04-11 14:34:50]

Actually ready for review now.

(missed to add as reviewer on previous message)

[perezju, 2017-04-11 14:46:36]

Chromium side CL:
https://codereview.chromium.org/2811993002

[perezju, 2017-04-11 14:47:49]

Description was changed from

==========
[devil] Raise DeviceUnreachableError on device not found

Raise a device_errors.DeviceUnreachableError when running an adb
command fails with: "error: device '(serial)' not found'.

BUG=catapult:#3476
==========

to

==========
[devil] Raise DeviceUnreachableError on device not found

Raise a device_errors.DeviceUnreachableError when running an adb
command fails with: "error: device '(serial)' not found".

BUG=catapult:#3476
==========

[commit-bot: I haz the power, 2017-04-11 14:57:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 14:57:27]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2017-04-11 16:16:02]

https://codereview.chromium.org/2808763004/diff/20001/devil/devil/android/sdk...
File devil/devil/android/sdk/adb_wrapper.py (right):

https://codereview.chromium.org/2808763004/diff/20001/devil/devil/android/sdk...
devil/devil/android/sdk/adb_wrapper.py:261: if
_DEVICE_NOT_FOUND_RE.match(output):
Should this match on exiting w/ specifically "error: device <device_serial> not
found" rather than any serial?

[perezju, 2017-04-12 10:31:39]

https://codereview.chromium.org/2808763004/diff/20001/devil/devil/android/sdk...
File devil/devil/android/sdk/adb_wrapper.py (right):

https://codereview.chromium.org/2808763004/diff/20001/devil/devil/android/sdk...
devil/devil/android/sdk/adb_wrapper.py:261: if
_DEVICE_NOT_FOUND_RE.match(output):
On 2017/04/11 16:16:02, jbudorick wrote:
> Should this match on exiting w/ specifically "error: device <device_serial>
not
> found" rather than any serial?

Done.

[pasko, 2017-04-12 12:57:55]

For the commit message without looking at the actual change:

please make the BUG= a clickable link in this codereview. A link like this would
work for me:

BUG=https://github.com/catapult-project/catapult/issues/3476

If you have a common url shortener prefix in the catapult project, that would
work as well. Changing Rietveld would be less preferable :), since I also want
to be able to follow links in git log.

[pasko, 2017-04-12 13:00:34]

On 2017/04/12 12:57:55, pasko wrote:
> For the commit message without looking at the actual change:
> 
> please make the BUG= a clickable link in this codereview. A link like this
would
> work for me:
> 
> BUG=https://github.com/catapult-project/catapult/issues/3476
> 
> If you have a common url shortener prefix in the catapult project, that would
> work as well. Changing Rietveld would be less preferable :), since I also want
> to be able to follow links in git log.

oops, wanted to only post it in the chrome-side change, sorry for duplication.
Though I think a proper link would only make it better here.

[perezju, 2017-04-12 13:25:37]

On 2017/04/12 13:00:34, pasko wrote:
> On 2017/04/12 12:57:55, pasko wrote:
> > For the commit message without looking at the actual change:
> > 
> > please make the BUG= a clickable link in this codereview. A link like this
> would
> > work for me:
> > 
> > BUG=https://github.com/catapult-project/catapult/issues/3476
> > 
> > If you have a common url shortener prefix in the catapult project, that
would
> > work as well. Changing Rietveld would be less preferable :), since I also
want
> > to be able to follow links in git log.
> 
> oops, wanted to only post it in the chrome-side change, sorry for duplication.
> Though I think a proper link would only make it better here.

On catapult side CLs the format I used is the standard practice. As far as I
understand it's needed to match patches landed with issues in
https://github.com/catapult-project/catapult/issues/

[perezju, 2017-04-13 10:46:46]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 10:46:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 11:10:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 11:10:35]

Dry run: This issue passed the CQ dry run.

[jbudorick, 2017-04-17 21:38:22]

lgtm

[perezju, 2017-04-18 11:24:30]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2017-04-18 11:24:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 11:48:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492514670576910,
"parent_rev": "f15e8a5937fde19035306787e7a3b5676eaa434e", "commit_rev":
"2f8fce497d967c866ae866d36cdc08d2bfa30ff7"}

[commit-bot: I haz the power, 2017-04-18 11:48:19]

Description was changed from

==========
[devil] Raise DeviceUnreachableError on device not found

Raise a device_errors.DeviceUnreachableError when running an adb
command fails with: "error: device '(serial)' not found".

BUG=catapult:#3476
==========

to

==========
[devil] Raise DeviceUnreachableError on device not found

Raise a device_errors.DeviceUnreachableError when running an adb
command fails with: "error: device '(serial)' not found".

BUG=catapult:#3476

Review-Url: https://codereview.chromium.org/2808763004
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-04-18 11:48:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...