When I do: ./mach mochitest-plain js/xpconnect/tests/mochitest/test_bug789713.html --run-until-failure and I put a failure in test_bug789713.html on purpose, the mochitest run will keep on running for 30 times (the default for --run-until-failure). It should stop after the first time it fails. For ./mach mochitest-browser --run-until-failure seems to work fine. I haven't been able to test the ./mach mochitest-chrome case.

Could this be a regression from bug 1046515?

(In reply to Martijn Wargers [:mwargers] (QA) from comment #1) > Could this be a regression from bug 1046515? Hi Martijn, After I revert bug 1046515, the problem still exists.

I traced some of the code. When [1] is called, then tests stop. I also verify that "runUntilFailure" is true in TestRunner. But the problem is that TestRunner.error is not called even the test cases fail. Does anyone can help? [1] http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#436

There was some removal of ParentRunner.error in SimpleTest.js in http://hg.mozilla.org/mozilla-central/rev/a283c1237d96#l24.167 , which is from bug 886570. Ahmed, could this be caused by the structured logger thing? Currently, after updating my tree, I now get this randomly with --run-until-failure: TEST-INFO | Main app process: killed by SIGHUP 0:13.15 TEST-UNEXPECTED-FAIL | /tests/testing/mochitest/tests/Harness_sanity/test_sanity.html | application terminated with exit code 1 0:13.15 runtests.py | Application ran for: 0:00:07.987645 I filed bug 1052147 for that.

@slee: You're right, this is caused by the fact that TestRunner.error isn't called. Yet another regression from Bug 886570 (sorry :)!) since we're not using the "error" logging function to log failed tests and I didn't notice it had the following side-effects: if (TestRunner.runUntilFailure) { TestRunner._haltTests = true; } I'll try to add some proxy in structured logger that turns this flag on when we have an error.

Hopefully this should fix it (by adding a handler system and checking all "error-y" messages). I'm still having some problems with my local setup so I couldn't test it.

Comment on attachment 8471197 [details] [diff] [review] 0001-Bug-1051635-Fix-the-missing-error-handling-in-TestRu.patch Review of attachment 8471197 [details] [diff] [review]: ----------------------------------------------------------------- Yes, that seems to work! I don't really know about how well the patch code-wise is, though, you would need a real reviewer for that.

Comment on attachment 8471197 [details] [diff] [review] 0001-Bug-1051635-Fix-the-missing-error-handling-in-TestRu.patch Let's see what chmanchester thinks of this :) (btw, this could be used to automatically take these errors into account in the summary by calling TestRunner.updateUI([{ result: false }]);)

Comment on attachment 8471197 [details] [diff] [review] 0001-Bug-1051635-Fix-the-missing-error-handling-in-TestRu.patch Review of attachment 8471197 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/tests/SimpleTest/TestRunner.js @@ +133,5 @@ > this.structuredFormatter = new StructuredFormatter(); > > + this.addHandler = function(handler) { > + this.handlers.push(handler); > + }; I'd like to avoid getting this further out of sync with StructuredLog.jsm. @@ +257,1 @@ > this._dumpMessage(allData); If we do decide this is necessary, I think it would be reasonable to let the idea of a handler subsume that of a dump function. @@ +257,4 @@ > this._dumpMessage(allData); > }; > > this._dumpMessage = function(message) { Would it be sufficient to put our check in here by looking for expected in the message? I think that would cover most of the cases we care about. Let me know if you don't think that will work and we can talk about another approach. @@ +690,5 @@ > } > > function runNextTest() { > if (TestRunner.currentTestURL != TestRunner.getLoadedTestURL()) { > + TestRunner.structuredLogger.testEnd(TestRunner.currentTestURL, This change probably makes sense, but it seems unrelated.

I thought adding handlers would make it work similarly to StructuredLog.jsm (where you have mutators which can act as handlers). Otherwise yes, I guess I can just add that logic into _dumpMessage and reference TestRunner from there if you think it's not worth adding a list of handlers. For the last change (from testStatus to testEnd), it is related because it's also a refactoring error I've done when converting those .error messages.

This implements the fix in a way that makes it easier to integrate StructuredLog.jsm later.

Comment on attachment 8471716 [details] [diff] [review] 0001-Bug-1051635-Fix-the-missing-error-handling-in-TestRu.patch Review of attachment 8471716 [details] [diff] [review]: ----------------------------------------------------------------- You're on the right track, please address the two issues below. ::: testing/mochitest/tests/SimpleTest/TestRunner.js @@ -435,5 @@ > - // You've hit this line because you requested to break into the > - // debugger upon a testcase failure on your test run. > - debugger; > - } > -}; I don't think we can delete this, but with this change it can just become an alias for the StructuredLogger method. @@ +687,5 @@ > + "OK", > + TestRunner.getLoadedTestURL() + > + " finished in a non-clean fashion, probably" + > + " because it didn't call SimpleTest.finish()", > + {subtest: TestRunner.getLoadedTestURL()}); This calls testEnd again just below, so this will result in ending the test twice. It's not ideal, but I'd prefer to leave this alone for the purposes of this bug.

I had to factor the error code in a function to keep the .error function (since the same code would have to be executed if logging is not enabled but someone is using that function). For the record, I think that was only used in the test harness and not exposed through SimpleTest.

Comment on attachment 8471842 [details] [diff] [review] 0001-Bug-1051635-Fix-the-missing-error-handling-in-TestRu.patch Review of attachment 8471842 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. ::: testing/mochitest/tests/SimpleTest/TestRunner.js @@ +476,5 @@ > window.setTimeout('TestRunner._makeIframe("'+url+'", '+(retry+1)+')', 1000); > return; > } > > + TestRunner.structuredLogger.info("Error: Unable to restore focus, expect failures and timeouts."); I'm not sure what was here before, but is this change needed now?

Yes! I went and looked at the patch in bug 886570, and that was an info message, not an error. (so we don't want it to make --run-until-failure stop now)

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0b5665b3e73e

Try run looks OK, except for this failure in Android 4.0 Debug m-1: "PROCESS-CRASH | /tests/content/base/test/test_bug353334.html | application crashed [Unknown top frame]"; Not sure it's related.