595294 - remotereftest.py assumes process.txt exists even when we provide a log file

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

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

for all remote testing, we by default redirect the output to process.txt:
fennec args > path/process.txt

when we query the process in automation.py, we look for proc.stdout which in this case is a deviceManager.getFile(process.txt).  Since we are passing in an output file, we should treat the stdout as a deviceManager.getFile(<logfilename>).

I suggest modifying this code:
http://mxr.mozilla.org/mozilla-central/source/build/mobile/devicemanager.py#390

so that it goes from:
  def launchProcess(self, cmd, outputFile = "process.txt", cwd = ''):
    if (outputFile == "process.txt"):
      outputFile = self.getDeviceRoot() + '/' + "process.txt"

    cmdline = subprocess.list2cmdline(cmd)
    self.fireProcess(cmdline + " > " + outputFile)
    handle = outputFile

    return handle


to:
  def launchProcess(self, cmd, outputFile = "process.txt", cwd = ''):
    cmdline = subprocess.list2cmdline(cmd)
    if (outputFile == "process.txt" || outputFile == None):
      outputFile = self.getDeviceRoot() + '/' + "process.txt"
      self.fireProcess(cmdline + " > " + outputFile)
    else:
      self.fireProcess(cmdline)
    return outputFile


Next in remoteautomation.py, change:
        def __init__(self, dm, 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


to:
        def __init__(self, dm, 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, stdout)
            exepath = cmd[0]
            name = exepath.split('/')[-1]
            self.procName = name


Finally in the RemoteAutomation.init() add a remoteLogFile parameter so we can set it as a member of the class and overload Process.stdout to be self.remoteLogFile.

Of course this is just my idea of how to solve this.  Please comment if this doesn't make sense, otherwise it needs some testing:)

Comment 1

[cmtalbert]

Yeah this makes sense, Let's go your route there, Joel.

Comment 2

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

Attachment: remote reftest uses direct logging instead of stdout (1.0)

when you specify a --remote-logfile='name', we will use that and have reftest log directly to that log file.  This patch builds on that logic and removes the redirect code in devicemanager and remoteautomation and fixes the code to look for the remoteLogFile variable instead of a hardcoded process.txt.

Testing using remote testing on n900.

as a note, this is the devicemanager.py that is currently in the works for talos which would make them synchronized (for once)

Comment 3

[cmtalbert]

Comment on attachment 475386 [details] [diff] [review]
remote reftest uses direct logging instead of stdout (1.0)

>diff --git a/build/mobile/devicemanager.py b/build/mobile/devicemanager.py
>   def launchProcess(self, cmd, outputFile = "process.txt", cwd = ''):
>-    if (outputFile == "process.txt"):
>+    cmdline = subprocess.list2cmdline(cmd)
>+    if (outputFile == "process.txt" or outputFile == None):
>       outputFile = self.getDeviceRoot() + '/' + "process.txt"
>+      cmdline += " > " + outputFile
This is kind of hacky, but I'm ok with that.  I can't think of a cleaner way to do it without extensive refactoring, so let's go with this approach for now.

>@@ -711,8 +710,34 @@ class DeviceManager:
>   """
>   def uninstallAppAndReboot(self, appName, installPath=None):
>     cmd = 'uninst ' + appName
>     if installPath:
>       cmd += ' ' + installPath
>     self.sendCMD([cmd])
>     return True
> 
>+  """
>+    edit the user.js in the profile (on the host machine) and 
>+    add the xpconnect priviledges for the remote server
>+  """
>+  def addRemoteServerPref(self, profile_dir, server):
>+    user_js_filename = os.path.join(profile_dir, 'user.js')
>+    user_js_file = open(user_js_filename, 'a+')
>+   
>+    #TODO: p2 is hardcoded, how do we determine what prefs.js has hardcoded?
>+    remoteCode = """
>+user_pref("capability.principal.codebase.p1.granted", "UniversalPreferencesWrite UniversalXPConnect UniversalPreferencesRead");
>+user_pref("capability.principal.codebase.p1.id", "http://%(server)s");
>+user_pref("capability.principal.codebase.p1.subjectName", "");
>+""" % { "server": server }
>+    user_js_file.write(remoteCode)
>+    user_js_file.close()
This function doesn't belong here.  This should go into the remote testing code that handles setting up profiles for testing.  We should focus on keeping devicemanager.py a simple wrapper to communicate with a remote device.  I want to resist putting specialized things like this into it because if that continues it will make this code a lot harder to maintain.  Please put this into the necessary talos remote code.

The rest of the patch looks great.  r+ with that change.

Comment 4

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

Attachment: remote reftest uses direct logging instead of stdout (1.1)

updated with the devicemanager.py cleanup requested.  kicked off a try server run just to be safe.

Comment 5

[cmtalbert]

http://hg.mozilla.org/mozilla-central/rev/93b05a41880f