Closed Bug 996313 Opened 10 years ago Closed 10 years ago

JavascriptMessageParser does not immediately end the test when a js assertion fails

Categories

(Firefox for Android Graveyard :: Testing, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(1 file, 2 obsolete files)

Perhaps this can be done by sending a message to Gecko/Java, and having Java throw the assertion.
This was by design, http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/helpers/JavascriptMessageParser.java#64

But if we decide to change it, we only need to make one change in JavascriptMessageParser.
(In reply to Jim Chen [:jchen :nchen] from comment #1)
> This was by design,

If one assertion fails, it could throw off the rest of the test and the remaining assertions may be invalid - as far as I know, that's why other testing frameworks (our Java included) end immediately when an assertion fails.

Nick (since I think you wrote the code initially - feel free to punt if not), why do you feel this should be different for JS?
Flags: needinfo?(nalexander)
Summary: JavascriptTest does not immediately fail when a js assertion fails → JavascriptMessageParser does not immediately end the test when a js assertion fails
(In reply to Michael Comella (:mcomella) from comment #3)
> (In reply to Jim Chen [:jchen :nchen] from comment #1)
> > This was by design,
> 
> If one assertion fails, it could throw off the rest of the test and the
> remaining assertions may be invalid - as far as I know, that's why other
> testing frameworks (our Java included) end immediately when an assertion
> fails.

For most Java frameworks, this is correct.

> Nick (since I think you wrote the code initially - feel free to punt if
> not), why do you feel this should be different for JS?

Personally, I don't feel this should be different and we should die on the first error, but I think I was following mochitest at the time, which maybe does not die on the first error?  Reading about xpcshell suggests it does die on the first error.

I am a little concerned about cleanly killing the test in this situation, but I'd be happy to see the change made.  In general, I'd like to keep the invariant that "JavascriptTest == xpcshell in Gecko".
Flags: needinfo?(nalexander)
> I am a little concerned about cleanly killing the test in this situation,
> but I'd be happy to see the change made.

Do you mean cleaning up the JavaScript elements of the test? I took a quick
look into the code, but the rabbit hole went a little deeper than I was hoping
to look. I tested this patch by purposely failing a test and seeing how the
next text faired, and it seemed okay, by looking at the debug output (though I
suppose we might be leaving around resources in memory which could fail in the
long run).

We seem to run the test in a sandbox [1] (though I don't know the details), so
perhaps that will ensure listeners and such are disconnected?

I can look into the code further if you'd like, or file a followup.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop_testharness.js?rev=f51f24a55515#24
Attachment #8407034 - Flags: review?(nalexander)
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Comment on attachment 8407034 [details] [diff] [review]
Kill test when JavascriptMessageParser finds a failed js assertion.

Review of attachment 8407034 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, wait -- I think I remember what's up here.

Harnesses within harnesses!  What happens is that the Javascript level (a single test_*.js file) can have multiple test functions inside it.  We get one of these TEST-UNEXPECTED messages for *each test function*.  That is, the Javascript framework does stop the the individual test on first failure.

We catch the message and want to format it as a Robocop failure, but we don't want to stop running the Javascript test functions.  That's what the comment was supposed to describe.  Big r+ to a documentation-only patch that says something like that, but in better language :)  And bonus points for making sure that's what happens :)
Attachment #8407034 - Flags: review?(nalexander) → review-
In my testing,

> the Javascript framework does stop the the individual test on first failure.

True.

> We get one of these TEST-UNEXPECTED messages for *each test function*.

False - the test fails immediately after one failure.

Each function to be tested, added by `add_test`, must call `run_next_test`. However, since each test appears to throw on the first failure, `run_next_test` never gets called and all of the test functions are not called (seems pretty :\ ).

This also seems to be the case with `add_task` despite the lack of `run_next_test` (probably called by the framework).

> Big r+ to a documentation-only patch that says something like that, but
> in better language :)

I agree with a documentation upgrade, but I think we should also be able to switch on immediate Java failures for JS assertion failures, for use in JavascriptBridge.
Unfortunately, the stack traces aren't great:

21 INFO TEST-UNEXPECTED-FAIL | testJavascriptBridge | testJavascriptBridge.js - 1 == 2 - See following stack:

Looks like the messages sent can be more comprehensive though:

{"message":"\nTEST-UNEXPECTED-FAIL | testJavascriptBridge.js | 1 == 2 - See following stack:\n","innerType":"progress","type":"Robocop:JS"}

Followup?
Note that I have not investigated concerns regarding Javascript cleanup.
After throwing in some `dump()`s, I don't think Javascript cleans itself up at all in testJavascriptBridge (i.e. the function given to `do_register_cleanup` does not appear to get called).

We might be able to do this by figuring out the JS cleanup hooks, sending a Java->JS message over the bridge, calling the cleanup functions sychronously, sending a JS->Java "finished" message, and letting Java propagate the assertion Exception.

Jim, do you have any input?
Flags: needinfo?(nchen)
(In reply to Michael Comella (:mcomella) from comment #11)
> After throwing in some `dump()`s, I don't think Javascript cleans itself up
> at all in testJavascriptBridge (i.e. the function given to
> `do_register_cleanup` does not appear to get called).

Is that when the test exits normally or when you throw an exception?

> We might be able to do this by figuring out the JS cleanup hooks, sending a
> Java->JS message over the bridge, calling the cleanup functions
> sychronously, sending a JS->Java "finished" message, and letting Java
> propagate the assertion Exception.

Do you think proper cleanup is important when we have an exception in JS? I don't think we do any cleanup when we have an assert exception in Java; we just crash IIRC.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen :nchen] from comment #12)
> (In reply to Michael Comella (:mcomella) from comment #11)
> > After throwing in some `dump()`s, I don't think Javascript cleans itself up
> > at all in testJavascriptBridge (i.e. the function given to
> > `do_register_cleanup` does not appear to get called).
> 
> Is that when the test exits normally or when you throw an exception?

Both.

> Do you think proper cleanup is important when we have an exception in JS?

Depends on how the harness creates testing environments, right? If we create an entirely new environment, great! If we don't, we have to clean up. In particular, do we remove files stored on disk? What about event listeners?

> I don't think we do any cleanup when we have an assert exception in Java; we
> just crash IIRC.

As far as I know, JUnit (and the Android framework that is based off of it) catches the Exception in the harness, keeps the JVM, calls tearDown, and moves on to the next test.
Comment on attachment 8409237 [details] [diff] [review]
Add boolean for ending test on assertion failure in JavascriptMessageParser.

Review of attachment 8409237 [details] [diff] [review]:
-----------------------------------------------------------------

If it works for you, it works for me!

::: mobile/android/base/tests/helpers/JavascriptMessageParser.java
@@ +34,5 @@
>      private String lastTestName = "";
>      // Have we seen a message saying the test is finished?
>      private boolean testFinishedMessageSeen = false;
> +    // Should we end the test if we see a JS assertion failure?
> +    private boolean endOnAssertionFailure = false;

nit: final, and move the docstrings to the constructors.

@@ +39,3 @@
>  
>      public JavascriptMessageParser(final Assert asserter) {
>          this.asserter = asserter;

nit: pass through to |this(asserter, end)| (and just use @link in the docstring).  Consider only having the two argument version, and possibly an enum for the boolean.

@@ +72,5 @@
> +                    // Now we can end the test, if applicable.
> +                    if (this.endOnAssertionFailure) {
> +                        throw e;
> +                    }
> +                    // Otherwise, swallow the Error. The JS framework we're

nit: this explanation is really valuable; mostly duplicate it in the constructor docstring above.
Attachment #8409237 - Flags: review?(nalexander) → review+
Comment on attachment 8412054 [details] [diff] [review]
Add boolean for ending test on assertion failure in JavascriptMessageParser.

Move r+.
Attachment #8412054 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/96eefe207020
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.