1051635 - ./mach mochitest-plain --run-until-failure keeps on running after failure

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Martijn Wargers (dead)]

Could this be a regression from bug 1046515?

Comment 2

[StevenLee[:slee]]

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

Comment 3

[StevenLee[:slee]]

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

Comment 4

[Martijn Wargers (dead)]

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.

Comment 5

[Ahmed Kachkach [:akachkach]]

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

Comment 6

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1051635-Fix-the-missing-error-handling-in-TestRu.patch

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 7

[Martijn Wargers (dead)]

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 8

[Ahmed Kachkach [:akachkach]]

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 9

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 10

[Ahmed Kachkach [:akachkach]]

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.

Comment 11

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1051635-Fix-the-missing-error-handling-in-TestRu.patch

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

Comment 12

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 13

[Ahmed Kachkach [:akachkach]]

Attachment: 0001-Bug-1051635-Fix-the-missing-error-handling-in-TestRu.patch

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 14

[Chris Manchester (limited bugmail, email directly)]

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?

Comment 15

[Ahmed Kachkach [:akachkach]]

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)

Comment 16

[Ahmed Kachkach [:akachkach]]

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

Comment 17

[Ahmed Kachkach [:akachkach]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2077ae60741f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2077ae60741f

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/254904326f4b