Closed Bug 776737 Opened 8 years ago Closed 8 years ago

AutoPhone: replace adb usage with DeviceManagerSUT

Categories

(Testing :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcote, Unassigned)

Details

Attachments

(2 files, 1 obsolete file)

adb is very flaky, especially with multiple phones. The SUT agent appears to be much more reliable, as well as being remotely accessible. Thus all usage of adb in AutoPhone should be replaced with equivalent DeviceManagerSUT functionality.
All adb usage has been removed and replaced with SUT.

If you want to run the smoke test, configure a phone (see INSTALL) and then run ./smoketest.sh. After it starts, just (re)start the agent on your phone, which will ping the AutoPhone instance and start the smoke test. You can't have another instance of AutoPhone running while the smoke test runs, btw.

Any questions about AutoPhone in general, lemme know!
Attachment #645352 - Flags: review?(wlachance)
Comment on attachment 645352 [details] [diff] [review]
Changed adb usage to DeviceManagerSUT

r- only because importing AgentError inside worker.py makes me comfortable. The rest of the comments are either nits, general observations, or suggestions for followup work.

>diff --git a/autophone.py b/autophone.py
>index ff7ba3c..3903a05 100644
>--- a/autophone.py
>+++ b/autophone.py
>@@ -28,14 +28,13 @@ try:
> except ImportError:
>     import simplejson
> 
>-import androidutils
>-import builds
>-import phonetest
>-
> from manifestparser import TestManifest
>+from mozdevice.devicemanager import NetworkTools

Just FYI, mozhttpd also has a copy of a similar method (get_lan_ip) as the getLanIp() method of NetworkTools. 

Someday it would be nice to settle on one definitive interface for that. A few months ago I thought mozhttpd would be the right place for that sort of method, but I guess maybe not.

>+class DeviceManagerSUT(mozdevice.devicemanagerSUT.DeviceManagerSUT):
>+
>+    def getInfo(self, directive=None):
>+        """Overriden for uptimemillis info directive."""
>+        ...

Why is this functionality not in the upstream devicemanagerSUT? (likewise: where is the code to the agent which supports the "uptimemillis" directive)

>+    """Overriding to not use su -c."""
>+    def shell(self, cmd, outputfile, env=None, cwd=None):
>+        cmdline = self._escapedCommandLine(cmd)
>+        if env:
>+            cmdline = '%s %s' % (self.formatEnvString(env), cmdline)
>+
>+        try:
>+            if cwd:
>+                self.sendCmds([{ 'cmd': 'execcwd %s %s' % (cwd, cmdline) }],
>+                              outputfile)
>+            else:
>+                import logging
>+                logging.debug('cmdline: %s' % cmdline)
>+                self.sendCmds([{ 'cmd': 'exec "%s"' % cmdline }], outputfile)
>+        except mozdevice.devicemanager.AgentError:
>+            return None
>+
>+        # dig through the output to get the return code
>+        lastline = _pop_last_line(outputfile)
>+        if lastline:
>+            m = re.search('return code \[([0-9]+)\]', lastline)
>+            if m:
>+                return int(m.group(1))
>+
>+        # woops, we couldn't find an end of line/return value
>+        return None

Let's link to bug 777034 in a comment for this one.

diff --git a/publishAgentIni.py b/publishAgentIni.py
>index 5bd39b2..0717694 100644
>--- a/publishAgentIni.py
>+++ b/publishAgentIni.py
>@@ -1,37 +1,42 @@
>-from devicemanagerSUT import DeviceManagerSUT
>-from optparse import OptionParser
>-import sys
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this file,
>+# You can obtain one at http://mozilla.org/MPL/2.0/.
>+
> import os
>+import sys
>+from optparse import OptionParser
>+
>+from device import DeviceManagerSUT
> 
> def main(ip, port, filename):
>-  dm = DeviceManagerSUT(ip, port)
>-  dm.pushFile(filename, '/data/data/com.mozilla.SUTAgentAndroid/files/SUTAgent.ini')
>+    dm = DeviceManagerSUT(ip, port)
>+    dm.pushFile(filename, '/data/data/com.mozilla.SUTAgentAndroid/files/SUTAgent.ini')
> 
>-parser = OptionParser()
>-defaults = {}
>-parser.add_option("-i", "--ip", dest="ip", help="IP address of device")
>-parser.add_option("-p", "--port", dest="port", help="Agent port on device, defaults to 20701")
>-defaults["port"] = "20701"
>-parser.add_option("-f", "--file", dest="file", help="SUTAgent.ini file to copy to device, must be named 'SUTAgent.ini' and defaults to checking in the local directory for the file.")
>-defaults["file"] = "SUTAgent.ini"
>-parser.set_defaults(**defaults)
>+if __name__ == '__main__':
>+    parser = OptionParser()
>+    defaults = {}
>+    parser.add_option("-i", "--ip", dest="ip", help="IP address of device")
>+    parser.add_option("-p", "--port", dest="port", help="Agent port on device, defaults to 20701")

Just a quick note about code like this. In Eideticker I also gave the option of being able to set the options like device ip via environment variables, using the same conventions as mochitest (https://github.com/mozilla/eideticker/blob/master/src/eideticker/eideticker/device.py#L229). It might be nice if we could do this across harnesses/test frameworks. In this case, we'd only to use the TEST_DEVICE environment variable.

(and yes, I know this is just a tiny little helper utility so it probably doesn't matter that much... but it gave me a convenient opportunity to expound on this)

>diff --git a/worker.py b/worker.py
>index 231de8d..c6ab2dd 100644
>--- a/worker.py
>+++ b/worker.py
>@@ -4,17 +4,20 @@
> 
> ...
> 
>-import androidutils
>+from mozdevice.devicemanagerSUT import AgentError

I don't think we should use AgentError here. I'd prefer to define our own exception, rather than throwing something that's supposed to be internal API to DeviceManager.

> ...
>+    def check_sdcard(self):
>+        dev_root = self.dm.getDeviceRoot()
>         if dev_root is None:
>             logging.error('no response from device when querying device root')
>             return False
>         d = dev_root + '/autophonetest'
>-        androidutils.run_adb('shell', ['rmdir', d], serial=self.phone_cfg['serial'], timeout=15)
>-        out = androidutils.run_adb('shell', ['mkdir', d], serial=self.phone_cfg['serial'], timeout=15)
>-        if not out:
>-            # Sometimes we don't get an error creating the dir, but we do
>-            # when changing to it. Blah.
>-            out = androidutils.run_adb('shell', ['cd', d], serial=self.phone_cfg['serial'], timeout=15)            
>-        if out:
>+        self.dm.removeDir(d)
>+        success = self.dm.mkDir(d) and self.dm.dirExists(d)
>+        if not success:
>             logging.error('device root is not writable!')
>             logging.error(out)
>             logging.info('checking sdcard...')
>-            out = androidutils.run_adb('shell', ['mkdir', '/mnt/sdcard/tests/autophonetest'], serial=self.phone_cfg['serial'], timeout=15)
>-            if out:
>-                logging.error(out)
>-            else:
>+            sdcard_writable = self.dm.mkDir('/mnt/sdcard/tests/autophonetest')
>+            if sdcard_writable:
>                 logging.error('weird, sd card is writable but device root isn\'t! I\'m confused and giving up anyway!')
>             self.clear_test_base_paths()
>             return False
>-        androidutils.run_adb('shell', ['rmdir', d], serial=self.phone_cfg['serial'], timeout=15)
>+        self.dm.removeDir(d)
>         return True

I'd like to see this method inside mozdevice (probably in devicemanager.py for maximum reusability between android and b2g). 

>     def ping(self):
>         logging.info('Pinging phone')
>         # verify that the phone is still responding
>-        response = androidutils.run_adb('shell',
>-                                        ['echo', 'autophone'],
>-                                        self.phone_cfg['serial'])
>-        if response:
>+        output = StringIO.StringIO()
>+        response = self.dm.shell(['echo', 'autophone'], output)
>+        if 'autophone' in output.getvalue():
>             logging.info('Pong!')
>             return True

As with check_sdcard, something like this would be useful for mozdevice generally.
Attachment #645352 - Flags: review?(wlachance) → review-
(In reply to William Lachance (:wlach) from comment #2)
> Comment on attachment 645352 [details] [diff] [review]
> Changed adb usage to DeviceManagerSUT
> >diff --git a/autophone.py b/autophone.py
> >index ff7ba3c..3903a05 100644
> >--- a/autophone.py
> >+++ b/autophone.py
> >@@ -28,14 +28,13 @@ try:
> > except ImportError:
> >     import simplejson
> > 
> >-import androidutils
> >-import builds
> >-import phonetest
> >-
> > from manifestparser import TestManifest
> >+from mozdevice.devicemanager import NetworkTools
> 
> Just FYI, mozhttpd also has a copy of a similar method (get_lan_ip) as the
> getLanIp() method of NetworkTools. 
> 
> Someday it would be nice to settle on one definitive interface for that. A
> few months ago I thought mozhttpd would be the right place for that sort of
> method, but I guess maybe not.

Yeah that's definitely out of scope of the functionality of mozhttpd--and of mozdevice for that matter. Not sure where it should go, though.

> 
> >+class DeviceManagerSUT(mozdevice.devicemanagerSUT.DeviceManagerSUT):
> >+
> >+    def getInfo(self, directive=None):
> >+        """Overriden for uptimemillis info directive."""
> >+        ...
> 
> Why is this functionality not in the upstream devicemanagerSUT? (likewise:
> where is the code to the agent which supports the "uptimemillis" directive)

Right, filed as bug 777331.

> 
> >+    """Overriding to not use su -c."""
> >+    def shell(self, cmd, outputfile, env=None, cwd=None):
> >+        cmdline = self._escapedCommandLine(cmd)
> >+        if env:
> >+            cmdline = '%s %s' % (self.formatEnvString(env), cmdline)
> >+
> >+        try:
> >+            if cwd:
> >+                self.sendCmds([{ 'cmd': 'execcwd %s %s' % (cwd, cmdline) }],
> >+                              outputfile)
> >+            else:
> >+                import logging
> >+                logging.debug('cmdline: %s' % cmdline)
> >+                self.sendCmds([{ 'cmd': 'exec "%s"' % cmdline }], outputfile)
> >+        except mozdevice.devicemanager.AgentError:
> >+            return None
> >+
> >+        # dig through the output to get the return code
> >+        lastline = _pop_last_line(outputfile)
> >+        if lastline:
> >+            m = re.search('return code \[([0-9]+)\]', lastline)
> >+            if m:
> >+                return int(m.group(1))
> >+
> >+        # woops, we couldn't find an end of line/return value
> >+        return None
> 
> Let's link to bug 777034 in a comment for this one.

I think I will take this out, since apparently the su stuff was just removed from the Agent. If it changes again, we'll modify mozdevice.

> Just a quick note about code like this. In Eideticker I also gave the option
> of being able to set the options like device ip via environment variables,
> using the same conventions as mochitest
> (https://github.com/mozilla/eideticker/blob/master/src/eideticker/eideticker/
> device.py#L229). It might be nice if we could do this across harnesses/test
> frameworks. In this case, we'd only to use the TEST_DEVICE environment
> variable.
> 
> (and yes, I know this is just a tiny little helper utility so it probably
> doesn't matter that much... but it gave me a convenient opportunity to
> expound on this)

Excellent idea. Done. I also cleaned the hell up out of the file...which actually should be in mozdevice, probably, since it's used to configure the SUTAgent. Agreed?

> 
> >diff --git a/worker.py b/worker.py
> >index 231de8d..c6ab2dd 100644
> >--- a/worker.py
> >+++ b/worker.py
> >@@ -4,17 +4,20 @@
> > 
> > ...
> > 
> >-import androidutils
> >+from mozdevice.devicemanagerSUT import AgentError
> 
> I don't think we should use AgentError here. I'd prefer to define our own
> exception, rather than throwing something that's supposed to be internal API
> to DeviceManager.

Hm, it wasn't clear to me that it was meant to be internal, but yeah I see that sometimes it is caught. I think we need something in DeviceManager, then, to signal a fatal error (most likely a failure to (re)connect). I'll just remove it for now.

> I'd like to see this method inside mozdevice (probably in devicemanager.py
> for maximum reusability between android and b2g). 
> ...
> As with check_sdcard, something like this would be useful for mozdevice
> generally.

Agreed. I will migrate this over at some point, along with rxdroid stuff. For now I will leave it in, since that's a separate task.
Addressed comments in review. As mentioned, I just deleted AgentError usage for now. We'll have to come up with another way to be notified of fatal errors.
Attachment #645352 - Attachment is obsolete: true
Attachment #646437 - Flags: review?(wlachance)
Comment on attachment 646437 [details] [diff] [review]
Changed adb usage to DeviceManagerSUT, take 2

LGTM, as far as I can tell. Big +1 on adding the publishAgentIni utility to mozdevice (maybe just as a subcommand of the existing sut tool?)
Attachment #646437 - Flags: review?(wlachance) → review+
Just realized I forgot to remove some adb usage from s1s2test.py. I'll get on that.
Attached patch Fix s1s2 testSplinter Review
This converts the s1s2 test over to use SUT, which I forgot to do in the last patch.
Attachment #649637 - Flags: review?(wlachance)
Comment on attachment 649637 [details] [diff] [review]
Fix s1s2 test

LGTM
Attachment #649637 - Flags: review?(wlachance) → review+
This mostly works. There are some problems apparently with SUT crashes, but they have been filed separately.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.