493748 - update xpchell harness to work with winmo

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

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

Attachment: example of a working runxpc.py script in windows mobile

In order to get xpcshell tests running on windows mobile, we need to work with the crippled version of python that we have available to us.  This means the harness script needs to adjust for:
 - popen and use of return values
 - tmpdir
 - forward slash vs backslash

I have attached an example which works on WinMo (still have to figure out how to get the results) but will show some of the changes that we need to take.  

We can create a new set of scripts for windows mobile or retrofit the existing scripts (which I prefer) to work with the limited python and os support.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 378363 [details]
example of a working runxpc.py script in windows mobile

Change MIME type to view in browser

Comment 2

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

Attachment: runxpcshell.py

adding my working copy of runxpcshell.py.  this depends on the private version of python2.5 that blassey has built to develop subprocess.py support.  

Currently I run this from the visual studio debugger like so:
program name: \python25\python.exe
arguments: \tests\xpcshell\runxpcshell.py


I have included #HACK statements to indicate where I have hacked up the code.  Once we resolve some issues with the Popen handling spaces like it does on windows or linux we can remove a lot of the HACKs around the -e parameters.  I can also remove the hardcoded paths instead of the argv inputs if we all agree that running via a debugger style program is the route to go.

Lastly there is some changes around the Popen where I open/close a file for stdout/stderr.  I need to clean this up and maybe put it in a separate function...it is messy right now.


To run this I always create a dir structure like this:

\tests
  \bin
  \certs
  \mochitest
  \xpcshell
  \reftest
  \fennec

this is just the make package-tests and make package unzip/tar'd in the \tests folder.  Then I copy runxpcshell.py to the \tests\xpcshell directory and run it.

Comment 3

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

Attachment: runxpcshelltests.py

This patch requires bug 503137 for the --environ:CWD=<path> fix as well as a version of python that supports windows ce/mobile and subprocess (http://people.mozilla.org/~blassey/python25.zip).  

Currently this patch will work for windows mobile devices with these caveats:
 - will not exit script upon completion
 - device will hang randomly during a run (I have seen over 90 tests pass at once, but usually somewhere <20).

These issues don't appear to be related to the specific python code in this script.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 387934 [details] [diff] [review]
runxpcshelltests.py

+  #gettempdir fails more often than not on wince

Style nit: please add a space after the # in comment lines.

+  if os.name == 'ce':
+    leakLogFile = "runxpcshelltests_leaks.log"

Can you use os.getcwd() on wince? Or make that "./runxpcshelltests_leaks.log"? It would be nice if it was a little more obvious where the file will wind up.

+      if os.name == 'ce':
+        stdout = None
+        if pStdout != None: 
+          pStdout.close()

This seems a little weird, since you dump stdout to a file which means that the test harness will never be able to indicate a failure. I think it'd be good to have the harness be consistent here, even if you intend to postprocess the logs on another machine later. Might make it easier for developers to run the tests locally, for example.

r- for the last bit there, but overall it looks pretty good.

Comment 5

[Jeff Walden [:Waldo]]

(In reply to comment #4)
> +  if os.name == 'ce':
> +    leakLogFile = "runxpcshelltests_leaks.log"
> 
> Can you use os.getcwd() on wince? Or make that "./runxpcshelltests_leaks.log"?
> It would be nice if it was a little more obvious where the file will wind up.

Last I heard wince (perfect example of a euonym if I ever heard one) doesn't even have the concept of a current working directory.

Comment 6

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

python has a hack for getting around cwd.  But in general there is no support for it :(

Comment 7

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

Attachment: working solution running end ot end

running this script will run end to end with pythonce on the device with this command:

python.exe \tests\xpcshell\runxpcshelltests.py --manifest=\tests\xpcshell\tests\all-test-dirs.list \tests\fennec\xulrunner\xpcshell.exe

This script also has tests removed as per bug 516050

Comment 8

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

Attachment: runxpcshelltests.py - in progress with ip toolset on winmo

this is a WIP script which utilizes ce.py from hg.mozilla.org/qa/maemkit.  This will connect to a winmo/ce device running the test-agent.exe and run xpcshell commands.

Comment 9

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

Attachment: Refactor the xpcshell harness so we can subclass it and run remotely

initial stab at running remotely refactoring.  I need to clean this patch up and ensure it works fine on try server before submitting a real version for review.

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to comment #9)
> Created an attachment (id=426680) [details]
> Refactor the xpcshell harness so we can subclass it and run remotely (WIP)
> 
> initial stab at running remotely refactoring.  I need to clean this patch up
> and ensure it works fine on try server before submitting a real version for
> review.

Do you want my review here then, or should I wait for a new patch?

Comment 11

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

yeah, it was a WIP while I resolved some other issues, then realized it would be resolved in the subclass code.  Please give this a review.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 426680 [details] [diff] [review]
Refactor the xpcshell harness so we can subclass it and run remotely

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>@@ -69,16 +69,188 @@ class XPCShellTests(object):
>         path = os.path.join(manifestdir, dir)
>         if os.path.isdir(path):
>           testdirs.append(path)
>       f.close()
>     except:
>       pass # just eat exceptions
>     return testdirs
> 
>+  def setAbsPath(self):

Please add docstring comments for all these new functions to give a brief description of what they do.

>+    self.testharnessdir = os.path.dirname(os.path.abspath(__file__))
>+
>+    #xpcshell is set by argv[1]
>+    self.xpcshell = os.path.abspath(self.xpcshell)

In comments, please leave a space after the #. Also, I don't think this comment is really necessary here.

>+  def getPipes(self):

I think this function should just return (pStdout, pStderr), instead of setting them as object members.

>+    # xpcsRunArgs: <head.js> function to call to run the test.
>+    # pStdout, pStderr: Parameter values for later |Popen()| call.
>+    if self.interactive:
>+      self.xpcsRunArgs = [
>+      '-e', 'print("To start the test, type |_execute_test();|.");',
>+      '-i']

I think you should put the xpcsRunArgs stuff somewhere else. Having it in a function called "getPipes" doesn't make a lot of sense. It's ok if you have to duplicate a "if self.interactive" block somewhere else to make it work.

>+  def getHeadFiles(self, testdir):
>+    # get the list of head and tail files from the directory

This comment is no longer correct. (Also you could fix it and just make it a docstring.)

>+    testHeadFiles = []
>+    for f in sorted(glob(os.path.join(testdir, "head_*.js"))):
>+      if os.path.isfile(f):
>+        testHeadFiles += [f]
>+
>+    return testHeadFiles          

You could just turn this into a list comprehension:
return [f for f in sorted(glob(os.path.join(testdir, "head_*.js"))) if os.path.isfile(f)]

Also, you have trailing whitespace after the return statement, you might want to sanity check that.

>+
>+  def getTailFiles(self, testdir):
>+    testTailFiles = []
>+    # Tails are executed in the reverse order, to "match" heads order,
>+    # as in "h1-h2-h3 then t3-t2-t1".
>+    for f in reversed(sorted(glob(os.path.join(testdir, "tail_*.js")))):
>+      if os.path.isfile(f):
>+        testTailFiles += [f]
>+
>+    return testTailFiles

You could do the same refactoring here.

>+  def mkTempDir(self):
>+    profileDir = mkdtemp()
>+    self.env["XPCSHELL_TEST_PROFILE_DIR"] = profileDir
>+    return profileDir

This could probably use a more descriptive name, since it not only makes a temp dir, it sets it in the environment. Maybe setupProfileDir or something?


>+  def mkLeakLogFile(self):
>+    filename = "runxpcshelltests_leaks.log"
>+
>+    # Enable leaks (only) detection to its own log file.
>+    leakLogFile = os.path.join(self.profileDir,  filename)
>+    self.env["XPCOM_MEM_LEAK_LOG"] = leakLogFile
>+    return leakLogFile

Similarly here. Maybe setupLeakLogging?


>+  def setSignal(self, proc, sig1, sig2):
>+    signal.signal(sig1, sig2)

You should probably make this higher level, just like:
def ignoreSIGINT():
  ...
def unignoreSIGINT():
  ...

Is there a reason you need this function anyway? It only effects the harness process, which even with your mobile stuff will be running on a desktop machine. (Notice you're not actually using the "proc" argument here...)

>+  def removeDir(self, dirname):
>+    shutil.rmtree(dirname)
>+
>+  def verifyDirPath(self, dirname):
>+    return os.path.abspath(testdir)

You really do need docstring comments for all of these, I have no idea what this function does.

>+        
>+  def getReturnCode(self, procID):
>+    return procID.returncode

I'd name this variable "proc", not "procID", even if it's a PID in your subclass, because here it's clearly a Popen object.

>+  def buildCmdHead(self, headfiles, tailfiles, xpcscmd):
>+    cmdH = ", ".join(['"' + f.replace('\\', '/') + '"'
>+                   for f in headfiles])
>+    cmdT = ", ".join(['"' + f.replace('\\', '/') + '"'
>+                   for f in tailfiles])
>+    cmdH = xpcscmd + \
>+            ['-e', 'const _SERVER_ADDR = "localhost";'] + \
>+            ['-e', 'const _HEAD_FILES = [%s];' % cmdH] + \
>+            ['-e', 'const _TAIL_FILES = [%s];' % cmdT]

While you're here, clean this up to just be a single list. You don't even need line continuations if you do it that way, just
foo = [abc, 123
       xyz, 456]
will work. Also, you can just return the list instead of assigning it into cmdH again.

>   def runTests(self, xpcshell, xrePath=None, symbolsPath=None,

>     # Process each test directory individually.
>     for testdir in testdirs:
>+      print "testdir: " + testdir

Ditch the extra print here.

>+      # create a temp dir that the JS harness can stick a profile in
>+      self.profileDir = None
>+      self.profileDir = self.mkTempDir()

Why the double-assignment? Also, this is in the wrong place now. You moved it out of the loop where it belongs. We create a temp dir for every test intentionally, so there's no cross-test contamination. (It looks like we screwed up along the way somewhere, because we're supposed to delete the dir after every test, maybe you can fix that while you're here!)


>+      testfiles = self.getTestFiles(testdir)
>+      if (testfiles == None):
>+        continue

You don't need the parens in this if statement, also, prefer "if testfiles is None:"

> 
script
>           # - don't move this line above Popen, or child will inherit the SIG_IGN
>-          signal.signal(signal.SIGINT, signal.SIG_IGN)
>+          self.setSignal(proc, signal.SIGINT, signal.SIG_IGN)
>           # |stderr == None| as |pStderr| was either |None| or redirected to |stdout|.
>-          stdout, stderr = proc.communicate()
>-          signal.signal(signal.SIGINT, signal.SIG_DFL)
>+          stdout, stderr = self.communicate(proc)
>+          self.setSignal(proc, signal.SIGINT, signal.SIG_DFL)

I don't think the signal stuff is any clearer to read, as I said above.

>-          if proc.returncode != 0 or (stdout and re.search("^TEST-UNEXPECTED-FAIL", stdout, re.MULTILINE)):
>+          if ((self.getReturnCode(proc) != 0) or (stdout and re.search("^TEST-UNEXPECTED-FAIL", stdout, re.MULTILINE))):

Ditch the extra parens.

>+class XpcShellOptions(OptionParser):

I think I'd like this named XPCShellOptions.

>+def main():
>+
>+  parser = XpcShellOptions()

Get rid of the blank line here.

Needs some cleanup, but overall not bad.

Comment 13

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

Attachment: Refactor the xpcshell harness so we can subclass it and run remotely (2)

updated with all the great feedback in comment #12.  This does clean up a lot of stuff and adds a lot of great comments:)  Currently I have this patch on the try servers and waiting for results.

Comment 14

[(not currently active) Ted Mielczarek]

Comment on attachment 430726 [details] [diff] [review]
Refactor the xpcshell harness so we can subclass it and run remotely (2)

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>+  def setAbsPath(self):
>+    """
>+      set the absolute path for xpcshell, httpdjspath and xrepath.
>+      these 3 variables depend on input from the command line and we need to allow for absolute paths
>+      This function is overloaded for a remote solution as os.path* won't work remotely
>+    """

This is really nitpicky, but try to keep your comments (and docstrings) so they read like sentences, including capitalizing the first word. It helps readability a lot.

>+    self.testharnessdir = os.path.dirname(os.path.abspath(__file__))
>+    self.xpcshell = os.path.abspath(self.xpcshell)
>+
>+    # we assume that httpd.js lives in components/ relative to xpcshell
>+    self.httpdJSPath = os.path.join(os.path.dirname(self.xpcshell) + 'components', 'httpd.js')

Why are you putting a + in there? You can pass multiple args to os.path.join, and it will join them appropriately.

>+    self.httpdJSPath = self.httpdJSPath.replace("\\", "/");
>+
>+    if self.xrePath is None:
>+      self.xrePath = os.path.dirname(self.xpcshell)
>+    else:
>+      self.xrePath = os.path.abspath(self.xrePath)
>+            
>+  def buildEnvironment(self):
>+    """
>+      create a dictionary of self.env to include all the appropriate env variables and values
>+      on a remote system, we overload this to set different values and are missing things like os.environ and PATH
>+    """

Should mention that the dictionary is also the return value.

>+  def buildXpcsRunArgs(self):
>+    """
>+      xpcsRunArgs: <head.js> function to call to run the test.
>+    """

Can you cleanup some of these comments while you're here and make them a little more readable? Serge's comment style is a little terse. (But I think English isn't his first language, and his English is certainly better than my French.)

>+  def getPipes(self):
>+    """
>+      pStdout, pStderr: Parameter values for later |Popen()| call.
>+    """

For example, this doesn't tell me much about what this function does. Also, in general, if the function has a return value you should mention what it returns in the comment.

>+  def buildCmdHead(self, headfiles, tailfiles, xpcscmd):
>+    """
>+      build the command line arguments for the head and tail files
>+      along with the address of the webserver which some tests require
>+
>+      On a remote system, this is overloaded to resolve \" issues over a secondary command line
>+    """

To resolve what issues? Quoting? If so, just say 'quoting'. :)

>+    cmdH = ", ".join(['"' + f.replace('\\', '/') + '"'
>+                   for f in headfiles])
>+    cmdT = ", ".join(['"' + f.replace('\\', '/') + '"'
>+                   for f in tailfiles])

I wonder if it wouldn't make sense to add a function that does this slash substitution, so we can call it by name, like replaceslashes(f). That would probably be a lot clearer.

r=me with those nits fixed.

Comment 15

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

Attachment: Refactor the xpcshell harness so we can subclass it and run remotely (2.1)

carryover r+, fixed nits (mostly comments).

Comment 16

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

Attachment: Refactor the xpcshell harness so we can subclass it and run remotely (2.2)

found one bug (was in a subsequent patch) where I forgot to add "replaceBackSlashes" to the list of __all__ functions.

Comment 17

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

Attachment: Refactor the xpcshell harness so we can subclass it and run remotely (2.3) [Checkin: Comment 18]

fixed a couple bugs found while testing this.

Comment 18

[cmtalbert]

Landed as 60d6b5d65a94

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (AAv1) Unregress --test-path, Some rewrites, Whitespace cleanup

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 432424 [details] [diff] [review]
(AAv1) Unregress --test-path, Some rewrites, Whitespace cleanup

Serge--if you notice that a patch causes a regression, I'd appreciate it if you could either reopen the bug, or file a bug blocking the bug, instead of just adding a new patch onto the existing bug with no comments about why. Second, mixing a bunch of cleanup and whitespace changes into a regression fix patch makes it a lot harder to review. Let's move this to bug 552847, which already has a candidate patch.

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (AAv2) Some rewrites, Whitespace cleanup

AAv1, unbitrotted.


(In reply to comment #20)
> just adding a new patch onto the existing bug with no comments about why.

I assumed "Unregress --test-path" would be explicit enough :-|

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 433378 [details] [diff] [review]
(AAv2) Some rewrites, Whitespace cleanup

I'm not reviewing this on this bug.