793236 - Make dmADB and dmSUT truly interchangeable

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

I'm trying to go through the b2g automation code to switch from using dmADB to dmSUT. Though in theory these should be interchangeable, practically every line has needed to be changed. It might be the case that the automation code is using methods it shouldn't be, but in that case they should be identified as private more clearly in the code/documentation. Furthermore we have a class in build/mobile/b2gautomation.py that will need to use both dmADB and dmSUT interchangeably.

For these reasons dmADB and dmSUT should be refactored such that they are truly interchangeable. This might break existing automation that depends on it, so we'll need to get a list of all dependencies and make sure they still work afterwards.

Comment 1

[Andrew Halberstadt [:ahal]]

Note, I use the word "interchangeably" in the description. I realize that for the most part they already are. But from what I've seen in automation from the wild, there are many instances where they aren't.

It might just be the case that we need better documentation and to update erroneously implemented consumers of mozdevice. If that is true, then this bug is mainly just about making it clear what is a private method and what isn't and improving comments.

Comment 2

[Andrew Halberstadt [:ahal]]

I started a branch on my repo to clean mozdevice up a bit: https://github.com/ahal/mozbase/tree/mozdevice_refactor/mozdevice

I don't mind working on this on the side and fixing bitrot as I go, but I would like to get the first commit checked in (use 4 space indents) asap as that will bitrot against everything.

Comment 3

[William Lachance (:wlach)]

I don't disagree that private methods particular to dmSUT and dmADB should be prefixed with a '_'. These interfaces have been abused quite a bit up to now, especially checkCmd and sendCmd which people have been using to execute commands on the device in absence of a good alternative. It's not out of the question to fix this, and now's not a bad time since jmaher is currently reworking sut_tools. We should probably check in bug 792084 first so people have a public alternative to what they were using the private sendCmd/checkCmd methods for.

Not sure about the commenting changes or other you're thinking of. I'd have to take a look at them first.

Could you file a seperate bug for the whitespace issue? I've been meaning to do that since forever, let's just get it done. If it's not too much trouble and you haven't done it already, could you use reindent.py (http://pypi.python.org/pypi/Reindent/0.1.0)? This is more error-proof, and will clean up other whitespace problems at the same time.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

not using sendCmd will require even more rework of the sut_tools.  That is fine to do, but we should enforce that on new code until we can get the older code retrofitted.

Comment 5

[Andrew Halberstadt [:ahal]]

(In reply to William Lachance (:wlach) from comment #3)
> Could you file a seperate bug for the whitespace issue? I've been meaning to
> do that since forever, let's just get it done. If it's not too much trouble
> and you haven't done it already, could you use reindent.py
> (http://pypi.python.org/pypi/Reindent/0.1.0)? This is more error-proof, and
> will clean up other whitespace problems at the same time.

I filed bug 793685. I hadn't heard of re-indent and had already done the changes. I used internet recommended vim ways of doing it and checked the files manually as well. I'll also run it through try first just to be sure.

Comment 6

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #4)
> not using sendCmd will require even more rework of the sut_tools.  That is
> fine to do, but we should enforce that on new code until we can get the
> older code retrofitted.

I assume you mean "we should not enforce that on new code". :) 

Are there any other users of sendCmd? If it's just sut_tools I don't mind creating/sending a patch for that after bug 792084 lands.

Comment 7

[Andrew Halberstadt [:ahal]]

We use sendCmd or it's variants all over the place in the B2G automation code. This is what lead me to being confused over what was supposed to be public or not. This doesn't really matter though since I need to change that code to use dmSUT anyway.

Comment 8

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Make it clear which methods are private and improve docstrings

This patch won't make dmADB and dmSUT completely interchangeable, but at least it helps make it clearer which methods are ok to be called and which aren't. It also improves the docstrings and fixes up a few oddities about the dmADB implementation.

Flagging jmaher for feedback since this will break sut_tools AIUI. I'm fine with holding off landing this for the short term.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 664629 [details] [diff] [review]
Patch 1.0 - Make it clear which methods are private and improve docstrings

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

just a bunch of nits.

::: mozdevice/mozdevice/devicemanager.py
@@ +130,5 @@
>  
>      @abstractmethod
>      def listFiles(self, rootdir):
>          """
> +        Lists files on the device rootdir, requires cd to directory first

this is confusing, doesn't listFiles cd to the directory first?

@@ +244,2 @@
>          returns:
> +          success: filecontents

can we indicate String here?

@@ +369,2 @@
>          """
> +        # TODO Support org.mozilla.com and B2G

org.mozilla.com?

@@ +430,5 @@
>  
>      @abstractmethod
>      def reboot(self, ipAddr=None, port=30000):
>          """
> +        Restarts the device

can we call this reboots vs restarts?

@@ +498,5 @@
>      @abstractmethod
>      def uninstallAppAndReboot(self, appName, installPath=None):
>          """
> +        Uninstalls the named application from device and causes a reboot
> +        installPath - the path to where the application was installed

installPath is optional, would be nice to mention that. 

Also you don't spell out what appName is (i.e. org.mozilla.fennec)

@@ +541,3 @@
>          returns:
> +          success: file is created in <testroot>/logcat.log
> +          failure:

right now this just clears the logcat file, there is no return value.

@@ +553,3 @@
>          returns: data from the local file
> +          success: file is in 'filename'
> +          failure: None

this returns a single dump of whatever information is currently in the logcat circular buffer.

::: mozdevice/mozdevice/devicemanagerADB.py
@@ +298,4 @@
>  
>      def removeSingleDir(self, remoteDir):
>          """
> +        Deletes a single empty directory

this doesn't exist in SUT, should this be internal?

@@ +331,5 @@
> +        """
> +        Checks if remotePath is a directory on the device
> +
> +        returns:
> +          succesS: True

success, not succesS

@@ +406,3 @@
>          DEPRECATED: Use shell() or launchApplication() for new code
> +
> +        external function

should we remove this syntax?

@@ +497,2 @@
>              # First attempt to pull file regularly
> +            outerr = self._runCmd(["pull",  remoteFile, localFile]).communicate()

outerr, odd variable name.

@@ +697,2 @@
>          """
> +        Restarts the device

reboots the device.

@@ +786,4 @@
>          print ret
>          return ret
>  
> +    def _runCmd(self, args):

no documented returns here

@@ +797,5 @@
>              args.insert(2, self.packageName)
>          finalArgs.extend(args)
>          return subprocess.Popen(finalArgs, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
>  
> +    def _runCmdAs(self, args):

no documented returns here

@@ +806,4 @@
>  
>      # timeout is specified in seconds, and if no timeout is given,
>      # we will run until we hit the default_timeout specified in the __init__
> +    def _checkCmd(self, args, timeout=None):

no documented returns here

@@ +833,3 @@
>          return ret_code
>  
> +    def _checkCmdAs(self, args, timeout=None):

no documented returns here

::: mozdevice/mozdevice/devicemanagerSUT.py
@@ -877,4 @@
>          returns:
>            success: True
>            failure: False
> -        Throws a FileError exception when null (invalid dir/filename)

we are we losing this information about what we throw?  I believe this should be documented

@@ +1029,5 @@
>          return data
>  
>      def reboot(self, ipAddr=None, port=30000):
>          """
> +        Restarts the device

reboots the device.

Comment 10

[William Lachance (:wlach)]

Comment on attachment 664629 [details] [diff] [review]
Patch 1.0 - Make it clear which methods are private and improve docstrings

This is looking nice. 

It might be interesting to do some kind of automated diff of the method signatures of all three files to see what's missing/inconsistent between the three.

e.g.

grep "def " devicemanager.py | sed 's/^[ ]*//g' | sort > dm-methods
grep "def " devicemanagerSUT.py | sed 's/^[ ]*//g' | sort > dmsut-methods
grep "def " devicemanagerADB.py | sed 's/^[ ]*//g' | sort > dmadb-methods
diff -u dmsut-methods dmadb-methods

Just a thought.

I found a few minor things in addition to what jmaher brought up in the feedback:


>diff --git a/mozdevice/mozdevice/devicemanager.py b/mozdevice/mozdevice/devicemanager.py
>index 3150176..3781ce2 100644

>-    def getLocalHash(self, filename):
>+    def _getLocalHash(self, filename):
>         """

If we're going to modify the method signature, we might as well make this a static method (see _escapedCommandLine for an example).

>diff --git a/mozdevice/mozdevice/devicemanagerADB.py b/mozdevice/mozdevice/devicemanagerADB.py
>index 1cbf435..1b63780 100644

>-    def getRemoteHash(self, filename):
>+    def _getRemoteHash(self, filename):
>         """
>-        return the md5 sum of a remote file
>-        internal function
>+        Return the md5 sum of a file on the device
>+
>         returns:
>           success: MD5 hash for given filename
>           failure: None
>         """
>-        data = self.runCmd(["shell", "ls", "-l", filename]).stdout.read()
>+        data = self._runCmd(["shell", "ls", "-l", filename]).stdout.read()
>         return data.split()[3]

This function is clearly broken, and was probably only working because getLocalHash (which we were using before) operating the same way. We should copy the file locally and compute the MD5Sum there (probably using _getLocalHash).

> 
>-    def getLocalHash(self, filename):
>-        data = subprocess.Popen(["ls", "-l", filename], stdout=subprocess.PIPE).stdout.read()
>-        return data.split()[4]
>-

Comment 11

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.1 - Address nits

Incremental patch to address nits. I also decided to remove the two deprecated functions (launchProcess and fireProcess) from the interface (but not the implementation).

Comment 12

[Andrew Halberstadt [:ahal]]

I pushed this to try yesterday, looks good: https://tbpl.mozilla.org/?tree=Try&rev=1246e5e9dc2c

CC'ing :vlad since he mentioned he had an app that depends on devicemanager in bug 793856.

So outside of m-c automation we have:
sut_tools
vlad's app
any others?

Joel will this break anything in sut_tools? Should I land now or wait a bit?

Comment 13

[Mark Côté [:mcote]]

Autophone, of course. But I have no problem making any necessary changes in the interest of a better devicemanager.

Comment 14

[William Lachance (:wlach)]

(In reply to Andrew Halberstadt [:ahal] from comment #12)
> I pushed this to try yesterday, looks good:
> https://tbpl.mozilla.org/?tree=Try&rev=1246e5e9dc2c
> 
> CC'ing :vlad since he mentioned he had an app that depends on devicemanager
> in bug 793856.
> 
> So outside of m-c automation we have:
> sut_tools
> vlad's app
> any others?
> 
> Joel will this break anything in sut_tools? Should I land now or wait a bit?

Eideticker (http://github.com/mozilla/eideticker). Autophone (http://github.com/mozilla/autophone). frof (http://github.com/wlach/frof). I don't believe any of those should be affected by these changes. Eideticker and frof were explicitly designed not to use any methods specific to dmADB or dmSUT, and as far as I can tell autophone is the same way (at the very least it doesn't use checkCmd or sendCmds.

This change will definitely break sut_tools in the short term, though it should be fairly straightforward to just substitute calls of sendCmds to _sendCmds.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

I am fine with the sut_tools changes as we need to modify the sut_tools enough as it is to support more modern devicemanager files.

Comment 16

[Andrew Halberstadt [:ahal]]

master: https://github.com/mozilla/mozbase/commit/2320449d4c14b30f95a5e4caa4a9e5e42629e2e5
version bump: https://github.com/mozilla/mozbase/commit/edcabf634ac91b0a031f0288976298dd5624a86b

Also uploaded to pypi.

I'm going to leave this bug open since while this patch is a step in the right direction, dmADB and dmSUT still aren't actually interchangeable. Also unassigning myself, hopefully I or someone else can get back to this in the future.

Comment 17

[William Lachance (:wlach)]

Bug 792072 is related to this.

Comment 18

[Andrew Halberstadt [:ahal]]

I think this bug is safe to close, especially with bc's implementation replacing dmADB. Marking fixed instead of WONTFIX because a patch did land here.