Closed Bug 1484238 Opened Last year Closed Last year

Add an 'adb_reverse' command to mozdevice.ADBAndroid

Categories

(Testing :: Mozbase, enhancement, P3)

Version 3
enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: rwood, Assigned: egao)

References

Details

Attachments

(1 file, 5 obsolete files)

Raptor uses 'adb reverse' so it's webext can talk to the control server, and so the geckoview app on the device can access the benchmarks running on localhost on the host machine.

Instead of using 'adb reverse' from within raptor directly, an 'adb_reverse' command should be added to mozdevice.ADBAndroid.
Blocks: 1473078
Depends on: 1480841
Bikeshedding for a moment.... adb_reverse seems like a redundant name considering where it will live. I would go with just 'reverse' but I'm ok with adb_reverse if you think that is more understandable as to its use.

adb reverse doesn't require any explicit android functionality on the device does it? I would think we could put it in ADBDevice where it could be reused by anyone still using the adb_b2g stuff. It doesn't really matter if we don't care about the b2g stuff anymore and I'm sure you would like to put that in the past as well. ;-)
egao: I think this will be a good bug to work on. the forward command is very similar and you can see how it is implemented in https://searchfox.org/mozilla-central/source/testing/mozbase/mozdevice/mozdevice/adb.py#907
Priority: -- → P3
I've committed proposed additions of the 'reverse' method to adb.py.

Method is heavily inspired and taken from the 'forward' method.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7e979612b672c614c69c0ad9ecc11d67145a90f
Upon closer inspection, I realized I was not iterating through a list of tuples signifying if the port is local or remote.

Try run for the fixed segment of code:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=603180c3f3faf206b911aed272bb4a7e7681a3d3
Attached patch reverse_mozdevice_android.patch (obsolete) — Splinter Review
Patch attached, includes correction to the for loop compared to the first submission.
Attachment #9005040 - Flags: review?(bob)
Assignee: nobody → egao
Comment on attachment 9005040 [details] [diff] [review]
reverse_mozdevice_android.patch

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

Looks good as far as it goes. Let's complete the methods to fully implement adb reverse.

::: testing/mozbase/mozdevice/mozdevice/adb.py
@@ +1050,3 @@
>  
>          self.command_output(cmd, timeout=timeout)
>  

add a comment to show this is distinct from the forwarding methods...

    # Port reversing methods

@@ +1071,5 @@
> +        cmd = ["reverse", local, remote]
> +        if not allow_rebind:
> +            cmd.insert(1, "--no-rebind")
> +        self.command_output(cmd, timeout=timeout)
> +

complete the methods by adding list_reverses and remove_reverses.
Attachment #9005040 - Flags: review?(bob)
(In reply to Bob Clary [:bc:] from comment #7)
> add a comment to show this is distinct from the forwarding methods...
> 
>     # Port reversing methods
> 

Comment added. Noted in an one-liner comment for both forward and reverse what they do.

> complete the methods by adding list_reverses and remove_reverses.

Implemented request:
- list_reverses
- remove_reverses

Code is essentially identical to the list_forwards and remove_forwards methods; I believe there is room to refactor this into one method in the near future, something akin to remove_ports and list_ports (naming TBD).


-----------------------------------------------------------------

In addition, the latest patch includes corrections to the docstring that referenced the methods where it was copied from (forward). These instances have been fixed to refer to 'reverse'.
Heavily inspired by the forward() method present in same file.
Fixed a mistake where for loop was iterating through a list of strings, not a tuple as forward does.

Depends on D4716
Added list_reverses and remove_reverses commands.
Corrected mistakes in the docstring.

Depends on D4717
Attachment #9005456 - Attachment is obsolete: true
Attachment #9005457 - Attachment is obsolete: true
Attachment #9005458 - Attachment is obsolete: true
Comment on attachment 9005656 [details]
Bug 1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid r?bc

Bob Clary [:bc:] has approved the revision.
Attachment #9005656 - Flags: review+
Attachment #9005286 - Attachment is obsolete: true
Attachment #9005286 - Flags: review?(bob)
Attachment #9005040 - Attachment is obsolete: true
Blocks: 1488327
Comment on attachment 9005656 [details]
Bug 1484238 - Add an 'adb_reverse' command to mozdevice.ADBAndroid r?bc

Geoff Brown [:gbrown] has approved the revision.
Attachment #9005656 - Flags: review+
Pushed by jmaher@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ea388fd649a
Add an 'adb_reverse' command to mozdevice.ADBAndroid r=gbrown,bc
https://hg.mozilla.org/mozilla-central/rev/1ea388fd649a
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.