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

RESOLVED FIXED in Firefox 33

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: martijn.martijn, Assigned: akachkach)

Tracking

unspecified
mozilla34
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
Could this be a regression from bug 1046515?

Comment 2

5 years ago
(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

5 years ago
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
Reporter

Comment 4

5 years ago
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.
Assignee

Comment 5

5 years ago
@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.
Assignee: nobody → akachkach
Assignee

Updated

5 years ago
Blocks: 886570
Assignee

Comment 6

5 years ago
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.
Attachment #8471197 - Flags: review?(martijn.martijn)
Reporter

Comment 7

5 years ago
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.
Attachment #8471197 - Flags: review?(martijn.martijn) → review+
Assignee

Comment 8

5 years ago
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 }]);)
Attachment #8471197 - Flags: review+ → review?(cmanchester)
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.
Attachment #8471197 - Flags: review?(cmanchester) → review-
Assignee

Comment 10

5 years ago
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.
Assignee

Comment 11

5 years ago
This implements the fix in a way that makes it easier to integrate StructuredLog.jsm later.
Attachment #8471197 - Attachment is obsolete: true
Attachment #8471716 - Flags: review?(cmanchester)
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.
Attachment #8471716 - Flags: review?(cmanchester) → review-
Assignee

Comment 13

5 years ago
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.
Attachment #8471716 - Attachment is obsolete: true
Attachment #8471842 - Flags: review?(cmanchester)
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?
Attachment #8471842 - Flags: review?(cmanchester) → review+
Assignee

Comment 15

5 years ago
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)
Assignee

Comment 17

5 years ago
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.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2077ae60741f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.