573263 - Refactor RemoteRefTest to work on Android, and create a shared RemoteAutomation class

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[cmtalbert]

Attachment: Refactor Patch v1

We needed to solve a number of things to make reftests run remote on android.
* Remove our "smart" path creation on the device
* Use the devicemanager test root
* Formalize the use of a standard "remoteAutomation" class rather than using almost identical code for both mochitest and reftest
* Make the options between mochitest's runtestsremote and reftests remotereftest have the same cli parameters and have those cli parameters work the same way. (Future target for refactoring into a shared options class perhaps).
* Provide the ability to roll an extension onto a profile as though the extension were installed (the redirection trick does NOT work on mobile)
* Handle Fennec's issue in bug 570027.

= Changes in Behavior =
* RemoteAutomation.py::waitForFinish will now respect the maxTime setting and will ensure that fennec is down should we time out.
* found a bug where xpcshell won't shut down the webserver on its own due to pointing at memory still in use, and waits forever.  Solved by having it do a non-blocking poll() and terminate if still running after the poll()
* We now require either --app or --remote-app-path to be set.  --app is useful when you have a global executable name for the application under test (like on android).  --remote-app-path is a path relative to device root that will launch your application under test.  If we were doing winmo it'd be: fennec/fennec.exe for instance.  This way we don't try to construct paths and we force the product that ultimately installed fennec (i.e. the system that is calling us) to know where fennec is on the device and how to launch it.
* Accidentally corrected some spacing issues in runtestsremote.py will attach a follow-on spaces only correction bug to correct the rest of them.
* Refactored reftest to use a verifyOptions step instead of doing verifyOptions work in main, also changed the "automation" variable to have teh same "_automation" name across classes to make code easier to follow and easier to refactor in the future.

= Questions =
* There are some old xulrunner settings for xre-path and utility-path in remotereftest.  I commented them out, I'm not sure if they are needed.
* I also left the old way (the link method) of extension installation in the code but commented out. I thought about putting it into my "installExtension" method as an optional method of extension installation, but I tend to think that doing extension installation the same way across all the test harnesses and using the same method that real extensions use is more valuable than testing this little hacky work around.  I'm going to run this on try server and see if doing true extension installs has any ill effects.  If not, I'm thinking of removing this code.

Comment 1

[cmtalbert]

Attachment: Space cleanup runtestsremote.py

We had a bunch of 2 space indenting that crept into this file, but most of the file is 4 space indenting.  Switched to 4 space indenting through the whole thing.
This patch only changes spacing.

Comment 2

[cmtalbert]

Attachment: Refactor Patch v2

Fixed a mistake in the makefiles.

Comment 3

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

Comment on attachment 452644 [details] [diff] [review]
Refactor Patch v2

>diff --git a/build/remoteautomation.py b/build/remoteautomation.py
>+SCRIPT_DIRECTORY = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])))
>+sys.path.append(SCRIPT_DIRECTORY)
>+#os.chdir(SCRIPT_DIRECTORY)         

we have this commented out, do we need the sys.path.append also?  

>+    def waitForFinish(self, proc, utilityPath, timeout, maxTime, startTime, debuggerInfo):
>+        # maxTime is used to override the default timeout, we should honor that
>+        status = proc.wait(timeout = maxTime)
>+
>+        # TODO: consider pulling log file from remote
>+        print proc.stdout

actually this TODO comment is pointless because proc.stdout does a getFile from the remote device :)

>+    # be careful here as this inner class doesn't have access to outer class members    
>+    class RProcess(object):
>+        # device manager process
>+        dm = None
>+        # TODO: Looks like appName is unused here
>+        def __init__(self, dm, appName, cmd, stdout = None, stderr = None, env = None, cwd = '.'):
>+            self.dm = dm
>+            print "going to launch process: " + str(self.dm.host)
>+            self.proc = dm.launchProcess(cmd)
>+            exepath = cmd[0]
>+            name = exepath.split('/')[-1]
>+            self.procName = name

We can remove the appName as it is not exepath = cmd[0].  RProcess is only called from the Process() function above it, so we should be safe.


For remoteautomation.py, I think it should live in build/mobile/remoteautomation.py.  Also, I would like to consider putting the remoteOptions class into it.  I know that is shoving extra stuff into this file, but we are duplicating code.  Maybe another new file for remoteOptions is better, but I think we can easily move this stuff into the same file.


>diff --git a/layout/tools/reftest/remotereftest.py b/layout/tools/reftest/remotereftest.py
>--- a/layout/tools/reftest/remotereftest.py
>+++ b/layout/tools/reftest/remotereftest.py
>@@ -42,205 +42,164 @@ import time
> SCRIPT_DIRECTORY = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])))
> sys.path.append(SCRIPT_DIRECTORY)
> #os.chdir(SCRIPT_DIRECTORY)         
> 
> from automation import Automation

do we need this anymore?  All references to Automation are actually remoteAutomation instances.


> class RemoteOptions(ReftestOptions):

I am thinking here that we have a mobileOptions defined in remoteAutomation.py, then we could do a:"
class RemoteOptions(ReftestOptions, MobileOptions):
 ...


> class RemoteReftest(RefTest):
>+    def copyExtraFilesToProfile(self, options, profileDir):
>+        pass

why do we pass here?  This is done out of band from the profile creation in runreftest.py

>+    def registerExtension(self, browserEnv, options, profileDir, extraArgs = ['-silent'] ):
>+        self.automation.log.info("REFTEST INFO | runreftest.py | Performing extension manager registration: start.\n")
>+        # Because our startProcess code doesn't return until fennec starts we just give it
>+        # a maxTime of 20 secs before timing it out and ensuring it is dead.
>+        # Besides registering the extension, this works around fennec bug 570027
>+        status = self.automation.runApp(None, browserEnv, options.app, profileDir,
>+                                   extraArgs,
>+                                   utilityPath = options.utilityPath,
>+                                   xrePath=options.xrePath,
>+                                   symbolsPath=options.symbolsPath,
>+                                   maxTime = 20)
>+        # We don't care to call |processLeakLog()| for this step.
>+        self.automation.log.info("\nREFTEST INFO | runreftest.py | Performing extension manager registration: end.")

does this actually work?  On winmo I had serious problems getting this to launch an app and the -silent flag wasn't working at all.  Have we see the maxTime variable work via remote?  Also, we are passing in symbolsPath, not sure if that really is valid for remote scenarios.  It is too bad we can't figure out a cleaner way to get maxTime in the normal registerExtension:)  Maybe set it as a cli arg in runreftest to a value of None, then we could just call the parent class with a new maxTime.  Otherwise, maxTime gets passed into remoteAutomation.waitForFinish and we give it default values there.


>diff --git a/layout/tools/reftest/remotereftest.py b/layout/tools/reftest/remotereftest.py

>+    #profileExtensionsPath = os.path.join(profileDir, "extensions")
>+    #os.mkdir(profileExtensionsPath)
>+    #reftestExtensionPath = os.path.join(SCRIPT_DIRECTORY, "reftest")
>+    #extFile = open(os.path.join(profileExtensionsPath, "[e-mail]"), "w")
>+    #extFile.write(reftestExtensionPath)
>+    #extFile.close()
>+    self.automation.installExtension(os.path.join(SCRIPT_DIRECTORY, "reftest"),
>+                                                  profileDir,
>+                                                  "[e-mail]")
>+

I wouldn't want to leave these comments in the file for checkin.


>diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in
>--- a/testing/mochitest/Makefile.in
>+++ b/testing/mochitest/Makefile.in
>@@ -61,16 +61,17 @@ include $(topsrcdir)/build/automation-bu
> _SERV_FILES = 	\
> 		runtests.py \
> 		automation.py \
> 		runtestsremote.py \
> 		runtestsvmware.py \
> 		$(topsrcdir)/build/mobile/devicemanager.py \
> 		$(topsrcdir)/build/automationutils.py \
> 		$(topsrcdir)/build/poster.zip \
>+    $(topsrcdir)/build/remoteautomation.py \
> 		gen_template.pl \
> 		server.js \
> 		harness-a11y.xul \
> 		harness-overlay.xul \
> 		harness.xul \
> 		browser-test-overlay.xul \
> 		browser-test.js \
> 		browser-harness.xul \

could you add two tabs so this lines up with the rest of the file?  Also my previous comment of moving it from build/ to build/mobile/


>diff --git a/testing/mochitest/runtestsremote.py b/testing/mochitest/runtestsremote.py
> from automation import Automation
>+from remoteautomation import RemoteAutomation
> from runtests import Mochitest
> from runtests import MochitestOptions
> from runtests import MochitestServer
> 

we need Automation in this file for the local webserver, could the be same for reftest as well.


Overall this is a good patch and cleanup.  r=me with the little nits cleaned up.  For the consolidation of options, we could do that in another followup bug.

Comment 4

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

Comment on attachment 452488 [details] [diff] [review]
Space cleanup runtestsremote.py

good catch.  Make sure this is compatible with all patches...would rather check this in after the other patches to avoid bitrot on functionality.

Comment 5

[cmtalbert]

Forgot to mark as fixed, landed as changeset: abddae3485f9