563860 - update runtestsremote.py to start webserver and work on different platforms

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

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

Attachment: runreftests.py webserver + platform fixes (1.0)

this is an update to runtestsremote.py after running with python agent on desktop linux and n900.  Also I have done work to auto start the webserver if there is no ip address given.

Comment 1

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

Comment on attachment 443554 [details] [diff] [review]
runreftests.py webserver + platform fixes (1.0)

please review this so we can simplify the end to end remote mochitest experience.

Comment 2

[cmtalbert]

Comment on attachment 443554 [details] [diff] [review]
runreftests.py webserver + platform fixes (1.0)

>diff --git a/testing/mochitest/runtestsremote.py b/testing/mochitest>@@ -229,21 +235,59 @@ class MochiRemote(Mochitest):
>         self.remoteProfile = "/tests/profile"
>         self.remoteLog = options.remoteLogFile
> 
>     def cleanup(self, manifest, options):
>         self._dm.getFile(self.remoteLog, self.localLog)
>         self._dm.removeFile(self.remoteLog)
>         self._dm.removeDir(self.remoteProfile)
> 
>+    def findPath(self, paths, filename = None):
>+      for path in paths:
>+        p = path
>+        if filename:
>+          p = os.path.join(p, filename)
>+        if os.path.exists(self.getFullPath(p)):
>+          return path
>+      return None
>+
>     def startWebServer(self, options):
>-        pass
>-        
>+      """ Create the webserver on the host and start it up """
>+      remoteXrePath = options.xrePath
>+      remoteProfilePath = options.profilePath
>+      remoteUtilityPath = options.utilityPath
>+      localAutomation = Automation()
>+
>+      paths = [localAutomation.DIST_BIN, self._automation._product, os.path.join('..', self._automation._product)]
So this is going to be a problem.  What if I already have a DIST_BIN in automation that exists on my box, but I want to run against a different directory where xpcshell is located such as ../bin?

I hit this case.  I did an --enable-tests android build, so automation.DIST_BIN pointed at the objdir of the android build, and I specified xrepath on the command line to point to ../bin (I'm running in packaged form), but this code threw out xrepath and tried to use the objdir from automation.DIST_BIN which did not work since that xpcshell was an android executable.  I think this is a good idea in a narrow set of cases, but if the user has specified an xrepath we should use that over this guesstimation.

>+      options.xrePath = self.findPath(paths)
>+      if options.xrePath == None:
>+        print "ERROR: unable to find xulrunner path for %s, please specify with --xre-path" % (os.name)
>+        sys.exit(1)
>+      paths.append("bin")
>+      paths.append(os.path.join("..", "bin"))
This feels like we might be getting too smart here.  Can we make this code simpler by having a remote xrepath and a local xrepath and then just use the appropriate one?

Should we make utility-path be for local system and xre-path be for remote?

>+#
>+# utilities to get the local ip address
>+#
>+if os.name != "nt":
>+  import fcntl
>+  import struct
>+  def get_interface_ip(ifname):
>+      s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
>+      return socket.inet_ntoa(fcntl.ioctl(
>+                      s.fileno(),
>+                      0x8915,  # SIOCGIFADDR
>+                      struct.pack('256s', ifname[:15])
>+                      )[20:24])
So, what's the deal here with windows?  If you're on windows we already errored out and told them to use xrepath.  Is there nothing we can do in that case?

I think r- until we get some of this stuff figured out.  The patch does work, which is really awesome compared to all the hacking around we used to have to do to get there. But I think some more design is required to get us correctly using the proper path to start xpcshell.

Comment 3

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

Attachment: runtestsremote.py + webserver + devicemanager + android (2.0)

turning into a bug of many things, but this should solve a lot of things on android.  This patch addresses all concerns in previous review as well as:
* no hardcoded testroot, queries the test-agent for that
* cleans up prompts and parsing code
* uses async process management only (with pid polling)
* supports cli variables better, especially --appname=org.mozilla.fennec

This was tested on my python agent and n900 running fennec.

Comment 4

[cmtalbert]

This is still not working for my xrepath setup.  I'm debugging and trying to figure out what's up.  It is still picking up my built android DIST_BIN tree, which isn't going to work.

Comment 5

[cmtalbert]

Comment on attachment 447456 [details] [diff] [review]
runtestsremote.py + webserver + devicemanager + android (2.0)

This looks great.  I've a few small things.  I've changed them on my side and I'll upload a final patch.

I'll make the comments here because the patch to patch diff on bugz is pitiful at best.
>diff --git a/build/mobile/devicemanager.py b/build/mobile/devicemanager.py
>+  base_prompt = '\$\>'
>+  prompt_sep = '\x00'
>+  prompt_regex = '.*' + base_prompt + prompt_sep
Hot.
>         while (found == False):
>-          if (self.debug >= 3): print "recv'ing..."
>+          if (self.debug >= 4): print "recv'ing..."
Thanks, I hate that line.

>   def fireProcess(self, appname):
>     if (self.debug >= 2): print "FIRE PROC: '" + appname + "'"
>-    self.process = myProc(self.host, self.port, ['exec ' + appname, 'quit'])
>-    self.process.start()  
>+    
>+    if (self.processExist(appname) != ''):
>+      print "WARNING: process %s appears to be running already\n" % appname
>+    
>+    self.sendCMD(['run ' + appname])
Oops, Bob and I changed this command to be "exec" on the agent.  We can make it run if you prefer that one. We understood "run" to be out and "exec" to be in (that keeps compat with winmo).  I'll make that change on my patch.

>   def getDeviceRoot(self):
>     if (not self.deviceRoot):
>-      if (self.dirExists('/tests')):
>-        self.deviceRoot = '/tests'
>-      else:
>-        self.mkDir('/tests')
>-        self.deviceRoot = '/tests'
>+      data = self.sendCMD(['root'], sleep = 1)
>+      if (data == None):
>+        return '/tests'
>+      self.deviceRoot = self.stripPrompt(data).strip('\n')
>+
>+
>+    if (not self.dirExists(self.deviceRoot)):
>+      self.mkDir(self.deviceRoot)
>+
>     return self.deviceRoot
So, on the call, we understood that the agent would return the raw root location for us to use.  We would need to add the /tests directory to that relative location.  I've made this change in my patch.
>+
>+  #TODO: make this simpler by doing a single directive at a time
>+  # Returns information about the device:
>+  # Directive indicates the information you want to get, your 
choices are:
I'll do a follow up bug for this patch.

>diff --git a/testing/mochitest/runtestsremote.py b/testing/mochitest/runtestsremote.py
> 
>+    def verifyRemoteOptions(self, options, automation):
>+        options.remoteTestRoot = automation._devicemanager.getDeviceRoot()
>+
>+        if (automation._product == "fennec"):
>+          options.xrePath = options.remoteTestRoot + "/" + automation._product + "/xulrunner"
>+        else:
>+          options.xrePath = options.remoteTestRoot + "/" + automation._product
So we set options.xrePath twice in this function once here ^

>+
>+        if (automation._product == "fennec"):
>+            options.xrePath = productRoot + "/xulrunner"
>+        else:
>+            options.xrePath = options.utilityPath
and once here ^.  In my patch, I take out the former.  For this latter setting, I do *not* set it if the user has specified xrePath on the cli.  Otherwise, when you admonish the user to set an xrePath in order to get the webserver started, they're going to get really frustrated, cause they'll keep setting it, and we will keep not listening to them when they do. :)
>     def startWebServer(self, options):
>-        pass
>-        
>+      """ Create the webserver on the host and start it up """
>+      remoteXrePath = options.xrePath
>+      remoteProfilePath = options.profilePath
>+      remoteUtilityPath = options.utilityPath
>+      localAutomation = Automation()
>+
>+      paths = [options.xrePath, localAutomation.DIST_BIN, self._automation._product, os.path.join('..', self._automation._product)]
>+      options.xrePath = self.findPath(paths)
This works great once we stop stomping on the user provided xrePath in verifyRemoteOptions.

>     def installChromeFile(self, filename, options):
>-        path = '/'.join(options.app.split('/')[:-1])
>+        parts = options.app.split('/')
>+        if (parts[0] == options.app):
>+          return "NO_CHROME_ON_DROID"
>+        path = '/'.join(parts[:-1])
DROID!

Patch coming with the outlined changes.

Comment 6

[cmtalbert]

Attachment: Same patch version 2.1

Feel kinda odd carrying forward my own review, so asking Joel to take a quick once over on this.  These are the changes I was suggesting.

Comment 7

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

Comment on attachment 447692 [details] [diff] [review]
Same patch version 2.1

>diff --git a/build/mobile/devicemanager.py b/build/mobile/devicemanager.py
>   def fireProcess(self, appname):
>     if (self.debug >= 2): print "FIRE PROC: '" + appname + "'"
>-    self.process = myProc(self.host, self.port, ['exec ' + appname, 'quit'])
>-    self.process.start()  
>+    
>+    if (self.processExist(appname) != ''):
>+      print "WARNING: process %s appears to be running already\n" % appname
>+    
>+    self.sendCMD(['run ' + appname])

exec or run ?

>@@ -546,11 +492,14 @@
>+      data = self.sendCMD(['testroot'], sleep = 1)
>+      if (data == None):
>+        return '/tests'
>+      self.deviceRoot = self.stripPrompt(data).strip('\n') + '/tests'

hmm, on second thought, we are doing a '/tests', shouldn't this be a '/'

>@@ -592,8 +543,8 @@
>         dir = '/'.join(parts[:-1])
>     elif self.fileExists('/' + filename):
>       dir = '/' + filename
>-    elif self.fileExists('/tests/' + filename):
>-      dir = '/tests/' + filename
>+    elif self.fileExists(self.getDeviceRoot() + '/' + filename):
>+      dir = self.getDeviceRoot() + '/' + filename
>     else:
>       return None

echoing my previous comment, we should do a self.getDeviceRoot(), then try appending tests to it.
testing for '/' isn't very smart.



>diff --git a/testing/mochitest/runtestsremote.py b/testing/mochitest/runtestsremote.py
>+    def verifyRemoteOptions(self, options, automation):
>+        # Set up our options that we depend on based on the above
>+        productRoot = options.remoteTestRoot + "/" + automation._product

hmm, this doesn't work with android so well, then again, we only use this to set app and xrePath to defaults


> class MochiRemote(Mochitest):
>+        self.remoteProfile = options.remoteTestRoot + "/profile"

we need to ensure optinos.remoteTestRoot = deviceManager.getDeviceRoot() + "/tests/"


>     def startWebServer(self, options):
>+      options.utilityPath = self.findPath(paths, xpcshell)

should we make sure options.utilityPath is in the paths array?


>+      options.profilePath = tempfile.mkdtemp()
>+      self.server = MochitestServer(localAutomation, options)
>+      self.server.start()
>+
>+      self.server.ensureReady(self.SERVER_STARTUP_TIMEOUT)
>+      options.xrePath = remoteXrePath
>+      options.utilityPath = remoteUtilityPath
>+      options.profilePath = remoteProfilePath

we lose track of profilePath for cleanup.


> def main():
>+    options = parser.verifyRemoteOptions(options, auto)

we need a clause here:
if options == None:
  sys.exit(1)

Comment 8

[cmtalbert]

checked in as: d4e6d9d727d5

Comment 9

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

Comment on attachment 447692 [details] [diff] [review]
Same patch version 2.1

forgot the r+ part