553700 - allow for xpcshell harness to run in chunks

Status: RESOLVED FIXED

(Testing :: XPCShell Harness, defect)

Description

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

Attachment: add --total-chunks and --this-chunk to xpcshell harness

to complete out the slow device testing project I want to provide a method for xpcshell test harness to run in chunks like mochitest and reftest.

Comment 1

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

Comment on attachment 433639 [details] [diff] [review]
add --total-chunks and --this-chunk to xpcshell harness

ted, could you do an initial review of this.

Comment 2

[(not currently active) Ted Mielczarek]

I know I didn't get to your similar reftest patch yet either, but what do you think about moving these options into automationutils.addCommonOptions, since Mochitest/Reftest/XPCshell all want them?

Comment 3

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

good idea

Comment 4

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

ted: I have bug 554026 to resolve the common cli options.  Can we get a review on this?

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 433639 [details] [diff] [review]
add --total-chunks and --this-chunk to xpcshell harness

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>@@ -1,8 +1,9 @@
>+
> #!/usr/bin/env python

Plz don't add extra blank lines. :)

>@@ -162,17 +164,17 @@ class XPCShellTests(object):
>   def buildTestPath(self):
>     """
>       If we specifiy a testpath, set the self.testPath variable to be the given directory or file.
> 
>       |testPath| will be the optional path only, or |None|.
>       |singleFile| will be the optional test only, or |None|.
>     """
>     self.singleFile = None

Do we still need this line given the initialization you do in the class definition up above?

>-    if self.testPath:
>+    if self.testPath != None:

I prefer "if self.testPath is not None:"

> def main():
>   parser = XPCShellOptions()
>   options, args = parser.parse_args()
> 
>   if len(args) < 2 and options.manifest is None or \
>      (len(args) < 1 and options.manifest is not None):
>      print >>sys.stderr, """Usage: %s <path to xpcshell> <test dirs>
>@@ -470,22 +480,36 @@ def main():
>   xpcsh = XPCShellTests()
>   debuggerInfo = getDebuggerInfo(xpcsh.oldcwd, options.debugger, options.debuggerArgs,
>     options.debuggerInteractive);
> 
>   if options.interactive and not options.testPath:
>     print >>sys.stderr, "Error: You must specify a test filename in interactive mode!"
>     sys.exit(1)
> 
>+  # Build up list of all tests, then create chunks
>+  testList = [options.testPath]
>+  testfiles = []
>+  if (options.totalChunks > 0 and options.thisChunk > 0):
>+    if options.manifest is not None:
>+      testdirs = xpcsh.readManifest(os.path.abspath(options.manifest))
>+
>+    for dir in testdirs:
>+      testfiles.extend(xpcsh.getTestFiles(dir))
>+
>+    testsPerChunk = len(testfiles) / options.totalChunks
>+    start = int(round((options.thisChunk-1) * testsPerChunk))
>+    end = int(round(options.thisChunk + testsPerChunk))
>+    testList = [os.path.basename(f) for f in testfiles[start:start + end]]

Yeah, I don't think this code should be in main(). main() should try to stay very simple, just passing the option args down to runTests, and runTests should do all the real work. Plus, now you're splitting the code that handles test paths etc into two places, which is messy.

Here's what I think you should do:
1) Refactor the existing readManifest code, and buildTestPath. Maybe make it so that buildTestPath gets called from runTests, and it calls readManifest if a manifest was specified? Make it so that we always get the list of test directories up front, and then the full list of test files from them. You could build a dict of the test dirs with each entry being the list of test files for that dir, like:
testDirs = {'testing/xpcshell/example/unit': ['test_get_file.js', 'test_load.js'], 'netwerk/test/unit': ['test_whatever.js', ...], ... }

2) Refactor the code in runTests that loops over testDirs and testFiles to use that pre-built data structure. This should actually be a net simplification

3) Once that's done, you can then go back and change the code that you refactored in step 1 so it knows how to handle --num-chunks / --this-chunk, by slicing the list of tests appropriately and modifying the data structure.

Does that sound sane?

Comment 6

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

Attachment: add --total-chunks and --this-chunk to xpcshell harness

updated with single logic for building a list of tests and then chunking.

Comment 7

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

as a note, I tested this on try server and all tests passed.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 435948 [details] [diff] [review]
add --total-chunks and --this-chunk to xpcshell harness

First off, just wanted to say that this looks a lot better!

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>@@ -33,17 +33,17 @@
> # use your version of this file under the terms of the MPL, indicate your
> # decision by deleting the provisions above and replace them with the notice
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK ***** */
> 
>-import re, sys, os, os.path, logging, shutil, signal
>+import re, sys, os, os.path, logging, shutil, signal, math
> from glob import glob
> from optparse import OptionParser
> from subprocess import Popen, PIPE, STDOUT
> from tempfile import mkdtemp
> 
> from automationutils import *
> 
> class XPCShellTests(object):
>@@ -52,34 +52,70 @@ class XPCShellTests(object):
>   oldcwd = os.getcwd()
> 
>   def __init__(self):
>     """ Init logging """
>     handler = logging.StreamHandler(sys.stdout)
>     self.log.setLevel(logging.INFO)
>     self.log.addHandler(handler)
> 
>-  def readManifest(self, manifest):
>+  def buildTestList(self):

It probably makes sense to keep readManifest as a separate method and call it from buildTestList, just to keep the logic separate.

>+    self.buildTestPath()
>+
>+    self.testlist = {}
>+    if self.manifest == None:
>+      for dir in self.testdirs:
>+        self.testlist[dir] = self.getTestFiles(dir)

I think you should write this like:
tests = self.getTestFiles(dir)
if tests:
  self.testlist[dir] = tests

That way if there aren't any tests in that dir, you won't put anything in the dict (and you can skip the check you added in runTests).

>+    else:
>+      manifestdir = os.path.dirname(self.manifest)
>+      try:
>+        f = open(self.manifest, "r")
>+        for line in f:
>+          path = os.path.join(manifestdir, line.rstrip())
>+          if os.path.isdir(path):
>+            self.testlist[path] = self.getTestFiles(path)

Same thing here. Actually, it might make more sense to just make the manifest parsing bit set self.testdirs, and then move the block above (for dir in self.testdirs) below the optional manifest parsing block, so you just loop over self.testdirs in both cases.

>+    if self.singleFile == None:

if self.singleFile is None:

Didn't get to the rest yet, hopefully will get there later. :)

Comment 9

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

Attachment: add --total-chunks and --this-chunk to xpcshell harness (1.2)

updated patch with comments so far.

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 436250 [details] [diff] [review]
add --total-chunks and --this-chunk to xpcshell harness (1.2)

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>+
>+  def buildTestList(self):
>+    """
>+      Builds a dict of {"testdir" : ["testfile1", "testfile2", ...], "testdir2"...}.

Should mention where the results wind up (in self.testlist).

>+      If manifest is given override testdirs to build initial list of directories and tests.
>+      If testpath is given, use that, otherwise chunk if requested.
>+    """
>+    self.buildTestPath()
>+
>+    self.testlist = {}

Hm. Seems a little odd to name the variable "testlist" when it's actually a dict. :) (Since list is also a Python type!)

>+    if self.manifest is not None:
>+      self.readManifest()
>+
>+    for dir in self.testdirs:
>+      tests = self.getTestFiles(dir)
>+      if tests and len(tests) > 0:

Redundant, |if []| is False, so |if tests:| will only be true if tests is not None and a non-empty list.

>+        self.testlist[dir] = tests
>+
>+    if self.singleFile is None:
>+      self.chunkTests(self.options.totalChunks, self.options.thisChunk)

Won't this wind up calling chunkTests even if totalChunks and thisChunk are None? Also, seems silly to pass member variables into a member function, since chunkTests can just pull them off of self anyway.

>+  def chunkTests(self, totalChunks, thisChunk):
>+    """
>+      Split the list of tests up into [totalChunks] pieces and filter the
>+      self.testlist based on thisChunk, so we only return a subset.
>+    """

This function doesn't return anything. Probably should fix the docstring. :)

>+    if totalChunks <= 1:
>+      return
>+
>+    totalTests = 0
>+    for dir in self.testlist:
>+      totalTests += len(self.testlist[dir])

You could probably do this in a one-liner, like:
totalTests = reduce(lambda x, y: x+len(self.testlist[y]), self.testlist, 0)

but that might be less readable, I dunno.

>+
>+    testsPerChunk = math.ceil(totalTests / float(totalChunks))
>+    start = int(round((thisChunk-1) * testsPerChunk))
>+    end = start + testsPerChunk
>+    currentCount = 0
>+
>+    for dir in self.testlist:
>+      startPosition = 0
>+      dirCount = len(self.testlist[dir])
>+      endPosition = dirCount
>+      if currentCount < start and currentCount + dirCount >= start:
>+        startPosition = int(start - currentCount)        
>+      if currentCount + dirCount > end:
>+        endPosition = int(end - currentCount)
>+      if end - currentCount < 0 or (currentCount + dirCount < start):
>+        endPosition = 0
>+
>+      self.testlist[dir] = self.testlist[dir][startPosition:endPosition]
>+      currentCount += dirCount
>+
>+    # Remove any empty dictionary entries
>+    templist = {}
>+    for dir in self.testlist:
>+      if self.testlist[dir] is not None and len(self.testlist[dir]) > 0:

if self.testlist[dir]:

>+        templist[dir] = self.testlist[dir]
>+    self.testlist = templist

It might be cleaner to just have templist = {} up above the part where you slice the lists, and instead of reassigning back into self.testlist[dir], just set templist[dir] (if startPosition and endPosition are not both zero), and then do this self.testlist = templist, so you don't have to look for empty lists. Instead, templist would just contain the set of dirs you want to run tests on. You could also just "continue" instead of that last "endPosition = 0", I think.

>@@ -162,17 +216,17 @@ class XPCShellTests(object):
>   def buildTestPath(self):
>     """
>       If we specifiy a testpath, set the self.testPath variable to be the given directory or file.
> 
>       |testPath| will be the optional path only, or |None|.
>       |singleFile| will be the optional test only, or |None|.
>     """
>     self.singleFile = None
>-    if self.testPath:
>+    if self.testPath != None:

if self.testPath is not None:

>+    for testdir in self.testlist:

I think one problem here that I might have forgotten to mention is that iterating over the keys in a dict will not produce them in any particular order. I think we currently run the test dirs in alphabetical order, so this should probably be:

for testdir in sorted(self.testlist.keys()):

>+      self.buildXpcsCmd(testdir)
>+      testHeadFiles = self.getHeadFiles(os.path.abspath(testdir))
>+      testTailFiles = self.getTailFiles(os.path.abspath(testdir))

You could save the result of os.path.abspath, but it's probably not a very big deal to call it twice.

>         # The test file will have to be loaded after the head files.
>         cmdT = ['-e', 'const _TEST_FILE = ["%s"];' %
>-                replaceBackSlashes(os.path.join(testdir, test))]
>+                replaceBackSlashes(os.path.abspath(test))]

I think you should just make getTestFiles put absolute paths in the list.

>@@ -450,16 +492,22 @@ class XPCShellOptions(OptionParser):
>                     type="string", dest="manifest", default=None,
>                     help="Manifest of test directories to use")
>     self.add_option("--no-logfiles",
>                     action="store_false", dest="logfiles",
>                     help="don't create log files")
>     self.add_option("--test-path",
>                     type="string", dest="testPath", default=None,
>                     help="single path and/or test filename to test")
>+    self.add_option("--total-chunks",
>+                    type = "int", dest = "totalChunks", default=None,
>+                    help = "how many chunks to split the tests up into")
>+    self.add_option("--this-chunk",
>+                    type = "int", dest = "thisChunk", default=None,
>+                    help = "which chunk to run between 1 and --total-chunks")

I think instead of defaulting to None, you could sanely default these both to 1, right? That way you're running chunk 1/1 by default, which is everything.


>@@ -470,22 +518,22 @@ def main():
>   xpcsh = XPCShellTests()
>   debuggerInfo = getDebuggerInfo(xpcsh.oldcwd, options.debugger, options.debuggerArgs,
>     options.debuggerInteractive);
> 
>   if options.interactive and not options.testPath:
>     print >>sys.stderr, "Error: You must specify a test filename in interactive mode!"
>     sys.exit(1)
> 
>-    
>   if not xpcsh.runTests(args[0],
>                         xrePath=options.xrePath,
>                         symbolsPath=options.symbolsPath,
>                         manifest=options.manifest,
>                         testdirs=args[1:],
>                         testPath=options.testPath,
>                         interactive=options.interactive,
>                         logfiles=options.logfiles,
>+                        options=options,
>                         debuggerInfo=debuggerInfo):

I have a counter-proposal here: instead of passing the 'options' object down wholesale, which I think obscures things, you can refactor this to look like:

xpcsh.runTests(args[0], **options.__dict__)

options.__dict__ contains a dict of option name: value, and the ** passes it as kwargs, so this is equivalent to writing "foo=options.foo" for every option that's set. You will have to add parameters to runTests for the new options you're adding, but it avoids making this call longer. How does that sound?

I think passing the 'options' object around makes it hard to see what values are available without looking at the options declaration, which kind of sucks. It's much nicer to be able to look at the function params and see what you have available.

r- for a little cleanup, but overall it looks good. This winds up being a nice refactoring in the end!

Comment 11

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

Attachment: add --total-chunks and --this-chunk to xpcshell harness (2)

updated with comments.  

This patch does the **options.__dict__ and that works.  The one caveat which was lightly mentioned in the comments is for any new args we add, they have to be explicitly declared in the 'def runTests(...)' argument list.  The note of caution here is we import options from automationutils:addCommonOptions, so if that changes this argument list needs to change as well.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 436479 [details] [diff] [review]
add --total-chunks and --this-chunk to xpcshell harness (2)

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>-  if not xpcsh.runTests(args[0],
>-                        xrePath=options.xrePath,
>-                        symbolsPath=options.symbolsPath,
>-                        manifest=options.manifest,
>-                        testdirs=args[1:],
>-                        testPath=options.testPath,
>-                        interactive=options.interactive,
>-                        logfiles=options.logfiles,
>-                        debuggerInfo=debuggerInfo):
>+  if not xpcsh.runTests(args[0], **options.__dict__):
>+#                        xrePath=options.xrePath,
>+#                        symbolsPath=options.symbolsPath,
>+#                        manifest=options.manifest,
>+#                        testdirs=args[1:],
>+#                        testPath=options.testPath,
>+#                        interactive=options.interactive,
>+#                        logfiles=options.logfiles,
>+#                        options=options,
>+#                        debuggerInfo=debuggerInfo):

I assume you meant to remove these lines and not just comment them out.

r=me with that change.

Comment 13

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

Attachment: add --total-chunks and --this-chunk to xpcshell harness (2.1)

final patch with the commented out code removed.

Comment 14

[cmtalbert]

Landed as changeset 9b06c65c5aef