Bug 1460914 - [xpcshell] Cleanup code to build the xpcshell command, r=ted
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Wed, 24 Oct 2018 13:56:57 +0000
changeset 491153 9688858bc5daf50a48ff789cf8ecf455ec315c3b
parent 491152 ab0947533e9afba5a0a9e2abd09735bfa3c090a4
child 491154 0ffffb5e0954d2e782ea4da7292d617f435ad3c0
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersted
bugs1460914
milestone65.0a1
Bug 1460914 - [xpcshell] Cleanup code to build the xpcshell command, r=ted The current command line building is hard to follow and make sense of. Let's just use a single command variable and comment the order it needs. Depends on D9549 Differential Revision: https://phabricator.services.mozilla.com/D9550
testing/xpcshell/remotexpcshelltests.py
testing/xpcshell/runxpcshelltests.py
--- a/testing/xpcshell/remotexpcshelltests.py
+++ b/testing/xpcshell/remotexpcshelltests.py
@@ -124,29 +124,27 @@ class RemoteXPCShellTestThread(xpcshell.
     def buildXpcsCmd(self):
         # change base class' paths to remote paths and use base class to build command
         self.xpcshell = posixpath.join(self.remoteBinDir, "xpcw")
         self.headJSPath = posixpath.join(self.remoteScriptsDir, 'head.js')
         self.httpdJSPath = posixpath.join(self.remoteComponentsDir, 'httpd.js')
         self.httpdManifest = posixpath.join(self.remoteComponentsDir, 'httpd.manifest')
         self.testingModulesDir = self.remoteModulesDir
         self.testharnessdir = self.remoteScriptsDir
-        xpcshell.XPCShellTestThread.buildXpcsCmd(self)
+        xpcsCmd = xpcshell.XPCShellTestThread.buildXpcsCmd(self)
         # remove "-g <dir> -a <dir>" and add "--greomni <apk>"
-        del(self.xpcsCmd[1:5])
+        del xpcsCmd[1:5]
         if self.options['localAPK']:
-            self.xpcsCmd.insert(3, '--greomni')
-            self.xpcsCmd.insert(4, self.remoteAPK)
+            xpcsCmd.insert(3, '--greomni')
+            xpcsCmd.insert(4, self.remoteAPK)
 
         if self.remoteDebugger:
             # for example, "/data/local/gdbserver" "localhost:12345"
-            self.xpcsCmd = [
-              self.remoteDebugger,
-              self.remoteDebuggerArgs,
-              self.xpcsCmd]
+            xpcsCmd = [self.remoteDebugger, self.remoteDebuggerArgs] + xpcsCmd
+        return xpcsCmd
 
     def killTimeout(self, proc):
         self.kill(proc)
 
     def launchProcess(self, cmd, stdout, stderr, env, cwd, timeout=None):
         self.timedout = False
         cmd.insert(1, self.remoteHere)
         cmd = ADBDevice._escape_command_line(cmd)
--- a/testing/xpcshell/runxpcshelltests.py
+++ b/testing/xpcshell/runxpcshelltests.py
@@ -163,17 +163,17 @@ class XPCShellTestThread(Thread):
         self.todoCount = 0
         self.failCount = 0
 
         # Context for output processing
         self.output_lines = []
         self.has_failure_output = False
         self.saw_proc_start = False
         self.saw_proc_end = False
-        self.complete_command = None
+        self.command = None
         self.harness_timeout = kwargs.get('harness_timeout')
         self.timedout = False
 
         # event from main thread to signal work done
         self.event = kwargs.get('event')
         self.done = False  # explicitly set flag so we don't rely on thread.isAlive
 
     def run(self):
@@ -382,30 +382,31 @@ class XPCShellTestThread(Thread):
         return profileDir
 
     def setupMozinfoJS(self):
         mozInfoJSPath = os.path.join(self.profileDir, 'mozinfo.json')
         mozInfoJSPath = mozInfoJSPath.replace('\\', '\\\\')
         mozinfo.output_to_file(mozInfoJSPath)
         return mozInfoJSPath
 
-    def buildCmdHead(self, headfiles, xpcscmd):
+    def buildCmdHead(self):
         """
           Build the command line arguments for the head files,
           along with the address of the webserver which some tests require.
 
           On a remote system, this is overloaded to resolve quoting issues over a
           secondary command line.
         """
+        headfiles = self.getHeadFiles(self.test_object)
         cmdH = ", ".join(['"' + f.replace('\\', '/') + '"'
                          for f in headfiles])
 
         dbgport = 0 if self.jsDebuggerInfo is None else self.jsDebuggerInfo.port
 
-        return xpcscmd + [
+        return [
             '-e', 'const _SERVER_ADDR = "localhost"',
             '-e', 'const _HEAD_FILES = [%s];' % cmdH,
             '-e', 'const _JSDEBUGGER_PORT = %d;' % dbgport,
         ]
 
     def getHeadFiles(self, test):
         """Obtain lists of head- files.  Returns a list of head files.
         """
@@ -433,49 +434,51 @@ class XPCShellTestThread(Thread):
           and test files. On a remote system, we overload this to add additional command
           line arguments, so this gets overloaded.
         """
         # - NOTE: if you rename/add any of the constants set here, update
         #   do_load_child_test_harness() in head.js
         if not self.appPath:
             self.appPath = self.xrePath
 
-        self.xpcsCmd = [
+        xpcsCmd = [
             self.xpcshell,
             '-g', self.xrePath,
             '-a', self.appPath,
             '-r', self.httpdManifest,
             '-m',
             '-s',
             '-e', 'const _HEAD_JS_PATH = "%s";' % self.headJSPath,
             '-e', 'const _MOZINFO_JS_PATH = "%s";' % self.mozInfoJSPath,
         ]
 
         if self.testingModulesDir:
             # Escape backslashes in string literal.
             sanitized = self.testingModulesDir.replace('\\', '\\\\')
-            self.xpcsCmd.extend([
+            xpcsCmd.extend([
                 '-e',
                 'const _TESTING_MODULES_DIR = "%s";' % sanitized
             ])
 
-        self.xpcsCmd.extend(['-f', os.path.join(self.testharnessdir, 'head.js')])
+        xpcsCmd.extend(['-f', os.path.join(self.testharnessdir, 'head.js')])
 
         if self.debuggerInfo:
-            self.xpcsCmd = [self.debuggerInfo.path] + self.debuggerInfo.args + self.xpcsCmd
+            xpcsCmd = [self.debuggerInfo.path] + self.debuggerInfo.args + xpcsCmd
 
         # Automation doesn't specify a pluginsPath and xpcshell defaults to
         # $APPDIR/plugins. We do the same here so we can carry on with
         # setting up every test with its own plugins directory.
         if not self.pluginsPath:
             self.pluginsPath = os.path.join(self.appPath, 'plugins')
 
         self.pluginsDir = self.setupPluginsDir()
         if self.pluginsDir:
-            self.xpcsCmd.extend(['-p', self.pluginsDir])
+            xpcsCmd.extend(['-p', self.pluginsDir])
+
+        return xpcsCmd
 
     def cleanupDir(self, directory, name):
         if not os.path.exists(directory):
             return
 
         # up to TRY_LIMIT attempts (one every second), because
         # the Windows filesystem is slow to react to the changes
         TRY_LIMIT = 25
@@ -524,17 +527,17 @@ class XPCShellTestThread(Thread):
 
     def log_line(self, line):
         """Log a line of output (either a parser json object or text output from
         the test process"""
         if isinstance(line, basestring):
             line = self.fix_text_output(line).rstrip('\r\n')
             self.log.process_output(self.proc_ident,
                                     line,
-                                    command=self.complete_command)
+                                    command=self.command)
         else:
             if 'message' in line:
                 line['message'] = self.fix_text_output(line['message'])
             if 'xpcshell_process' in line:
                 line['thread'] = ' '.join([current_thread().name, line['xpcshell_process']])
             else:
                 line['thread'] = current_thread().name
             self.log.log_raw(line)
@@ -625,37 +628,37 @@ class XPCShellTestThread(Thread):
         test_dir = os.path.dirname(path)
 
         # Create a profile and a temp dir that the JS harness can stick
         # a profile and temporary data in
         self.profileDir = self.setupProfileDir()
         self.tempDir = self.setupTempDir()
         self.mozInfoJSPath = self.setupMozinfoJS()
 
-        self.buildXpcsCmd()
-        head_files = self.getHeadFiles(self.test_object)
-        cmdH = self.buildCmdHead(head_files, self.xpcsCmd)
+        # The order of the command line is important:
+        # 1) Arguments for xpcshell itself
+        self.command = self.buildXpcsCmd()
 
-        # The test file will have to be loaded after the head files.
-        cmdT = self.buildCmdTestFile(path)
+        # 2) Arguments for the head files
+        self.command.extend(self.buildCmdHead())
 
-        args = self.xpcsRunArgs[:]
-        if 'debug' in self.test_object:
-            args.insert(0, '-d')
+        # 3) Arguments for the test file
+        self.command.extend(self.buildCmdTestFile(path))
+        self.command.extend(['-e', 'const _TEST_NAME = "%s";' % name])
 
-        # The test name to log
-        cmdI = ['-e', 'const _TEST_NAME = "%s"' % name]
+        # 4) Arguments for code coverage
+        if self.jscovdir:
+            self.command.extend(
+                ['-e', 'const _JSCOV_DIR = "%s";' % self.jscovdir.replace('\\', '/')])
 
-        # Directory for javascript code coverage output, null by default.
-        cmdC = ['-e', 'const _JSCOV_DIR = null']
-        if self.jscovdir:
-            cmdC = ['-e', 'const _JSCOV_DIR = "%s"' % self.jscovdir.replace('\\', '/')]
-            self.complete_command = cmdH + cmdT + cmdI + cmdC + args
-        else:
-            self.complete_command = cmdH + cmdT + cmdI + args
+        # 5) Runtime arguments
+        if 'debug' in self.test_object:
+            self.command.append('-d')
+
+        self.command.extend(self.xpcsRunArgs)
 
         if self.test_object.get('dmd') == 'true':
             self.env['PYTHON'] = sys.executable
             self.env['BREAKPAD_SYMBOLS_PATH'] = self.symbolsPath
 
         if self.test_object.get('subprocess') == 'true':
             self.env['PYTHON'] = sys.executable
 
@@ -674,19 +677,19 @@ class XPCShellTestThread(Thread):
             testTimer.start()
 
         proc = None
         process_output = None
 
         try:
             self.log.test_start(name)
             if self.verbose:
-                self.logCommand(name, self.complete_command, test_dir)
+                self.logCommand(name, self.command, test_dir)
 
-            proc = self.launchProcess(self.complete_command,
+            proc = self.launchProcess(self.command,
                                       stdout=self.pStdout, stderr=self.pStderr,
                                       env=self.env, cwd=test_dir,
                                       timeout=testTimeoutInterval)
 
             if hasattr(proc, "pid"):
                 self.proc_ident = proc.pid
             else:
                 # On mobile, "proc" is just a file.