990601 - Autophone - use adb instead of tcp/ip to control devices, only support local tests run from local storage.

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

As discussed in bug 969518 comment 9, we are throwing in the towel on testing page loading over wifi and will be transitioning away from using the SUTAgent altogether. In the future, we will control the devices over adb using devicemanagerADB and will load test files from local storage on the devices.

Comment 1

[Bob Clary [:bc] (inactive)]

I have a patch in progress that I have been testing. Unfortunately the galaxy s3s were unreliable and I spent yesterday using the production nexus phones to test April 21.

I've posted the results to:

<http://phonedash-dev.allizom.org/#/org.mozilla.fennec/throbberstart/local-twitter/norejected/2014-04-21/2014-04-21/notcached/noerrorbars/standarderror>

you can compare those to

<http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/local-twitter/rejected/2014-04-21/2014-04-21/notcached/noerrorbars/standarderror>

Note the phone ids have changed to device names without mac addresses and the values have shifted in some cases. I'm still working on reliability issues but will have a set of patches for review soon.

Overall the results are an improvement in terms of variability with the exception of the nexus one devices. I'm not sure what is happening there. It may be sdcard related.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: bug-990601-wip.patch

Work in progress patch if you want to see what I've been up to. wlach, you are probably most interested in the adb.py file. As you can see I've borrowed heavily from mozdevice but only implemented the parts I need for Autophone. Some of the changes I made probably aren't appropriate for devicemanager as is especially since I dropped the Android 2.2 cruft, replaced pushDir/pushFile with push, etc.

I am very happy with the testing I've done so far.

Some of the issues outstanding:

* Needs all devices to be online before autophone starts.
* Probably need to clean up more about registration/restarting etc.
* I want to see about removing most if not all of the retry stuff.
* I still need to update/test tests/runtestsremote.py
* ADBTimedout doesn't print. I had it inheriting from ADBError but in attempting to fix the printing issue, I separated them. I'll make it derive from ADBError when I fix the printing issue.

Feedback welcome but not immediately required. I hope to have a final patch for review this afternoon/evening.

Comment 3

[Bob Clary [:bc] (inactive)]

Nit: I removed the wait parameter from reboot while I was writing the doc strings, but failed to update worker.py before generating the patch. Just ignore that discrepancy for the moment.

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: bug-990601-wip-2.patch

Updated wip patch 2.

* fix default timeouts
* add reboot return value
* change ping to actually touch the device by checking sdcard
* make dm a normal property and remove the disconnect_dm
* change exitcode detections
* misc

I've updated the device names and started using this patch on phonedash.mozilla.org for 2014-05-03 builds and later. I've added an additional nexus-one, a droid pro and 3 galaxy s3s in order to test their behavior along side of the other nexi.

Comment 5

[Bob Clary [:bc] (inactive)]

I have been testing variations of this patch since Saturday. From Saturday through Monday, Autophone was running on the Mac Mini and testing some additional devices such as the Samsung Galaxy s3s. It turns out (not that I am actually surprised) that the Mini has issues with the devices connected via USB. I have seen frequent cases where the USB driver crashes and the devices lose connectivity. It addition the Mini gets very hot and appears to be working overly hard. For the short term, it will be adequate but in the long term I believe a Linux solution will be required.

Overall the behavior was much improved over before though the Nexus Ones were noisier than I had hoped.

I began testing all of my devices and got 19 of them to work pretty well. I started a full on test of today's results (Tuesday) on Linux with the hope that the USB behavior would be improved over the OSX Mini. I found an issue with adb between at least Linux and my Nexus Ones, which are running CyanogenMod 7 where pushing the profile to /data/local/tmp/profile was not working properly. This may have accounted for the noise in the earlier test on OSX but I'm not sure yet.

I have cleaned up the test results from this morning and have begun a test run on Linux with this patch without the Nexus Ones or the other devices such as the Galaxy S3s. I have added three Droid Pros to get additional 2.3/2.2 coverage. You can see the results at:

<http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/remote-twitter/norejected/2014-05-03/2014-05-09/notcached/errorbars/standarderror>

where 2014-05-03 - 2014-05-05 were run on the Mac Mini and on Linux after 2014-05-06.

Known Issues with this patch:

All devices must be on and connected prior to starting Autophone.

There should be debugging checks on the commands to make sure that they are returning the expected results, e.g. that the pushed profile actually contains the expected files, etc.

We can probably relax the use of root when creating the profile since I introduced most of those during the attempts to get the profile created properly on the Nexus Ones, but I'd like to leave that for later.

I need to update USAGE.md and include a devices.ini.example file.

I updated tests/runtestsremote.py but this will not actually work with the existing test frameworks such as reftest. Remote automation does not support specifying the serial number of the device. In order to get the Mochitest-Canvas tests additional work will need to be done on reftests and mochitests remote automation to support ADB connectivity over USB using serial number to specify the device.

Comment 6

[Bob Clary [:bc] (inactive)]

Attachment: bug-990601.patch v1

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 8418159 [details] [diff] [review]
bug-990601.patch v1

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

This is great!  Mostly just nits.

::: adb.py
@@ +102,5 @@
> +        for arg in cmd:
> +            arg.replace('&', '\&')
> +
> +            needsQuoting = False
> +            for char in [ ' ', '(', ')', '"', '&' ]:

Remove extra horizontal spaces.

@@ +107,5 @@
> +                if arg.find(char) >= 0:
> +                    needsQuoting = True
> +                    break
> +            if needsQuoting:
> +                arg = '\'%s\'' % arg

You use double quotes elsewhere, so for readability I'd probably do "'%s'".

@@ +116,5 @@
> +
> +    @staticmethod
> +    def _get_exitcode(file_obj):
> +        """
> +        Get the exitcode from the last line of stdout for shell commands.

This function doesn't seem to have anything to do with stdout specifically.

@@ +169,5 @@
> +                    if not self.dirExists(testRoot):
> +                        self.mkDir(testRoot, timeout=timeout, root=root)
> +                    self.deviceRoot = testRoot
> +                    return
> +                except:

Probably useful to log at least some exceptions here; if this fails as it is now, you're not going to have any idea why.

@@ +224,5 @@
> +        args.extend(cmds)
> +
> +        result['args'] = args
> +        result['stdout'] = tempfile.NamedTemporaryFile()
> +        result['stderr'] = tempfile.NamedTemporaryFile()

Why are these Named?  I might have missed something, but I don't see their names being used anywhere.

@@ +285,5 @@
> +                                result['exitcode'],
> +                                output))
> +            return output
> +        except:
> +            raise

I think this is unnecessary; you can have a try/finally without an except clause, in which case exceptions will be raised as usual and the finally clause will still be executed.

@@ +351,5 @@
> +        if root:
> +            ld_library_path='LD_LIBRARY_PATH=/vendor/lib:/system/lib'
> +            cmd = '%s %s' % (ld_library_path, cmd)
> +            if self._haveRootShell:
> +                pass

Is this just in here for clarity?

@@ +367,5 @@
> +            envstr = '&& '.join(map(lambda x: 'export %s=%s' % (x[0], x[1]), env.iteritems()))
> +            cmd = envstr + "&& " + cmd
> +        cmd += "; echo rc=$?"
> +
> +        args=[self._adbPath]

Spaces around =.

@@ +715,5 @@
> +                                         timeout=timeout,
> +                                         root=root).split('\r\n')
> +                self._logger.debug('listFiles: data: %s' % data)
> +            except ADBTimedout:
> +                raise

You could omit this and just keep the following except clause; ADBTimedout and any other exceptions will automatically be raised as usual.

@@ +776,5 @@
> +            raise TypeError("Process name %s is not a string" % processName)
> +
> +        pid = None
> +
> +        #filter out extra spaces

Properly format this and following comments.

@@ +777,5 @@
> +
> +        pid = None
> +
> +        #filter out extra spaces
> +        parts = filter(lambda x: x != '', processName.split(' '))

These days that would be done with a list comprehension, e.g. [x for x in processName.split(' ') if x != ''].

@@ +783,5 @@
> +
> +        #filter out the quoted env string if it exists
> +        #ex: '"name=value;name2=value2;etc=..." process args' -> 'process args'
> +        parts = processName.split('"')
> +        if (len(parts) > 2):

No need for outer parentheses.

@@ +791,5 @@
> +        parts = pieces[0].split('/')
> +        app = parts[-1]
> +
> +        procList = self.getProcessList(timeout=timeout)
> +        if (procList == []):

if not procList

@@ +796,5 @@
> +            return None
> +
> +        for proc in procList:
> +            procName = proc[1].split('/')[-1]
> +            if (procName == app):

No parentheses.

@@ +833,5 @@
> +            if user_i == -1 or pid_i == -1:
> +                raise ADBError('getProcessList: Unknown format: %s' % header)
> +            ret = []
> +            line = result['stdout'].readline()
> +            while (line):

No parentheses.

@@ +835,5 @@
> +            ret = []
> +            line = result['stdout'].readline()
> +            while (line):
> +                els = line.split()
> +                ret.append(list([int(els[pid_i]), els[-1], els[user_i]]))

I don't understand the use of list() here.

@@ +841,5 @@
> +            self._logger.debug('getProcessList: %s' % ret)
> +            return ret
> +        except ValueError:
> +                self._logger.exception('getProcessList: %s %s' % (header, line))
> +                raise

Indentation is wrong.

::: tests/runtestsremote.py
@@ +40,1 @@
>                                         '%(message)s')

May as well concatenate those lines now.

@@ +243,5 @@
>          test_parameters['ssl_port'] = test_parameters['port_manager'].reserve()
>  
>          common_args = [
> +            '--dm_trans=adb',
> +            '--deviceIP=dummy', # Required but not necessary.

What does "Required but not necessary" mean?

::: worker.py
@@ +175,5 @@
>                                         {'phoneid': self.phone_cfg['phoneid'],
> +                                        'pid': os.getpid()},
> +                                       '%(phoneid)s|%(pid)s|%(message)s')
> +
> +        # Move this from a @property since having the ADB initialized on the fly

I think "Moved" makes it clear that this has already been moved and is not a TODO item.

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: review.patch v1

(In reply to Mark Côté ( :mcote ) from comment #7)
> Comment on attachment 8418159 [details] [diff] [review]
> bug-990601.patch v1
> 
> Review of attachment 8418159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is great!  Mostly just nits.
> 

Heads up to wlach and me for later: Several of these nits came from copy/pasting from devicemanagerADB.py

> 
> @@ +224,5 @@
> > +        args.extend(cmds)
> > +
> > +        result['args'] = args
> > +        result['stdout'] = tempfile.NamedTemporaryFile()
> > +        result['stderr'] = tempfile.NamedTemporaryFile()
> 
> Why are these Named?  I might have missed something, but I don't see their
> names being used anywhere.
> 

I originally had meant to close them and reopen if needed, but later changed to keep them open until needed.

> 
> @@ +351,5 @@
> > +        if root:
> > +            ld_library_path='LD_LIBRARY_PATH=/vendor/lib:/system/lib'
> > +            cmd = '%s %s' % (ld_library_path, cmd)
> > +            if self._haveRootShell:
> > +                pass
> 
> Is this just in here for clarity?
> 

Yes.

> 
> @@ +715,5 @@
> > +                                         timeout=timeout,
> > +                                         root=root).split('\r\n')
> > +                self._logger.debug('listFiles: data: %s' % data)
> > +            except ADBTimedout:
> > +                raise
> 
> You could omit this and just keep the following except clause; ADBTimedout
> and any other exceptions will automatically be raised as usual.
> 

ADBTimedout is a child class of ADBError, wouldn't except ADBError catch both? See review.patch for comments. I want timeouts to be fatal but not ADBError. Think ls this-file-does-not-exist returns exitcode 1 but isn't really an error.

> 
> @@ +835,5 @@
> > +            ret = []
> > +            line = result['stdout'].readline()
> > +            while (line):
> > +                els = line.split()
> > +                ret.append(list([int(els[pid_i]), els[-1], els[user_i]]))
> 
> I don't understand the use of list() here.
> 

Carry over from devicemanager. I removed it.

> @@ +243,5 @@
> >          test_parameters['ssl_port'] = test_parameters['port_manager'].reserve()
> >  
> >          common_args = [
> > +            '--dm_trans=adb',
> > +            '--deviceIP=dummy', # Required but not necessary.
> 
> What does "Required but not necessary" mean?

Required by the current test runners but not necessary for us since we don't communicate with the devices over IP anymore. I added a comment to the patch.

Comment 9

[William Lachance (:wlach)]

Comment on attachment 8418159 [details] [diff] [review]
bug-990601.patch v1

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

As discussed on irc, we think this module might make a good replacement for the devicemanager stuff in mozdevice, which has quite a bit of historical baggage from trying to be compatible with devicemanagerSUT and is at times difficult to understand and use well.

One thing is that this new module uses a mix of "foo_bar" and "fooBar" method names. We should pick one and stick to it (most of the rest of mozbase uses "foo_bar", so maybe we should too). If we're going to rename stuff and make a new module, I might also change the method names to more closely map to the existing methods in python's os module, for consistency and learnability.

I guess in general most of the comments here and above are of the bike-shed variety, but I figure it's worth getting the little details right if we're doing up a new module. :)

::: adb.py
@@ +37,5 @@
> +class ADB(object):
> +
> +    def __init__(self, deviceSerial,
> +                 adb='adb',
> +                 deviceRoot='',

I think we should rename deviceRoot to testRoot (or something equivalently named), since it's more descriptive of what it actually is: a temporary area of the device for dumping test stuff.

@@ +154,5 @@
> +        if self.deviceRoot:
> +            if not self.dirExists(self.deviceRoot, timeout=timeout, root=root):
> +                try:
> +                    self.mkDir(self.deviceRoot, timeout=timeout, root=root)
> +                except:

Blanket try ... except tends to hide syntax errors and other problems. Could we look for the specific exception you think might be triggered here?

@@ +169,5 @@
> +                    if not self.dirExists(testRoot):
> +                        self.mkDir(testRoot, timeout=timeout, root=root)
> +                    self.deviceRoot = testRoot
> +                    return
> +                except:

Yes, and see above about blanket try except

@@ +475,5 @@
> +            result = self.shell(cmd, env=env, cwd=cwd, timeout=timeout, root=root)
> +            if result['timedout']:
> +                raise ADBTimedout(result=result)
> +            return result['exitcode'] == 0
> +        except:

Blanket try ... except tends to hide syntax errors and other problems. Could we look for the specific exception you think might be triggered here?

@@ +482,5 @@
> +            if result:
> +                result['stdout'].close()
> +                result['stderr'].close()
> +
> +    def chmodDir(self, remoteDir, mask="777", timeout=None, root=False):

Let's call this chmod, and change remoteDir to path (as far as I can tell there is no reason this couldn't work on either a directory or a specific file)

@@ +552,5 @@
> +        return (
> +            self.shell_bool('ls -a %s' % remotePath, timeout=timeout, root=root) and
> +            not self.shell_bool('ls -a %s/' % remotePath, timeout=timeout, root=root))
> +
> +    def getDeviceRoot(self):

May as well just kill this method and make deviceRoot/testRoot/test_root/whatever we want to call it a property.

@@ +588,5 @@
> +            lines = [line for line in lines if not re.search(regex, line)]
> +
> +        return lines
> +
> +    def killProcess(self, appname, sig=None, timeout=None, root=False):

Can we rename this to "kill"?

@@ +615,5 @@
> +                except ADBError, e:
> +                    if self.processExist(appname, timeout=timeout):
> +                        raise e
> +
> +    def launchApplication(self, appName, activityName, intent, url=None,

We may want to eventually move this method (and some other android specific methods) to an android subclass. It can stay for now.

@@ +722,5 @@
> +        data[:] = [item for item in data if item]
> +        self._logger.debug('listFiles: %s' % data)
> +        return data
> +
> +    def mkDir(self, path, parents=False, timeout=None, root=False):

mkdir might be a better name if we're starting fresh.

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: review.patch v2

I missed a couple of issues in the first review.

Comment 11

[Bob Clary [:bc] (inactive)]

(In reply to William Lachance (:wlach) from comment #9)
> Comment on attachment 8418159 [details] [diff] [review]
> bug-990601.patch v1
> 
> Review of attachment 8418159 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> As discussed on irc, we think this module might make a good replacement for
> the devicemanager stuff in mozdevice, which has quite a bit of historical
> baggage from trying to be compatible with devicemanagerSUT and is at times
> difficult to understand and use well.
> 
> One thing is that this new module uses a mix of "foo_bar" and "fooBar"
> method names. We should pick one and stick to it (most of the rest of
> mozbase uses "foo_bar", so maybe we should too). If we're going to rename
> stuff and make a new module, I might also change the method names to more
> closely map to the existing methods in python's os module, for consistency
> and learnability.
> 

I like that idea. I wanted to do more of that as well as keeping the same semantics of the corresponding command line options and behaviors.

> I guess in general most of the comments here and above are of the bike-shed
> variety, but I figure it's worth getting the little details right if we're
> doing up a new module. :)
> 
> ::: adb.py
> @@ +37,5 @@
> > +class ADB(object):
> > +
> > +    def __init__(self, deviceSerial,
> > +                 adb='adb',
> > +                 deviceRoot='',
> 
> I think we should rename deviceRoot to testRoot (or something equivalently
> named), since it's more descriptive of what it actually is: a temporary area
> of the device for dumping test stuff.
> 

Sounds good. The different names always confused me.

> @@ +154,5 @@
> > +        if self.deviceRoot:
> > +            if not self.dirExists(self.deviceRoot, timeout=timeout, root=root):
> > +                try:
> > +                    self.mkDir(self.deviceRoot, timeout=timeout, root=root)
> > +                except:
> 
> Blanket try ... except tends to hide syntax errors and other problems. Could
> we look for the specific exception you think might be triggered here?
> 
> @@ +169,5 @@
> > +                    if not self.dirExists(testRoot):
> > +                        self.mkDir(testRoot, timeout=timeout, root=root)
> > +                    self.deviceRoot = testRoot
> > +                    return
> > +                except:
> 
> Yes, and see above about blanket try except

I can see your point in general, but in the first case I just catch the exception so I can log an error (and that should be exception instead actually) as soon as possible with message about the deviceRoot but immediately raise it again as a hard fail. In the second case, I do fall through after logging the specific error, but do raise an ADBError as a hard fail before exiting the function.

What is the benefit of doing except clauses for the possible exceptions in these cases?

> 
> @@ +475,5 @@
> > +            result = self.shell(cmd, env=env, cwd=cwd, timeout=timeout, root=root)
> > +            if result['timedout']:
> > +                raise ADBTimedout(result=result)
> > +            return result['exitcode'] == 0
> > +        except:
> 
> Blanket try ... except tends to hide syntax errors and other problems. Could
> we look for the specific exception you think might be triggered here?
> 

If I understand which one we are looking at, that except had a raise immediately after which I removed in the review patch 2.

> @@ +482,5 @@
> > +            if result:
> > +                result['stdout'].close()
> > +                result['stderr'].close()
> > +
> > +    def chmodDir(self, remoteDir, mask="777", timeout=None, root=False):
> 
> Let's call this chmod, and change remoteDir to path (as far as I can tell
> there is no reason this couldn't work on either a directory or a specific
> file)
> 

Ok. I tried not to change the variable names from dmadb, but would like them to be consistent internally here.

> @@ +552,5 @@
> > +        return (
> > +            self.shell_bool('ls -a %s' % remotePath, timeout=timeout, root=root) and
> > +            not self.shell_bool('ls -a %s/' % remotePath, timeout=timeout, root=root))
> > +
> > +    def getDeviceRoot(self):
> 

Ok.

> May as well just kill this method and make
> deviceRoot/testRoot/test_root/whatever we want to call it a property.
> 
> @@ +588,5 @@
> > +            lines = [line for line in lines if not re.search(regex, line)]
> > +
> > +        return lines
> > +
> > +    def killProcess(self, appname, sig=None, timeout=None, root=False):
> 
> Can we rename this to "kill"?
> 

sure.

> @@ +615,5 @@
> > +                except ADBError, e:
> > +                    if self.processExist(appname, timeout=timeout):
> > +                        raise e
> > +
> > +    def launchApplication(self, appName, activityName, intent, url=None,
> 
> We may want to eventually move this method (and some other android specific
> methods) to an android subclass. It can stay for now.
> 
> @@ +722,5 @@
> > +        data[:] = [item for item in data if item]
> > +        self._logger.debug('listFiles: %s' % data)
> > +        return data
> > +
> > +    def mkDir(self, path, parents=False, timeout=None, root=False):
> 
> mkdir might be a better name if we're starting fresh.

agreed.

Comment 12

[Mark Côté [:mcote]]

(In reply to William Lachance (:wlach) from comment #9)
> One thing is that this new module uses a mix of "foo_bar" and "fooBar"
> method names. We should pick one and stick to it (most of the rest of
> mozbase uses "foo_bar", so maybe we should too).

Agree with "foo_bar"; it appears to be the typical convention in Python.

> ::: adb.py
> @@ +37,5 @@
> > +class ADB(object):
> > +
> > +    def __init__(self, deviceSerial,
> > +                 adb='adb',
> > +                 deviceRoot='',
> 
> I think we should rename deviceRoot to testRoot (or something equivalently
> named), since it's more descriptive of what it actually is: a temporary area
> of the device for dumping test stuff.

You mean test_root. ;)

> @@ +482,5 @@
> > +            if result:
> > +                result['stdout'].close()
> > +                result['stderr'].close()
> > +
> > +    def chmodDir(self, remoteDir, mask="777", timeout=None, root=False):
> 
> Let's call this chmod, and change remoteDir to path (as far as I can tell
> there is no reason this couldn't work on either a directory or a specific
> file)

I agree with that only if we add a 'recursive' option.  If I'm running chmod on a directory, I would not expect the default behaviour to include all files within that directory; it's entirely reasonable to chmod just a directory (and this is how the shell command works).

I agree with all of wlach's other comments.

Comment 13

[Mark Côté [:mcote]]

Er, except the blanket try/except statements.  As bc says, he's raising the error after logging it; I think that's an acceptable use.  It's only a problem when the exception is not propagated and silently disappears.

Comment 14

[Bob Clary [:bc] (inactive)]

Attachment: bug-990601-v2.patch

* updated USAGE.md and added devices.ini.example

* adb.py

** Changed ADBTimedout to not inherit from ADBError in order to handle the errors separately. Basicially any ADBTimedout in adb.py is now fatal and should be handled explicitly by callers.

** improved detection of ls

** names now in foo_bar format.

** property test_root

** chmodDir -> chmod

** new method exists

** dirExists -> isdir

** fileExists -> isfile

** new method kill (by pid)

** killProcess -> pkill

** mkDir -> mkdir

** removeDir -> rmdir (like shell counter-part should be empty)

** removeFile -> rm

* worker.py
** sorted status message by device id and added tree to current build

* trigger_runs.py
** changed to attempt to connect to Autophone prior to retrieving build urls so that any connection issues are discovered early.

Plan is to land this in Autophone's repository and then work with wlach to add this to mozbase.

Comment 15

[William Lachance (:wlach)]

Comment on attachment 8420928 [details] [diff] [review]
bug-990601-v2.patch

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

The ADB parts look good. I only have bikeshed type comments.

::: USAGE.md
@@ +7,4 @@
>  Autophone has a number of command-line options. Run "python autophone.py -h"
>  to see them. Some important ones are
>  
> +--devices <devices>: Specifies an ini file which lists each device to be tested.

I wonder a bit if USAGE.md might be better written as a tutorial defining how to use it with a common set of command line options / configuration files. People can always just run --help if they want to see the options.

::: adb.py
@@ +37,5 @@
> +    def __str__(self):
> +        return ADBError__str__(self)
> +
> +
> +class ADBTimedout(Exception):

I wonder if ADBTimeout makes a better name?

@@ +115,5 @@
> +    def test_root(self):
> +        return self._test_root
> +
> +    @staticmethod
> +    def _escapeCommandLine(cmd):

I know it's an internal function, but lingering camel case.

@@ +168,5 @@
> +            exitcode = None
> +
> +        return exitcode
> +
> +    def _setupTestRoot(self, timeout=None, root=False):

I know it's an internal function, but lingering camel case.

@@ +712,5 @@
> +
> +        cmd = self._escapeCommandLine(acmd)
> +        self.shell_output(cmd, timeout=timeout)
> +
> +    def launchFennec(self, app_name, intent="android.intent.action.VIEW",

Lingering camelcase.

@@ +746,5 @@
> +                               wait=wait, fail_if_running=fail_if_running,
> +                               timeout=timeout)
> +
> +
> +    def listFiles(self, path, timeout=None, root=False):

Lingering camelcase.

@@ +817,5 @@
> +        self.shell_output('mkdir %s' % path, timeout=timeout, root=root)
> +        if not self.isdir(path, timeout=timeout, root=root):
> +            raise ADBError('mkdir %s Failed' % path)
> +
> +    def processExist(self, processName, timeout=None):

Lingering camelcase.

@@ +857,5 @@
> +                pid = proc[0]
> +                break
> +        return pid
> +
> +    def getProcessList(self, timeout=None):

Lingering camelcase.

@@ +937,5 @@
> +        self.command_output(["wait-for-device"], timeout=timeout)
> +        time.sleep(self._reboot_settling_time)
> +        return self.getState() == 'device'
> +
> +    def clearLogcat(self, timeout=None):

Lingering camelcase.

@@ +1000,5 @@
> +        except ADBError, e:
> +            if not force and 'No such file or directory' in e.result['stdout']:
> +                raise
> +
> +    def installApp(self, apk_path, timeout=None):

Lingering camelcase.

@@ +1020,5 @@
> +        if data.find('Success') == -1:
> +            raise ADBError("install failed for %s. Got: %s" %
> +                           (apk_path, data))
> +
> +    def uninstallApp(self, app_name, timeout=None):

Lingering camelcase.

@@ +1038,5 @@
> +        data = self.command_output(["uninstall", app_name], timeout=timeout)
> +        if data.find('Success') == -1:
> +            raise ADBError("uninstall failed for %s. Got: %s" % (app_name, data))
> +
> +    def updateApp(self, appBundlePath, timeout=None):

Lingering camelcase.

@@ +1051,5 @@
> +        """
> +        return self.command_output(["install", "-r", appBundlePath],
> +                                   timeout=timeout)
> +
> +    def getProp(self, prop, timeout=None):

Lingering camelcase.

@@ +1064,5 @@
> +        """
> +        output = self.shell_output('getprop %s' % prop, timeout=timeout)
> +        return output
> +
> +    def powerOn(self, timeout=None):

Lingering camelcase.

@@ +1086,5 @@
> +            if e.result['exitcode'] != 137:
> +                raise
> +            self._logger.warning('Unable to set power stayon true: %s' % e)
> +
> +    def getState(self, timeout=None):

Lingering camelcase.

Comment 16

[Bob Clary [:bc] (inactive)]

I didn't realize that in addition to the properties and variables, the method names should be changed from camelCase to noncamel-case. Is that also desired?

Comment 17

[Mark Côté [:mcote]]

Yes, camelCase has no place in our Python code whatsoever. :)

Comment 18

[Bob Clary [:bc] (inactive)]

Ok. let me work that up and chase down a problem in ADBError and I'll put a new patch up.

Comment 19

[William Lachance (:wlach)]

"Function names should be lowercase, with words separated by underscores as necessary to improve readability.

mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility."

...

"Method Names and Instance Variables: Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability."

:P

http://legacy.python.org/dev/peps/pep-0008/#function-names

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: bug-990601-v3.patch

Comment 21

[Mark Côté [:mcote]]

Comment on attachment 8421764 [details] [diff] [review]
bug-990601-v3.patch

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

::: adb.py
@@ +42,5 @@
> +    def __init__(self, message='', result=None):
> +        ADBError__init__(self, message, result=result)
> +
> +    def __str__(self):
> +        return ADBError__str__(self)

This is basically a complicated and weird way of doing inheritance... I know you don't want ADBTimeout to be an ADBError, but why not make them both subclass something like ADBBaseError?

@@ +112,5 @@
> +        self._setup_test_root()
> +
> +    @property
> +    def test_root(self):
> +        return self._test_root

I think in wlach's original comment, he meant that test_root should be an attribute and that this function should be removed entirely, since it does nothing but pass the attribute along.  It does have the side effect of making test_root read only, but one can still set _test_root directly anyway.

@@ +769,5 @@
> +                                         root=root).split('\r\n')
> +                self._logger.debug('list_files: data: %s' % data)
> +            except ADBTimeout:
> +                # Timeout errors are fatal.
> +                raise

Now that ADBTimeout isn't a subclass of ADBError (which I missed last time), you don't need this.

@@ +838,5 @@
> +        processName = ' '.join(parts)
> +
> +        # Filter out the quoted env string if it exists
> +        # ex: '"name=value;name2=value2;etc=..." process args' -> 'process args'
> +        parts = processName.split('"')

Kill all camelCase.

@@ +847,5 @@
> +        parts = pieces[0].split('/')
> +        app = parts[-1]
> +
> +        procList = self.get_process_list(timeout=timeout)
> +        if not procList:

Kill all camelCase.

Comment 22

[Bob Clary [:bc] (inactive)]

Attachment: bug-990601-v4.patch

This is what I'm checking in.

Comment 23

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/8bddba38e1f536a2403beab888fac71a7a1d9df4