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.

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.

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.

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.

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.

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.

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.

Did you want review on those last two patches as well?

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.

updated automation.py to be a class and this patch uses 2 spaces instead of 4.

3rd patch in series, allows for smaller functions for easier subclass overrides.

ted: any ETA on this review? its blocking a Q4 security goal RelEng is working on with Jesse.

Got tangled up in some Breakpad work. Hoping to get my review queue cleaned out ASAP.

I will have unbitrot patches uploaded tonight

updated for bitrot, tested on linux from objdir and package-tests

updated for bitrot, tested on linux in objdir and package-tests

updated for bitrot, tested on linux in objdir and package-tests

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.

final version with Ted's nit fixed.

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.

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.

>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.

Just merging patch to tip, no changes.

Final patch submitted, carrying review forward. This was submitted as changeset: 85cd0f297421

(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

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)

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

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

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.

lets give this another try. this should pass mochitest-ipcplugins

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

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.

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.

(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.

updated for bitrot

ugh...finished testing this and found 1 error/typo in my bitrot fixes. This passes linux debug leaktests, mochitests and xpcshell.

Pushing again - e636b2d6ceb1 Keep your fingers and toes crossed!

Seems to have stuck. Can I uncross my fingers?

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.

updated patch to refactor the automation.py and runtests.py files. This is the last patch that has been reviewed for this bug.

(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

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

all code has been checked in, lets close this out for good.