810096 - Clean up internal variables in mozdevice

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[William Lachance (:wlach)]

We still have a mess of internal variables inside DeviceManagerADB. Many of them are initialized in the constructor, where they shouldn't be and also they should be prefixed with an underscore to make it clear that they should not be used outside this class.

Comment 1

[William Lachance (:wlach)]

There's a few cases in dmSUT that I want to clean up too, expanding scope of bug.

Comment 2

[William Lachance (:wlach)]

Attachment: Clean up internal variable usage in mozdevice

I'm pretty sure this shouldn't break anything. I checked for the adb variables in m-c and found they were only used inside dmADB:

wlach@popsicle:~/src/mozilla-central$ for var in haveRootShell haveSu useRunAs useDDCopy useZip logcatNeedsRoot pollingInterval tempDir; do git grep $var; done

testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py:        self.haveRootSh
testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py:        if root and not
testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py:        if root and not
testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py:        if not self.hav
testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py:        if not self.hav
testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py:            self.haveRo

Is there any kind of out-of-tree B2G harness that we should be worried about?

Comment 3

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #2)
> Created attachment 679865 [details] [diff] [review]
> Clean up internal variable usage in mozdevice
> 
> I'm pretty sure this shouldn't break anything. I checked for the adb
> variables in m-c and found they were only used inside dmADB:
>  ...


Hmm, actually I was wrong there (didn't realize git grep piped the output through less by default). Correcting the command, I see that some of the b2g tests are using this internal variable:

wlach@popsicle:~/src/mozilla-central$ for var in haveRootShell haveSu useRunAs useDDCopy useZip logcatNeedsRoot pollingInterval tempDir; do git grep \.$var; done | cat | grep -v devicemanagerADB | grep b2g.py
layout/tools/reftest/runreftestb2g.py:            if self._devicemanager.useDDCopy:
layout/tools/reftest/runreftestb2g.py:        if self._devicemanager.useDDCopy:
testing/mochitest/runtestsb2g.py:        if self._dm.useDDCopy:

It would actually be quite trivial to refactor this usage to shellCheckOutput (which will magically make them more sut-compatible by default). We should do that if/when we apply the above patch.

Comment 4

[Andrew Halberstadt [:ahal]]

Comment on attachment 679865 [details] [diff] [review]
Clean up internal variable usage in mozdevice

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

I know off the top of my head that the reftest and mochitest harness both use at least dmADB.useDDCopy (look at layout/tools/reftest/runreftestb2g.py and testing/mochitest/runtestsb2g.py). There may be other uses of some of the variables in that file as well.

We have reftests and mochitests running on try now too, so I'd also ask for a green try run before pushing this.

Comment 5

[Andrew Halberstadt [:ahal]]

The totally obvious try syntax for the b2g emulator platform is 'ics_armv7a_gecko'

Comment 6

[Mozilla RelEng Bot]

Try run for ac9623e82bfb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ac9623e82bfb
Results (out of 30 total builds):
    exception: 20
    success: 2
    warnings: 5
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ac9623e82bfb

Comment 7

[Mozilla RelEng Bot]

Try run for 921e2ab136fa is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=921e2ab136fa
Results (out of 30 total builds):
    exception: 3
    success: 17
    warnings: 10
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-921e2ab136fa

Comment 8

[William Lachance (:wlach)]

(In reply to Andrew Halberstadt [:ahal] from comment #4)
> Comment on attachment 679865 [details] [diff] [review]
> Clean up internal variable usage in mozdevice
> 
> Review of attachment 679865 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I know off the top of my head that the reftest and mochitest harness both
> use at least dmADB.useDDCopy (look at layout/tools/reftest/runreftestb2g.py
> and testing/mochitest/runtestsb2g.py). There may be other uses of some of
> the variables in that file as well.
> 
> We have reftests and mochitests running on try now too, so I'd also ask for
> a green try run before pushing this.

Just got a green try run for B2G (after modifying m-c and my patch a bit):

https://tbpl.mozilla.org/?tree=Try&rev=f2fbef008885

Did an Android run earlier with a few less cleanups that worked well (just ignore the b2g results here):

https://tbpl.mozilla.org/?tree=Try&rev=921e2ab136fa

Comment 9

[William Lachance (:wlach)]

Attachment: Patch with necessary changes made to m-c

Here's a new patch (written against m-c) which passes against try (see above).

If it looks ok, I'll push the mozdevice bits to mozbase, bump the version, then merge everything from mozbase in along with the m-c changes to adapt to the new API.

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 680904 [details] [diff] [review]
Patch with necessary changes made to m-c

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

Thanks! Looks good

Comment 11

[William Lachance (:wlach)]

Pushed the mozbase part of things:

https://github.com/mozilla/mozbase/commit/206129578a7686b249af439bcf9852e7b757e8e1

(like I mentioned, I'll include the m-c changes in a merge of mozdevice)

Comment 12

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/843df793a08a