[mozdevice] do not repeat wait-for-device in adb commands

RESOLVED FIXED in Firefox 45

Status

Testing
Mozbase
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bc, Assigned: bc)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed)

Details

Attachments

(2 attachments)

Created attachment 8679663 [details] [diff] [review]
bug-1214530.patch

In bug 1214530 when attempting to work around issues on the Autophone mac mini host, I upgraded adb to the most recent version (still marked as 1.0.32) which caused syntax errors due to the way the adb.py shell commands are implemented.

The problem was in the shell method:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozdevice/mozdevice/adb.py#1013

where I automatically prepended a wait-for-device to the adb command. Other parts of adb.py used shell_command to issue a wait-for-device which called shell() indirectly and resulted in command lines of the form adb -s <serial> wait-for-device wait-for-device ....

I've known about this duplication for a while but since it wasn't causing problems, left it alone. But with the upgraded adb version, repeating wait-for-device on the command line resulted in an error.

The solution I used was to not issue wait-for-device in shell_command. To perform a simple wait, I just did a self.command_output([], timeout=timeout).

This patch is a version of the bug 1214530 patch updated to account for bug 1145680 moving the reboot method from adb_android.py to adb.py.
Attachment #8679663 - Flags: review?(gbrown)
Attachment #8679663 - Flags: feedback?(wlachance)
Submitted to try
try: -b o -p android-api-9,android-api-11,android-x86,emulator,linux64_gecko -u all -t none
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c174b59b32bb
Comment on attachment 8679663 [details] [diff] [review]
bug-1214530.patch

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

command_output([]) looks funny and might be mistaken for a no-op -- maybe an in-line comment is warranted?
Attachment #8679663 - Flags: review?(gbrown) → review+
try was broken due to leaving a file out of dave's patch.
new try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab2e418e2020


(In reply to Geoff Brown [:gbrown] from comment #2)
> 
> command_output([]) looks funny and might be mistaken for a no-op -- maybe an
> in-line comment is warranted?

Ok. Sounds good.
Created attachment 8679786 [details] [diff] [review]
bug-1214530.patch v2 with comments

Do you think it would be better to just add a new method wait_for_device() ?
I think it would be fine either way.
Attachment #8679663 - Flags: feedback?(wlachance)
Comment on attachment 8679786 [details] [diff] [review]
bug-1214530.patch v2 with comments

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

::: testing/mozbase/mozdevice/mozdevice/adb_android.py
@@ +78,5 @@
>          :raises: * ADBTimeoutError
>                   * ADBError
>          """
> +        # Issue an empty command which effectively performs
> +        # a wait-for-device.

It would be good to explain *why* you're not doing wait-for-device here.
Attachment #8679786 - Flags: feedback+

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4ae3cda0b5ff
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.