Bug 1440674 - Simplify some code in remoteautomation.py; r=jmaher
authorGeoff Brown <gbrown@mozilla.com>
Fri, 23 Feb 2018 09:47:57 -0700
changeset 759091 b494bb88b01ca0c28db17cb0f5fbaf56ffe561eb
parent 759090 a5fdaf6da588414deb6eda32d1f27345dfa7a17e
child 759092 e379efa4ff54e48138e28de17e2d7b59e9724789
push id100272
push userrwood@mozilla.com
push dateFri, 23 Feb 2018 18:27:33 +0000
reviewersjmaher
bugs1440674
milestone60.0a1
Bug 1440674 - Simplify some code in remoteautomation.py; r=jmaher remove RemoteAutomation._product and product options; remove some unused code paths; remove "blobber" references; remove incorrect and/or unhelpful comments; s/_devicemanager/_dm/ (shorter, improve readability)
build/mobile/remoteautomation.py
layout/tools/reftest/reftestcommandline.py
layout/tools/reftest/remotereftest.py
testing/mochitest/mochitest_options.py
testing/mochitest/runrobocop.py
testing/mochitest/runtestsremote.py
--- a/build/mobile/remoteautomation.py
+++ b/build/mobile/remoteautomation.py
@@ -5,62 +5,52 @@
 import datetime
 import glob
 import time
 import re
 import os
 import posixpath
 import tempfile
 import shutil
-import subprocess
 import sys
 
 from automation import Automation
 from mozdevice import DMError, DeviceManager
 from mozlog import get_default_logger
 from mozscreenshot import dump_screen
 import mozcrash
 
 # signatures for logcat messages that we don't care about much
 fennecLogcatFilters = [ "The character encoding of the HTML document was not declared",
                         "Use of Mutation Events is deprecated. Use MutationObserver instead.",
                         "Unexpected value from nativeGetEnabledTags: 0" ]
 
 class RemoteAutomation(Automation):
     _devicemanager = None
 
-    # Part of a hack for Robocop: "am COMMAND" is handled specially if COMMAND
-    # is in this set. See usages below.
-    _specialAmCommands = ('instrument', 'start')
-
     def __init__(self, deviceManager, appName = '', remoteLog = None,
                  processArgs=None):
-        self._devicemanager = deviceManager
+        self._dm = deviceManager
         self._appName = appName
         self._remoteProfile = None
         self._remoteLog = remoteLog
         self._processArgs = processArgs or {};
 
-        # Default our product to fennec
-        self._product = "fennec"
         self.lastTestSeen = "remoteautomation.py"
         Automation.__init__(self)
 
     def setDeviceManager(self, deviceManager):
-        self._devicemanager = deviceManager
+        self._dm = deviceManager
 
     def setAppName(self, appName):
         self._appName = appName
 
     def setRemoteProfile(self, remoteProfile):
         self._remoteProfile = remoteProfile
 
-    def setProduct(self, product):
-        self._product = product
-
     def setRemoteLog(self, logfile):
         self._remoteLog = logfile
 
     # Set up what we need for the remote environment
     def environment(self, env=None, xrePath=None, crashreporter=True, debugger=False, lsanPath=None, ubsanPath=None):
         # Because we are running remote, we don't want to mimic the local env
         # so no copying of os.environ
         if env is None:
@@ -106,17 +96,17 @@ class RemoteAutomation(Automation):
             If maxTime seconds elapse or no output is detected for timeout
             seconds, kill the process and fail the test.
         """
         proc.utilityPath = utilityPath
         # maxTime is used to override the default timeout, we should honor that
         status = proc.wait(timeout = maxTime, noOutputTimeout = timeout)
         self.lastTestSeen = proc.getLastTestSeen
 
-        topActivity = self._devicemanager.getTopActivity()
+        topActivity = self._dm.getTopActivity()
         if topActivity == proc.procName:
             print "Browser unexpectedly found running. Killing..."
             proc.kill(True)
         if status == 1:
             if maxTime:
                 print "TEST-UNEXPECTED-FAIL | %s | application ran for longer than " \
                       "allowed maximum time of %s seconds" % (self.lastTestSeen, maxTime)
             else:
@@ -128,75 +118,73 @@ class RemoteAutomation(Automation):
 
         return status
 
     def deleteANRs(self):
         # empty ANR traces.txt file; usually need root permissions
         # we make it empty and writable so we can test the ANR reporter later
         traces = "/data/anr/traces.txt"
         try:
-            self._devicemanager.shellCheckOutput(['echo', '', '>', traces], root=True,
-                                                 timeout=DeviceManager.short_timeout)
-            self._devicemanager.shellCheckOutput(['chmod', '666', traces], root=True,
-                                                 timeout=DeviceManager.short_timeout)
+            self._dm.shellCheckOutput(['echo', '', '>', traces], root=True,
+                                       timeout=DeviceManager.short_timeout)
+            self._dm.shellCheckOutput(['chmod', '666', traces], root=True,
+                                       timeout=DeviceManager.short_timeout)
         except DMError:
             print "Error deleting %s" % traces
             pass
 
     def checkForANRs(self):
         traces = "/data/anr/traces.txt"
-        if self._devicemanager.fileExists(traces):
+        if self._dm.fileExists(traces):
             try:
-                t = self._devicemanager.pullFile(traces)
+                t = self._dm.pullFile(traces)
                 if t:
                     stripped = t.strip()
                     if len(stripped) > 0:
                         print "Contents of %s:" % traces
                         print t
                 # Once reported, delete traces
                 self.deleteANRs()
             except DMError:
                 print "Error pulling %s" % traces
             except IOError:
                 print "Error pulling %s" % traces
         else:
             print "%s not found" % traces
 
     def deleteTombstones(self):
-        # delete any existing tombstone files from device
+        # delete any tombstone files from device
         tombstones = "/data/tombstones/*"
         try:
-            self._devicemanager.shellCheckOutput(['rm', '-r', tombstones], root=True,
-                                                 timeout=DeviceManager.short_timeout)
+            self._dm.shellCheckOutput(['rm', '-r', tombstones], root=True,
+                                       timeout=DeviceManager.short_timeout)
         except DMError:
             # This may just indicate that the tombstone directory is missing
             pass
 
     def checkForTombstones(self):
         # pull any tombstones from device and move to MOZ_UPLOAD_DIR
         remoteDir = "/data/tombstones"
-        blobberUploadDir = os.environ.get('MOZ_UPLOAD_DIR', None)
-        if blobberUploadDir:
-            if not os.path.exists(blobberUploadDir):
-                os.mkdir(blobberUploadDir)
-            if self._devicemanager.dirExists(remoteDir):
-                # copy tombstone files from device to local blobber upload directory
+        uploadDir = os.environ.get('MOZ_UPLOAD_DIR', None)
+        if uploadDir:
+            if not os.path.exists(uploadDir):
+                os.mkdir(uploadDir)
+            if self._dm.dirExists(remoteDir):
+                # copy tombstone files from device to local upload directory
                 try:
-                    self._devicemanager.shellCheckOutput(['chmod', '777', remoteDir], root=True,
-                                                 timeout=DeviceManager.short_timeout)
-                    self._devicemanager.shellCheckOutput(['chmod', '666', os.path.join(remoteDir, '*')], root=True,
-                                                 timeout=DeviceManager.short_timeout)
-                    self._devicemanager.getDirectory(remoteDir, blobberUploadDir, False)
+                    self._dm.shellCheckOutput(['chmod', '777', remoteDir], root=True,
+                                               timeout=DeviceManager.short_timeout)
+                    self._dm.shellCheckOutput(['chmod', '666', os.path.join(remoteDir, '*')],
+                                               root=True, timeout=DeviceManager.short_timeout)
+                    self._dm.getDirectory(remoteDir, uploadDir, False)
                 except DMError:
                     # This may just indicate that no tombstone files are present
                     pass
                 self.deleteTombstones()
-                # add a .txt file extension to each tombstone file name, so
-                # that blobber will upload it
-                for f in glob.glob(os.path.join(blobberUploadDir, "tombstone_??")):
+                for f in glob.glob(os.path.join(uploadDir, "tombstone_??")):
                     # add a unique integer to the file name, in case there are
                     # multiple tombstones generated with the same name, for
                     # instance, after multiple robocop tests
                     for i in xrange(1, sys.maxint):
                         newname = "%s.%d.txt" % (f, i)
                         if not os.path.exists(newname):
                             os.rename(f, newname)
                             break
@@ -204,113 +192,96 @@ class RemoteAutomation(Automation):
                 print "%s does not exist; tombstone check skipped" % remoteDir
         else:
             print "MOZ_UPLOAD_DIR not defined; tombstone check skipped"
 
     def checkForCrashes(self, directory, symbolsPath):
         self.checkForANRs()
         self.checkForTombstones()
 
-        logcat = self._devicemanager.getLogcat(filterOutRegexps=fennecLogcatFilters)
+        logcat = self._dm.getLogcat(filterOutRegexps=fennecLogcatFilters)
 
         javaException = mozcrash.check_for_java_exception(logcat, test_name=self.lastTestSeen)
         if javaException:
             return True
 
         # If crash reporting is disabled (MOZ_CRASHREPORTER!=1), we can't say
         # anything.
         if not self.CRASHREPORTER:
             return False
 
         try:
             dumpDir = tempfile.mkdtemp()
             remoteCrashDir = posixpath.join(self._remoteProfile, 'minidumps')
-            if not self._devicemanager.dirExists(remoteCrashDir):
+            if not self._dm.dirExists(remoteCrashDir):
                 # If crash reporting is enabled (MOZ_CRASHREPORTER=1), the
                 # minidumps directory is automatically created when Fennec
                 # (first) starts, so its lack of presence is a hint that
                 # something went wrong.
                 print "Automation Error: No crash directory (%s) found on remote device" % remoteCrashDir
-                # Whilst no crash was found, the run should still display as a failure
                 return True
-            self._devicemanager.getDirectory(remoteCrashDir, dumpDir)
+            self._dm.getDirectory(remoteCrashDir, dumpDir)
 
             logger = get_default_logger()
-            if logger is not None:
-                crashed = mozcrash.log_crashes(logger, dumpDir, symbolsPath, test=self.lastTestSeen)
-            else:
-                crashed = Automation.checkForCrashes(self, dumpDir, symbolsPath)
+            crashed = mozcrash.log_crashes(logger, dumpDir, symbolsPath, test=self.lastTestSeen)
 
         finally:
             try:
                 shutil.rmtree(dumpDir)
             except:
                 print "WARNING: unable to remove directory: %s" % dumpDir
         return crashed
 
     def buildCommandLine(self, app, debuggerInfo, profileDir, testURL, extraArgs):
         # If remote profile is specified, use that instead
-        if (self._remoteProfile):
+        if self._remoteProfile:
             profileDir = self._remoteProfile
 
         # Hack for robocop, if app & testURL == None and extraArgs contains the rest of the stuff, lets
         # assume extraArgs is all we need
-        if app == "am" and extraArgs[0] in RemoteAutomation._specialAmCommands:
+        if app == "am" and extraArgs[0] in ('instrument', 'start'):
             return app, extraArgs
 
         cmd, args = Automation.buildCommandLine(self, app, debuggerInfo, profileDir, testURL, extraArgs)
-        # Remove -foreground if it exists, if it doesn't this just returns
         try:
             args.remove('-foreground')
         except:
             pass
-#TODO: figure out which platform require NO_EM_RESTART
-#        return app, ['--environ:NO_EM_RESTART=1'] + args
         return app, args
 
     def Process(self, cmd, stdout = None, stderr = None, env = None, cwd = None):
-        if stdout == None or stdout == -1 or stdout == subprocess.PIPE:
-            stdout = self._remoteLog
-
-        return self.RProcess(self._devicemanager, cmd, stdout, stderr, env, cwd, self._appName,
+        return self.RProcess(self._dm, cmd, self._remoteLog, env, cwd, self._appName,
                              **self._processArgs)
 
-    # be careful here as this inner class doesn't have access to outer class members
     class RProcess(object):
-        # device manager process
         dm = None
-        def __init__(self, dm, cmd, stdout=None, stderr=None, env=None, cwd=None, app=None,
+        def __init__(self, dm, cmd, stdout=None, env=None, cwd=None, app=None,
                      messageLogger=None, counts=None):
             self.dm = dm
             self.stdoutlen = 0
             self.lastTestSeen = "remoteautomation.py"
             self.proc = dm.launchProcess(cmd, stdout, cwd, env, True)
             self.messageLogger = messageLogger
             self.utilityPath = None
 
             self.counts = counts
             if self.counts is not None:
                 self.counts['pass'] = 0
                 self.counts['fail'] = 0
                 self.counts['todo'] = 0
 
-            if (self.proc is None):
-                if cmd[0] == 'am':
-                    self.proc = stdout
-                else:
-                    raise Exception("unable to launch process")
-            self.procName = cmd[0].split('/')[-1]
-            if cmd[0] == 'am' and cmd[1] in RemoteAutomation._specialAmCommands:
+            if self.proc is None:
+                self.proc = stdout
+            self.procName = cmd[0].split(posixpath.sep)[-1]
+            if cmd[0] == 'am' and cmd[1] in ('instrument', 'start'):
                 self.procName = app
 
             # Setting timeout at 1 hour since on a remote device this takes much longer.
             # Temporarily increased to 90 minutes because no more chunks can be created.
             self.timeout = 5400
-            # The benefit of the following sleep is unclear; it was formerly 15 seconds
-            time.sleep(1)
 
             # Used to buffer log messages until we meet a line break
             self.logBuffer = ""
 
         @property
         def pid(self):
             pid = self.dm.processExist(self.procName)
             # HACK: we should probably be more sophisticated about monitoring
--- a/layout/tools/reftest/reftestcommandline.py
+++ b/layout/tools/reftest/reftestcommandline.py
@@ -435,24 +435,16 @@ class RemoteArgumentsParser(ReftestArgum
 
         self.add_argument("--devicePort",
                           action="store",
                           type=str,
                           default="20701",
                           dest="devicePort",
                           help="port of remote device to test")
 
-        self.add_argument("--remote-product-name",
-                          action="store",
-                          type=str,
-                          dest="remoteProductName",
-                          default="fennec",
-                          help="Name of product to test - either fennec or firefox, "
-                               "defaults to fennec")
-
         self.add_argument("--remote-webserver",
                           action="store",
                           type=str,
                           dest="remoteWebServer",
                           help="IP Address of the webserver hosting the reftest content")
 
         self.add_argument("--http-port",
                           action="store",
@@ -498,17 +490,17 @@ class RemoteArgumentsParser(ReftestArgum
                           action="store_false",
                           dest="printDeviceInfo",
                           default=True,
                           help="do not display verbose diagnostics about the remote device")
 
     def validate_remote(self, options, automation):
         # Ensure our defaults are set properly for everything we can infer
         if not options.remoteTestRoot:
-            options.remoteTestRoot = automation._devicemanager.deviceRoot + \
+            options.remoteTestRoot = automation._dm.deviceRoot + \
                 '/reftest'
         options.remoteProfile = options.remoteTestRoot + "/profile"
 
         if options.remoteWebServer is None:
             options.remoteWebServer = self.get_ip()
 
         # Verify that our remotewebserver is set properly
         if options.remoteWebServer == '127.0.0.1':
@@ -563,17 +555,17 @@ class RemoteArgumentsParser(ReftestArgum
         # httpd-path is specified by standard makefile targets and may be specified
         # on the command line to select a particular version of httpd.js. If not
         # specified, try to select the one from hostutils.zip, as required in
         # bug 882932.
         if not options.httpdPath:
             options.httpdPath = os.path.join(options.utilityPath, "components")
 
         if not options.ignoreWindowSize:
-            parts = automation._devicemanager.getInfo(
+            parts = automation._dm.getInfo(
                 'screen')['screen'][0].split()
             width = int(parts[0].split(':')[1])
             height = int(parts[1].split(':')[1])
             if (width < 1366 or height < 1050):
                 self.error("ERROR: Invalid screen resolution %sx%s, "
                            "please adjust to 1366x1050 or higher" % (
                             width, height))
 
--- a/layout/tools/reftest/remotereftest.py
+++ b/layout/tools/reftest/remotereftest.py
@@ -197,18 +197,17 @@ class RemoteReftest(RefTest):
             localAutomation.IS_MAC = True
         elif (hostos == 'linux' or hostos == 'linux2'):
             localAutomation.IS_LINUX = True
             localAutomation.UNIXISH = True
         elif (hostos == 'win32' or hostos == 'win64'):
             localAutomation.BIN_SUFFIX = ".exe"
             localAutomation.IS_WIN32 = True
 
-        paths = [options.xrePath, localAutomation.DIST_BIN, self.automation._product,
-                 os.path.join('..', self.automation._product)]
+        paths = [options.xrePath, localAutomation.DIST_BIN]
         options.xrePath = self.findPath(paths)
         if options.xrePath is None:
             print ("ERROR: unable to find xulrunner path for %s, "
                    "please specify with --xre-path" % (os.name))
             return 1
         paths.append("bin")
         paths.append(os.path.join("..", "bin"))
 
@@ -410,19 +409,16 @@ def run_test_harness(parser, options):
         traceback.print_exc()
         print ("Automation Error: exception while initializing devicemanager.  "
                "Most likely the device is not in a testable state.")
         return 1
 
     automation = RemoteAutomation(None)
     automation.setDeviceManager(dm)
 
-    if options.remoteProductName:
-        automation.setProduct(options.remoteProductName)
-
     # Set up the defaults and ensure options are set
     parser.validate_remote(options, automation)
 
     # Check that Firefox is installed
     expected = options.app.split('/')[-1]
     installed = dm.shellCheckOutput(['pm', 'list', 'packages', expected])
     if expected not in installed:
         print "%s is not installed on this device" % expected
--- a/testing/mochitest/mochitest_options.py
+++ b/testing/mochitest/mochitest_options.py
@@ -874,23 +874,16 @@ class AndroidArguments(ArgumentContainer
           "suppress": True,
           }],
         [["--devicePort"],
          {"dest": "devicePort",
           "type": int,
           "default": 20701,
           "help": "port of remote device to test",
           }],
-        [["--remote-product-name"],
-         {"dest": "remoteProductName",
-          "default": "fennec",
-          "help": "The executable's name of remote product to test - either \
-                   fennec or firefox, defaults to fennec",
-          "suppress": True,
-          }],
         [["--remote-logfile"],
          {"dest": "remoteLogFile",
           "default": None,
           "help": "Name of log file on the device relative to the device \
                    root. PLEASE ONLY USE A FILENAME.",
           }],
         [["--remote-webserver"],
          {"dest": "remoteWebServer",
--- a/testing/mochitest/runrobocop.py
+++ b/testing/mochitest/runrobocop.py
@@ -59,21 +59,16 @@ class RobocopTestRunner(MochitestDesktop
         self.remoteLog = options.remoteLogFile
         self.auto.setRemoteLog(self.remoteLog)
         self.remoteScreenshots = "/mnt/sdcard/Robotium-Screenshots"
         self.remoteMozLog = os.path.join(options.remoteTestRoot, "mozlog")
         self.auto.setServerInfo(
             self.options.webServer, self.options.httpPort, self.options.sslPort)
         self.localLog = options.logFile
         self.localProfile = None
-        productPieces = self.options.remoteProductName.split('.')
-        if (productPieces is not None):
-            self.auto.setProduct(productPieces[0])
-        else:
-            self.auto.setProduct(self.options.remoteProductName)
         self.auto.setAppName(self.options.remoteappname)
         self.certdbNew = True
         self.remoteCopyAvailable = True
         self.passed = 0
         self.failed = 0
         self.todo = 0
 
     def startup(self):
@@ -176,19 +171,17 @@ class RobocopTestRunner(MochitestDesktop
         """
            Setup xrePath and utilityPath and verify xpcshell.
 
            This is similar to switchToLocalPaths in runtestsremote.py.
         """
         localAutomation = self.makeLocalAutomation()
         paths = [
             self.options.xrePath,
-            localAutomation.DIST_BIN,
-            self.auto._product,
-            os.path.join('..', self.auto._product)
+            localAutomation.DIST_BIN
         ]
         self.options.xrePath = self.findPath(paths)
         if self.options.xrePath is None:
             self.log.error(
                 "unable to find xulrunner path for %s, please specify with --xre-path" %
                 os.name)
             sys.exit(1)
         self.log.debug("using xre path %s" % self.options.xrePath)
--- a/testing/mochitest/runtestsremote.py
+++ b/testing/mochitest/runtestsremote.py
@@ -113,18 +113,16 @@ class MochiRemote(MochitestDesktop):
         remoteXrePath = options.xrePath
         remoteProfilePath = options.profilePath
         remoteUtilityPath = options.utilityPath
 
         localAutomation = self.makeLocalAutomation()
         paths = [
             options.xrePath,
             localAutomation.DIST_BIN,
-            self._automation._product,
-            os.path.join('..', self._automation._product)
         ]
         options.xrePath = self.findPath(paths)
         if options.xrePath is None:
             self.log.error(
                 "unable to find xulrunner path for %s, please specify with --xre-path" %
                 os.name)
             sys.exit(1)
 
@@ -337,21 +335,16 @@ def run_test_harness(parser, options):
 
     # Check that Firefox is installed
     expected = options.app.split('/')[-1]
     installed = dm.shellCheckOutput(['pm', 'list', 'packages', expected])
     if expected not in installed:
         log.error("%s is not installed on this device" % expected)
         return 1
 
-    productPieces = options.remoteProductName.split('.')
-    if (productPieces is not None):
-        auto.setProduct(productPieces[0])
-    else:
-        auto.setProduct(options.remoteProductName)
     auto.setAppName(options.remoteappname)
 
     logParent = os.path.dirname(options.remoteLogFile)
     dm.removeDir(logParent)
     dm.mkDir(logParent)
     auto.setRemoteLog(options.remoteLogFile)
     auto.setServerInfo(options.webServer, options.httpPort, options.sslPort)