Last Comment Bug 792048 - add support for mDNS to mozdevice
: add support for mDNS to mozdevice
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Mozbase (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-18 07:47 PDT by Vladimir Vukicevic [:vlad] [:vladv]
Modified: 2012-10-10 11:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wget https://github.com/mozilla/mozbase/pull/35.diff (63.34 KB, patch)
2012-10-10 11:10 PDT, Jeff Hammel
wlachance: review+
Details | Diff | Review

Description Vladimir Vukicevic [:vlad] [:vladv] 2012-09-18 07:47:50 PDT
[Note: I know pull requests aren't great, but in this case I've got 9 patches in as separate commits -- 8 of them are just largely trivial misc fixes, and 1 adds mDNS support.  I'd rather keep them separate for easier reviewing than mashing everything together (even mashing them into 2 patches), so I'll post a pull request here shortly (chicken-and-egg problem -- I need the bug # to reference in the pull request, and I need the pull request to reference here!)

If whoever ends up reviewing would really, really rather see things squashed into two patches, I can do that as well.]

This adds Droid.DroidConnectByHWID to mozdevice, which will first try to connect via SUT on the local network via mDNS, and then will try adb locally for the given hwid.

It also makes a couple other misc changes, largely around making things less verbose, making 'adb root' optional (because it causes adbd to restart even if it can't get root, which can screw up a usb connection), and adds DeviceManager.installLocalApp for both adb and sut (install local apk file to remote device -- the existing installApp functionality disagreed where the apk was located between adb and sut implementations!)
Comment 1 Vladimir Vukicevic [:vlad] [:vladv] 2012-09-18 07:48:20 PDT
Pull request: https://github.com/mozilla/mozbase/pull/33
Comment 2 William Lachance (:wlach) 2012-09-18 08:18:23 PDT
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #1)
> Pull request: https://github.com/mozilla/mozbase/pull/33

I will review this. The pull request is strictly speaking against policy but I don't think it's that big a deal and I agree it would be a lot of work to manually attach this many patches to a bug (at least not without a tool).
Comment 3 Jeff Hammel 2012-10-09 16:55:31 PDT
Status on this?
Comment 4 Vladimir Vukicevic [:vlad] [:vladv] 2012-10-09 20:07:20 PDT
I got distracted by some of the surrounding stuff that happened as part of my pull request; will fix things up and land the mdns part tomorrow.
Comment 5 Vladimir Vukicevic [:vlad] [:vladv] 2012-10-10 09:09:56 PDT
Let's make this just about mDNS.  Down to two patches, one just a minor quieting cleanup.

Can test via sutcli.py:

  $ python sutcli.py -d 015d2bc2825ff206 info screen
  X:800 Y:1205

Two commits: https://github.com/vvuk/mozbase/compare/mdns

Looks good?
Comment 6 William Lachance (:wlach) 2012-10-10 10:24:44 PDT
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #5)
> Let's make this just about mDNS.  Down to two patches, one just a minor
> quieting cleanup.
> 
> Can test via sutcli.py:
> 
>   $ python sutcli.py -d 015d2bc2825ff206 info screen
>   X:800 Y:1205
> 
> Two commits: https://github.com/vvuk/mozbase/compare/mdns
> 
> Looks good?

Yup, LGTM.

I'd be happy to merge this.
Comment 7 Vladimir Vukicevic [:vlad] [:vladv] 2012-10-10 11:04:30 PDT
https://github.com/mozilla/mozbase/pull/35 for your one-click merging pleasure.
Comment 8 Jeff Hammel 2012-10-10 11:06:26 PDT
We *don't* merge pull requests directly, but ... ;)
Comment 9 Vladimir Vukicevic [:vlad] [:vladv] 2012-10-10 11:07:51 PDT
I know, but one click!  Give in to the ease and convenience!  Give iiiiinnnnnnnnn....
Comment 11 Vladimir Vukicevic [:vlad] [:vladv] 2012-10-10 11:14:57 PDT
Hopefully the diff doesn't get manually committed.. (loses separate commits, loses author, etc.)
Comment 12 Jeff Hammel 2012-10-10 11:17:14 PDT
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #9)
> I know, but one click!  Give in to the ease and convenience!  Give
> iiiiinnnnnnnnn....

There are reasons we have the workflow that we have.  If our workflow was entirely in github, that would at least be consistent and there would be one place to look at an issue.  But mozbase's issue tracker is bugzilla, which is not only a much more thorough issue tracker but also is used for the rest of mozilla's efforts, so when other mozilla software including test harnesses on mozilla-central need pieces of mozbase, they can depend on and cross-reference bugs in a meaningful way.

Having worked on a number of projects that split issue tracking across bugzilla and github pull requests, I can safely say that it is anything but convenient for me.  Some comments go in the bug. Some comments go in the pull request.  It is at best difficult to follow both conversations and many of the comments need to be cross-posted.
Comment 13 Jeff Hammel 2012-10-10 11:18:43 PDT
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #11)
> Hopefully the diff doesn't get manually committed.. (loses separate commits,
> loses author, etc.)

We general want a single commit per bug;  otherwise you are free to generate separate commits and post here separately for review (`git show <rev>`). The author is done via --author 'name <email>'
Comment 14 William Lachance (:wlach) 2012-10-10 11:32:51 PDT
I pulled in vlad's branch and rebased the commits to our standard format (preserving the author information). Thanks for the patch! This looks sweet.

https://github.com/mozilla/mozbase/compare/e9ab5da488...bd895aeb46

Note You need to log in before you can comment on or make changes to this bug.