convert testharness python code to classes for future integration with mobile framework

VERIFIED FIXED in mozilla1.9.3a1

Status

VERIFIED FIXED
9 years ago
8 months ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 19 obsolete attachments)

97.08 KB, patch
Details | Diff | Splinter Review
21.91 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
Created attachment 413986 [details] [diff] [review]
mochitest, reftest, xpcshell test harness with class definition

adjust the python test harness code to be in a class.  All modifications will be in the form of moving test functionality into a runtest() type of function along with adding spaces to just about every line of the file.  

Once this is checked in we will start pulling the harness into smaller methods allowing for overriding specific actions to work on a remote device.
(Assignee)

Comment 1

9 years ago
Comment on attachment 413986 [details] [diff] [review]
mochitest, reftest, xpcshell test harness with class definition

ted, please let me know if this is a good format to get the patch to you.  I did only a few logic changes to make this work with a class structure.
Attachment #413986 - Attachment is patch: true
Attachment #413986 - Attachment mime type: application/octet-stream → text/plain
Attachment #413986 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

9 years ago
Assignee: nobody → jmaher
(Assignee)

Comment 2

9 years ago
Created attachment 413990 [details] [diff] [review]
mochitest, reftest, xpcshell test harness with class definition

forgot one test case in this logic, this passes tests on m-c (firefox and fennec) for mochitest, chrome, browser-chrome, reftest, crashtest, jsreftest, xpcshell.
Attachment #413986 - Attachment is obsolete: true
Attachment #413990 - Flags: review?(ted.mielczarek)
Attachment #413986 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 3

9 years ago
Created attachment 416003 [details] [diff] [review]
xpcshell, mochitest, reftest harnesses- convert to classes

This is slightly updated from the last patch to fix two minor bugs as found while porting automation.py to a class and overloading select methods in mochitest.
Attachment #413990 - Attachment is obsolete: true
Attachment #416003 - Flags: review?(ted.mielczarek)
Attachment #413990 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 4

9 years ago
Created attachment 416007 [details] [diff] [review]
automation.py converted to a class

converts automation.py to a class structure.  retrofitted code for mochitest and reftest to use new class interface as well as leaktest and pgo bits.  

This patch is intended to be applied to mozilla-central after the previous patch.
Attachment #416007 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 5

9 years ago
Created attachment 416283 [details] [diff] [review]
3rd patch in series...refactoring functionality to smaller pieces

additional patch which breaks out a lot of functionality in automation.py and runtests.py into smaller pieces.  We use these pieces for creating a subclass and doing all the remote stuff.
(Assignee)

Comment 6

9 years ago
Created attachment 416287 [details]
working subclass script for running remote mochitest

additional file which is useful for running remote.  This is an example of how all these patches will be used.
Comment on attachment 416003 [details] [diff] [review]
xpcshell, mochitest, reftest harnesses- convert to classes

Global style nit: you're switching things to 4-space indentation, this file uses 2-space indentation. Please keep it at 2-space.

>diff --git a/layout/tools/reftest/runreftest.py b/layout/tools/reftest/runreftest.py
>--- a/layout/tools/reftest/runreftest.py
>+++ b/layout/tools/reftest/runreftest.py
>+class RefTest(object):
>+    oldcwd = ''
>+    def __init__(self):
>+        self.oldcwd = os.getcwd()
>+        os.chdir(SCRIPT_DIRECTORY)

I don't think the os.chdir is actually necessary here. Try removing that, or at least move the os.getcwd() up to the member definition.

>+    def getFullPath(self, path):
>+        "Get an absolute path relative to oldcwd."

I'd change this to say "relative to self.oldcwd".

>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>--- a/testing/mochitest/runtests.py.in
>+++ b/testing/mochitest/runtests.py.in

> #################
> # MAIN FUNCTION #
> #################

Just remove this comment.

>+class MochiTest(object):

I don't think we capitalize the T anywhere else, so just make this "class Mochitest".

>+    # Path to the test script on the server
>+    TEST_SERVER_HOST = "localhost:8888"
>+    TEST_PATH = "/tests/"
>+    CHROME_PATH = "/redirect.html";
>+    A11Y_PATH = "/redirect-a11y.html"
>+    TESTS_URL = "http://" + TEST_SERVER_HOST + TEST_PATH
>+    CHROMETESTS_URL = "http://" + TEST_SERVER_HOST + CHROME_PATH
>+    A11YTESTS_URL = "http://" + TEST_SERVER_HOST + A11Y_PATH
>+    SERVER_SHUTDOWN_URL = "http://" + TEST_SERVER_HOST + "/server/shutdown"
>+    # main browser chrome URL, same as browser.chromeURL pref
>+    #ifdef MOZ_SUITE
>+    BROWSER_CHROME_URL = "chrome://navigator/content/navigator.xul"
>+    #else
>+    BROWSER_CHROME_URL = "chrome://browser/content/browser.xul"
>+    #endif

The preprocessor blocks inside the class definition here bother me. I actually don't think we need them, since they're only used to write out the overlay bits. We should be able to just write both URLs to the chrome manifest, and the app will only use the one it cares about anyway. Change the lines down below that read:
    overlayLine = "overlay " + BROWSER_CHROME_URL + " " \
                  "chrome://mochikit/content/browser-test-overlay.xul\n"
    manifestFile.write(overlayLine)

to read:
  manifestFile.write("""overlay chrome://browser/content/browser.xul chrome://mochikit/content/browser-test-overlay.xul
overlay chrome://navigator/content/navigator.xul chrome://mochikit/content/browser-test-overlay.xul
""")

And then just remove BROWSER_CHROME_URL completely.


>+    def getFullPath(self, path):
>+        "Get an absolute path relative to oldcwd."

"relative to self.oldcwd"

>+        for v in options.environment:
>+            ix = v.find("=")
>+            if ix <= 0:
>+                print "Error: syntax error in --setenv=" + v
>+                sys.exit(1)

Probably want to just "return 1" instead of sys.exit, since main() takes care of that for you.

>+        # hanging due to non-halting threads is no fun; assume we hit the errors we
>+        # were going to hit already and exit.
>+        sys.exit(status)

Same here, just return status.

>+    def addChromeToProfile(self, options):

>+        # write userChrome.css
>+        chromeFile = open(os.path.join(self.PROFILE_DIRECTORY, "userChrome.css"), "a")
>+        chromeFile.write("".join(chrome))
>+        chromeFile.close()

While you're here, why don't you simplify this code? There's no reason it needs to use an array and join it back to a string, since it only wants to write out that one string.

> #########
> # DO IT #
> #########
> 

Just remove this comment.

>diff --git a/testing/xpcshell/runxpcshelltests.py b/testing/xpcshell/runxpcshelltests.py
>--- a/testing/xpcshell/runxpcshelltests.py
>+++ b/testing/xpcshell/runxpcshelltests.py
>+class XpcShell(object):

I think I'd name this "XPCShellTests" to make it a little clearer.


>+    log = None
>+    oldcwd = ''

You should be able to initialize both of these inline instead of in the constructor.

r- only for the 4-space indentation, otherwise it looks good aside from those other few nits.
Attachment #416003 - Flags: review?(ted.mielczarek) → review-
Did you want review on those last two patches as well?
(Assignee)

Comment 9

9 years ago
Created attachment 416420 [details] [diff] [review]
xpcshell, mochitest, reftest harnesses- convert to classes (2)

updated with 2 spaces instead of 4, as well as the few minor cleanups mentioned in previous review.  I will upload the automation.py patch with 2 spaces later today.
Attachment #416420 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 10

9 years ago
Created attachment 416571 [details] [diff] [review]
automation.py converted to a class (2)

updated automation.py to be a class and this patch uses 2 spaces instead of 4.
Attachment #416007 - Attachment is obsolete: true
Attachment #416571 - Flags: review?(ted.mielczarek)
Attachment #416007 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 11

9 years ago
Created attachment 416573 [details] [diff] [review]
mochitest refactor for remote testing (2)

3rd patch in series, allows for smaller functions for easier subclass overrides.
Attachment #416283 - Attachment is obsolete: true
ted: any ETA on this review? its blocking a Q4 security goal RelEng is working on with Jesse.
Blocks: 505520
Got tangled up in some Breakpad work. Hoping to get my review queue cleaned out ASAP.
(Assignee)

Comment 14

9 years ago
I will have unbitrot patches uploaded tonight
(Assignee)

Comment 15

9 years ago
Created attachment 418324 [details] [diff] [review]
xpcshell, mochitest, reftest harnesses- convert to classes (3)

updated for bitrot, tested on linux from objdir and package-tests
Attachment #416003 - Attachment is obsolete: true
(Assignee)

Comment 16

9 years ago
Created attachment 418325 [details] [diff] [review]
automation.py converted to a class (3)

updated for bitrot, tested on linux in objdir and package-tests
Attachment #416571 - Attachment is obsolete: true
Attachment #418325 - Flags: review?(ted.mielczarek)
Attachment #416571 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

9 years ago
Attachment #418324 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

9 years ago
Attachment #416420 - Attachment is obsolete: true
Attachment #416420 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 17

9 years ago
Created attachment 418327 [details] [diff] [review]
mochitest refactor for remote testing (3)

updated for bitrot, tested on linux in objdir and package-tests
Attachment #416573 - Attachment is obsolete: true
Attachment #418327 - Flags: review?(ted.mielczarek)
Comment on attachment 418324 [details] [diff] [review]
xpcshell, mochitest, reftest harnesses- convert to classes (3)

>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):

Can you make these line up with the args[0] argument?

r=me with that nit fixed.
Attachment #418324 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 19

9 years ago
Created attachment 418719 [details] [diff] [review]
xpcshell, mochitest, reftest harnesses- convert to classes (final)

final version with Ted's nit fixed.
Attachment #418324 - Attachment is obsolete: true
Comment on attachment 418325 [details] [diff] [review]
automation.py converted to a class (3)

>diff --git a/build/automation.py.in b/build/automation.py.in
>--- a/build/automation.py.in
>+++ b/build/automation.py.in

>-#################
>-# SUBPROCESSING #
>-#################
>-
>-class Process(subprocess.Popen):

Do you need to move the Process class inside the Automation class? I don't think it's actually used anywhere else, but it just feels a bit weird to do that.

>+class Automation(object):
> 
>-def readLocations(locationsPath = "server-locations.txt"):
>-  """
>-  Reads the locations at which the Mochitest HTTP server is available from
>-  server-locations.txt.
>-  """
>+#expand   DIST_BIN = __XPC_BIN_PATH__
>+#expand   IS_WIN32 = len("__WIN32__") != 0
>+#expand   IS_MAC = __IS_MAC__ != 0
>+#expand   IS_LINUX = __IS_LINUX__ != 0
>+#ifdef IS_CYGWIN
>+#expand   IS_CYGWIN = __IS_CYGWIN__ == 1
>+#else
>+  IS_CYGWIN = False
>+#endif
>+#expand   IS_CAMINO = __IS_CAMINO__ != 0
>+#expand   BIN_SUFFIX = __BIN_SUFFIX__
>+#expand   PERL = __PERL__

Ick, having these expansions inside the class definition is really gross. Can you keep them at the top of the file, and just do like:
class Automation(object):
  DIST_BIN = __XPC_BIN_PATH__
...

You can set __all__ = ["Automation"] up top if you want to ensure that we only export the class for consumers and not the constants directly.

>+  def __init__(self):
>+    """
>+    Runs the browser from a script, and provides useful utilities
>+    for setting up the browser environment.
>+    """

Presumably you want this comment below the "class" line, and not beneath __init__? (I'm not exactly sure where the class comment belongs in Python.)

>+    self.SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])))

This feels kind of weird in a class. Can you a) move this assignment up to the class body, and b) try using __file__ instead of sys.argv[0]?

>+    # timeout, in seconds
>+    self.DEFAULT_TIMEOUT = 60.0
>+
>+    # We use the logging system here primarily because it'll handle multiple
>+    # threads, which is needed to process the output of the server and application
>+    # processes simultaneously.
>+    self.log = logging.getLogger()

These two assignments can also get moved up to the class level.

>+  @property
>+  def __all__(self):

I'm not really wild about this, but I guess we can refactor it later.

>+  def readLocations(self, locationsPath = "server-locations.txt"):

Does this function need to live in the class? It's pretty unrelated to the rest of the functionality, I think. Not sure how your encapsulation needs to work, though.

>+  def addExtraCommonOptions(self, parser):
>+    "Adds command-line options which are common to mochitest and reftest."

This could probably get renamed to just "addCommonOptions", since inside the class it shouldn't conflict with the one from automationutils.py.

>+  def runApp(self, testURL, env, app, profileDir, extraArgs,
>+             runSSLTunnel = False, utilityPath = None,
>+             xrePath = None, certPath = None,
>+             debuggerInfo = None, symbolsPath = None,
>+             timeout = None):

I think some of these parameters could be moved up to the constructor, but you're free to do that in a followup (as long as you file a followup bug on it).

>diff --git a/layout/tools/reftest/runreftest.py b/layout/tools/reftest/runreftest.py
>--- a/layout/tools/reftest/runreftest.py
>+++ b/layout/tools/reftest/runreftest.py
>-  def __init__(self):
>+  def __init__(self, automation):
>+    self._automation = automation

Do you expect that anything outside the class itself will have to access the automation member? If so, I'd just name this |automation|. If not, then this is fine.

>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>--- a/testing/mochitest/runtests.py.in
>+++ b/testing/mochitest/runtests.py.in
>@@ -52,7 +52,7 @@ from urllib import quote_plus as encodeU
>+    # we want to pass down everything from self._automation.__all__
>+    addCommonOptions(self, defaults=dict(zip(self._automation.__all__, 
>+             [getattr(self._automation, x) for x in self._automation.__all__])))
>+    self._automation.addExtraCommonOptions(self)

There's probably a cleaner way to do this now that it's part of a class, but that can go in a followup.

>@@ -369,7 +374,7 @@ class Mochitest(object):
>     else:
>       if options.autorun:
>         urlOpts.append("autorun=1")
>-      if options.timeout
>+      if options.timeout:

Huh, is that some random local change you had or something?

r=me with those things fixed.
Attachment #418325 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 418327 [details] [diff] [review]
mochitest refactor for remote testing (3)

>diff --git a/build/automation.py.in b/build/automation.py.in
>--- a/build/automation.py.in
>+++ b/build/automation.py.in
>@@ -556,6 +556,96 @@ user_pref("camino.use_system_proxy_setti
>     self.log.info("Can't trigger Breakpad, just killing process")
>     proc.kill()
> 
>+  def waitForOutput(self, outputPipe, utilityPath, timeout, proc):

I'm not wild about this method name, it implies to me that it will return when the process outputs something. I don't have a better suggestion at the moment, but perhaps you can think of something? Also, can you add a docstring comment here explaining what the method actually does? Also, I think "proc" should be the first argument. Finally, you shouldn't need a separate "outputPipe" argument, you can just check proc.stdout.

>+  def buildCLI(self, app, debuggerInfo, profileDir, testURL, extraArgs):
>+    # now run with the profile we created

Kill the out-of-place comment, but add a docstring comment. Also, I think I'd call this "buildCommandline", without the abbreviation.

Looking at both of these functions, it feels like you could save a few of these variables as member variables on the class, then just use them instead of passing them around to each member function.

>+  def checkForZombies(self, processLog):
>+    # Do a final check for zombie child processes.

Move this comment down to runApp where you're calling this, and add a docstring comment to this method.

>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>--- a/testing/mochitest/runtests.py.in
>+++ b/testing/mochitest/runtests.py.in
>-    self.PROFILE_DIRECTORY = os.path.abspath("./mochitesttestingprofile")
>+    self.localProfile = os.path.abspath("./mochitesttestingprofile")
>+    self.PROFILE_DIRECTORY = self.localProfile

Didn't you have another patch that changed the profile directory?

>+    self.runSSLTunnel = True

Are you using this just for your mobile tests because you're not using ssltunnel yet?

>+    self.TESTS_URL = "http://" + self.TEST_SERVER_HOST + self.TEST_PATH
>+    self.CHROMETESTS_URL = "http://" + self.TEST_SERVER_HOST + self.CHROME_PATH
>+    self.A11YTESTS_URL = "http://" + self.TEST_SERVER_HOST + self.A11Y_PATH
>+    self.SERVER_SHUTDOWN_URL = "http://" + self.TEST_SERVER_HOST + "/server/shutdown"

Since you've moved most of this into buildTestPath, I don't think these TESTS_URL variables need to be member variables. Just make them locals (or use them directly) in that function.

>+  def buildTestPath(self, options):

Add a docstring comment, please.

>+    testURL = self.TESTS_URL + options.testPath
>+    self.urlOpts = []

You should probably set self.urlOpts = [] in the constructor, not here.

 
>+  def startWebServer(self, options):

Docstring comment!

>+  def stopWebServer(self):

And here...

>+  def getLogFilePath(self, logFile):
>+    return self.getFullPath(logFile)

This seems a little silly, but I guess you need it for your mobile subclass? Anyway, add a docstring comment.

>+
>+  def buildProfile(self, options):

And here...


>+  def buildBrowserEnv(self, options):
>     # browser environment
>     browserEnv = self._automation.environment(xrePath = options.xrePath)
> 
>@@ -327,77 +366,13 @@ class Mochitest(object):
>         return 1

You don't want to return 1 in case of error here. Maybe return None?

>+  def registerExtension(self, browserEnv, options):
>     # run once with -silent to let the extension manager do its thing
>     # and then exit the app

Just convert this comment block to a docstring. Also, I don't like this method name. Perhaps "runExtensionRegistration"?

>+  def buildURLOptions(self, options):

Docstring comment! I don't know if just converting the comment block is sufficient, it doesn't really describe what this function does.

>+      if len(self.urlOpts) > 0:
>+        return "?" + "&".join(self.urlOpts)
>+    return ""

I think I'd rather that this function just sets self.urlOpts, and lets the caller build a string out of it. This maybe-returning-an-empty-string thing just doesn't feel right.

>+
>+  def cleanup(self, manifest):
>+    # delete the profile and manifest
>+    os.remove(manifest)

a) docstring comment (you tired of hearing this yet?)
b) this comment seems to be a lie

>+  def runTests(self, options):

Docstring comment.

>   def makeTestConfig(self, options):
>@@ -501,17 +524,23 @@ toolbar#nav-bar {
>       chrometestDir = "file:///" + chrometestDir.replace("\\", "/")
> 
> 
>-    (path, leaf) = os.path.split(options.app)
>-    manifest = os.path.join(path, "chrome", "mochikit.manifest")
>-    manifestFile = open(manifest, "w")
>+    tempfile = "mochikit.manifest"
>+    manifestFile = open(tempfile, "w")

If you're going to make this a tempfile, please put it in a temp dir.

>+  def installChromeFile(self, filename, options):
>+    (p, file) = os.path.split(filename)
>+    (path, leaf) = os.path.split(options.app)
>+    manifest = os.path.join(path, "chrome", file)
>+    shutil.copy(filename, manifest)
>     return manifest

I'd really like to make Mochitest stop jamming stuff into the app directory. Can you file a followup on making this suck less? I think we can probably stick it in the profile dir somewhere since we do the extension manager restart these days.

I know that's a lot of comments, but none of them seem to be particularly large, so I'm going to r+ this.
Attachment #418327 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

9 years ago
Blocks: 536388
(Assignee)

Comment 22

9 years ago
>diff --git a/build/automation.py.in b/build/automation.py.in
>--- a/build/automation.py.in
>+++ b/build/automation.py.in

>-#################
>-# SUBPROCESSING #
>-#################
>-
>-class Process(subprocess.Popen):

Do you need to move the Process class inside the Automation class? I don't
think it's actually used anywhere else, but it just feels a bit weird to do
that.

actually I found I needed to do this.  In my attached sample script, I subclass automation and rewrite the Process class.  This is where 90% of the work is done and by putting it inside of Automation, I can reference it the same way no matter who is calling it.



>+class Automation(object):
> 
>-def readLocations(locationsPath = "server-locations.txt"):
>-  """
>-  Reads the locations at which the Mochitest HTTP server is available from
>-  server-locations.txt.
>-  """
>+#expand   DIST_BIN = __XPC_BIN_PATH__
>+#expand   IS_WIN32 = len("__WIN32__") != 0
>+#expand   IS_MAC = __IS_MAC__ != 0
>+#expand   IS_LINUX = __IS_LINUX__ != 0
>+#ifdef IS_CYGWIN
>+#expand   IS_CYGWIN = __IS_CYGWIN__ == 1
>+#else
>+  IS_CYGWIN = False
>+#endif
>+#expand   IS_CAMINO = __IS_CAMINO__ != 0
>+#expand   BIN_SUFFIX = __BIN_SUFFIX__
>+#expand   PERL = __PERL__

Ick, having these expansions inside the class definition is really gross. Can
you keep them at the top of the file, and just do like:
class Automation(object):
  DIST_BIN = __XPC_BIN_PATH__
...


I did the expand at the top of the file and pulled these into the class definition similar to Ted's suggestion.


You can set __all__ = ["Automation"] up top if you want to ensure that we only
export the class for consumers and not the constants directly.


>+    self.SCRIPT_DIR = os.path.abspath(os.path.realpath(os.path.dirname(sys.argv[0])))

This feels kind of weird in a class. Can you a) move this assignment up to the
class body, and b) try using __file__ instead of sys.argv[0]?


using __file__ works the same since we do all the os.path operations on the value.



>+  @property
>+  def __all__(self):

I'm not really wild about this, but I guess we can refactor it later.

bug 536388



>+  def readLocations(self, locationsPath = "server-locations.txt"):

Does this function need to live in the class? It's pretty unrelated to the rest
of the functionality, I think. Not sure how your encapsulation needs to work,
though.


This does not need to live in the class.  It is referenced in genpgocerts.py as well as the ssl setup.  Should this live in automationutils.py?  My goal was to have 'from automation import Automation' and have all functionality in the Automation object.



>+  def runApp(self, testURL, env, app, profileDir, extraArgs,
>+             runSSLTunnel = False, utilityPath = None,
>+             xrePath = None, certPath = None,
>+             debuggerInfo = None, symbolsPath = None,
>+             timeout = None):

I think some of these parameters could be moved up to the constructor, but
you're free to do that in a followup (as long as you file a followup bug on
it).

bug 536388


>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>--- a/testing/mochitest/runtests.py.in
>+++ b/testing/mochitest/runtests.py.in
>@@ -52,7 +52,7 @@ from urllib import quote_plus as encodeU
>+    # we want to pass down everything from self._automation.__all__
>+    addCommonOptions(self, defaults=dict(zip(self._automation.__all__, 
>+             [getattr(self._automation, x) for x in self._automation.__all__])))
>+    self._automation.addExtraCommonOptions(self)

There's probably a cleaner way to do this now that it's part of a class, but
that can go in a followup.


bug 536388


>@@ -369,7 +374,7 @@ class Mochitest(object):
>     else:
>       if options.autorun:
>         urlOpts.append("autorun=1")
>-      if options.timeout
>+      if options.timeout:

Huh, is that some random local change you had or something?


Yea, bad me...fixed that.
(Assignee)

Updated

9 years ago
Blocks: 536407

Comment 23

9 years ago
Created attachment 419022 [details] [diff] [review]
Full patch, merged to tip 12-23-09 10:06am pst

Just merging patch to tip, no changes.

Comment 24

9 years ago
Created attachment 420354 [details] [diff] [review]
Final Patch submitted to try and m-c

Final patch submitted, carrying review forward.  This was submitted as changeset: 85cd0f297421
Attachment #416287 - Attachment is obsolete: true
Attachment #418325 - Attachment is obsolete: true
Attachment #418327 - Attachment is obsolete: true
Attachment #418719 - Attachment is obsolete: true
Attachment #419022 - Attachment is obsolete: true
Attachment #420354 - Flags: review+

Updated

9 years ago
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 537739
Flags: in-testsuite-
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk

Updated

9 years ago
No longer blocks: 505520

Comment 25

9 years ago
(In reply to comment #24)
> Created an attachment (id=420354) [details]
> Final Patch submitted to try and m-c
> 
> Final patch submitted, carrying review forward.  This was submitted as
> changeset: 85cd0f297421
And backed out in changeset 2e7fe7682fe3 because it caused unredeemable errors with the bloat tests and linux debug mochitests.

--> REOPENED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Were there additional failures on top of what I pointed out yesterday? (the indententation change in runxpcshelltests.py, and the PERL problem in automation.py)
(Assignee)

Comment 27

9 years ago
the follow up patch used this.PERL instead of self.PERL (my bad for not catching that).

Comment 28

9 years ago
Created attachment 420568 [details] [diff] [review]
Final patch, passes full green on try, passes local testing

Joel's final patch with standard8's changes to it and passes on try (again).  Ready to try this again.

(Carrying reviews forward)
Attachment #420354 - Attachment is obsolete: true
Attachment #420568 - Flags: review+

Comment 29

9 years ago
Trying again with new patch changeset: e67e79969232

Leaving status alone until we see green.
Green we didn't see, it's back out again.

The two classes of things that I think were both it were

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262901605.1262905406.23679.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262897654.1262901496.12744.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1262897654.1262901496.12744.gz

two Linux and one Mac Md4, all hanging in-or-after /tests/layout/style/test/test_transitions_per_property.html, and, every single Windows mochitest-ipcplugins run timing out.
Full log shows that the test_transitions_per_property.html "hangs" are actually crashes, but not all at the same place.  Not sure if that changes anything here.
I might have been misreading it, but I thought that the full log showed that the "crashes" were actually hangs, where the test harness says "oh, you hung, did you? well I'll crash you to show where you hung."
Oh, interesting! That would explain why the crashes are in different places.  I wasn't aware that the test harness could do that -- apologies for confusing the issue.
I guess we could make the harness output a little clearer in the case where a hang leads to it being forcibly killed.
(Assignee)

Comment 35

9 years ago
Created attachment 420789 [details] [diff] [review]
single patch that has included code I missed in botrot merging

lets give this another try.  this should pass mochitest-ipcplugins
Attachment #420568 - Attachment is obsolete: true
Pushed Joel's latest patch as http://hg.mozilla.org/mozilla-central/rev/ca10f6b27b95
Backed it out again:  http://hg.mozilla.org/mozilla-central/rev/961400b8b0b6.  There were two issues:

1 - a failure after running test /tests/layout/style/test/test_transitions_per_property.html on linux Md4, same as comment #30 above.
2 - a timeout after running test_crashing.html in mochitest-ipcplugins on linux and windows
Problem #2 in comment #37 is caused by a typo in automation.py.in:  should be XRE_NO_WINDOWS_CRASH_DIALOG, currently is XRE_NO_WINDOW_CRASH_DIALOG
(Assignee)

Comment 39

9 years ago
I have not had much luck figuring out the failure after: /tests/layout/style/test/test_transitions_per_property.html.

It appears to be a SIGSEGV after the end of that test.  I am not sure if it is in the cleanup of that test, or the start of the next test (tests_units_angle.html.)  Here is the output of the minidump: http://mozilla.pastebin.com/f356556f2.  


This failure only appears on Linux Debug.  Since this is a SIGSEGV in the actual product, it would be logical to assume that the cli or environment (including stdin/out/err and vars) would be the problem here.  The patch I have been using is changes to the python code only.  

I guess there could be a remote chance that by moving the python code to classes we somehow pollute the memory space of the launched process.  I am not sure how wild of an idea that is, but the test_transitions_per_property.html is 26K+ tests which is the largest set of results for a single file in mochitest.

Another interesting fact is that this failure seems to only show up after my checkin is in, but doesn't always fail on MD(4) linux boxes...just about 4/5 of them (enough to point to probable cause of my patch.)
So, one thing to note there is that the app didn't crash, it hung, note the:
NEXT ERROR TEST-UNEXPECTED-FAIL | automation.py | application timed out after 60 seconds with no output (see my comment 34).

The test harness is then forcibly killing the browser.
That's a very long-running test, it's possible your patch changed the buffering of the output of the subprocess somehow, causing us to think it was hung. It could also be a subtle bug introduced in readWithTimeout.
Actually, looking at that, I notice it says "timed out after 60 seconds with no output". 60 seconds is the DEFAULT_TIMEOUT:
http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#82

but runtests.py passes a longer timeout value:
http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py.in#469

So I think this patch broke that setting of a longer timeout, which is causing this long running test to bump into the too-short timeout value.
(Assignee)

Comment 43

9 years ago
Created attachment 421513 [details] [diff] [review]
updated patch to convert test harnesses to classes

updated to fix known mochitest-ipcplugins (using the env variable) as well as fixing an issue with the default timeout which was causing a failure in the layout tests as we were timing out at 60 seconds vs 330 seconds.

as a note, this failed on mozilla try win32 due to toolkit/xre/test/test_fpuhandler.html.  This appears to be an issue with all win32 boxes on try via bug 539530.
Attachment #420789 - Attachment is obsolete: true

Comment 44

9 years ago
(In reply to comment #43)
> Created an attachment (id=421513) [details]
> updated patch to convert test harnesses to classes
> 
> updated to fix known mochitest-ipcplugins (using the env variable) as well as
> fixing an issue with the default timeout which was causing a failure in the
> layout tests as we were timing out at 60 seconds vs 330 seconds.
> 
> as a note, this failed on mozilla try win32 due to
> toolkit/xre/test/test_fpuhandler.html.  This appears to be an issue with all
> win32 boxes on try via bug 539530.
Applied to try server after disabling the fpuhandler test on try and it went green, so here we go again.  

Attempted landing in changeset 745af1f3dbf5
This was backed out again, FWIW:
http://hg.mozilla.org/mozilla-central/rev/b5d9fbeec04e

Although Joel says it was known random orange, so we should reland it yet again.
(Assignee)

Comment 46

9 years ago
Created attachment 421718 [details] [diff] [review]
updated patch to convert test harnesses to classes (5)

updated for bitrot
Attachment #421513 - Attachment is obsolete: true
(Assignee)

Comment 47

9 years ago
Created attachment 421744 [details] [diff] [review]
updated patch to convert test harnesses to classes (5.1)

ugh...finished testing this and found 1 error/typo in my bitrot fixes.  This passes linux debug leaktests, mochitests and xpcshell.
Attachment #421718 - Attachment is obsolete: true

Comment 48

9 years ago
Pushing again - e636b2d6ceb1

Keep your fingers and toes crossed!

Comment 49

9 years ago
Seems to have stuck.  Can I uncross my fingers?
(Assignee)

Comment 50

9 years ago
yeah, this is in and staying in.  As a matter of fact, I have seen good tinderbox results lately!

There is 1 more patch to land (the biggest two landed) which I am planning on first thing next week.  This 3rd patch is to refactor some of the automation.runapp() logic and runtests.py stuff.  Really just to make the exact same stuff into smaller functions so I can overload them in a subclass for winmo.
Let's take that to a new bug. This one is tired.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Depends on: 540617
(Assignee)

Comment 52

9 years ago
Created attachment 422371 [details] [diff] [review]
refactor automation.py and runtests.py

updated patch to refactor the automation.py and runtests.py files.  This is the last patch that has been reviewed for this bug.
Attachment #422371 - Flags: review+

Comment 53

9 years ago
(In reply to comment #52)
> Created an attachment (id=422371) [details]
> refactor automation.py and runtests.py
> 
> updated patch to refactor the automation.py and runtests.py files.  This is the
> last patch that has been reviewed for this bug.
And landed, crossing fingers, hopefully this is the last we'll see of this bad bug. Changeset: 0aeedccc0125

Comment 54

9 years ago
This broke tests using maxTime, because startTime wasn't given to waitForFinish().  Fixed in http://hg.mozilla.org/mozilla-central/rev/162bcb3e7bb3.
(Assignee)

Comment 55

9 years ago
all code has been checked in, lets close this out for good.
Status: RESOLVED → VERIFIED
Depends on: 601243

Updated

7 years ago
Depends on: 665316
Component: New Frameworks → General
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.