914092 - debug.js NS_ASSERT does not cause tests to fail

Status: RESOLVED DUPLICATE of bug 932419

(Testing :: General, defect)

Description

[Ed Morley [:emorley]]

In bug 910517, the initial patch caused a debug.js NS_ASSERT [1], however the only visible sign of this was from the opt builds timing out due to lack of focus.

eg:
{
22:21:37     INFO -  105036 INFO TEST-PASS | /tests/content/events/test/test_dom_wheel_event.html | document's onwheel property isn't performed
22:21:37     INFO -  105037 INFO TEST-PASS | /tests/content/events/test/test_dom_wheel_event.html | html element's onwheel property isn't performed
22:21:37     INFO -  105038 INFO TEST-PASS | /tests/content/events/test/test_dom_wheel_event.html | body element's onwheel property isn't performed
22:21:37     INFO -  105039 INFO TEST-PASS | /tests/content/events/test/test_dom_wheel_event.html | div element's onwheel property isn't performed
22:21:37     INFO -  ASSERT: reporter ghost-windows has made more than one report
22:21:37     INFO -  105040 INFO TEST-END | /tests/content/events/test/test_dom_wheel_event.html | finished in 86561ms
22:21:37     INFO -  105041 INFO TEST-START | /tests/content/events/test/test_draggableprop.html
22:21:40     INFO -  105042 INFO Error: Unable to restore focus, expect failures and timeouts.
22:21:40     INFO -  105043 INFO TEST-PASS | /tests/content/events/test/test_draggableprop.html | elem1-initial
22:21:40     INFO -  105044 INFO TEST-PASS | /tests/content/events/test/test_draggableprop.html | elem1-true
}

We should:
1) Make these fail the test run.
2) Adjust the stdout failure message so that will be picked up by the usual log parsing regexp.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/debug.js#32

Comment 1

[Ed Morley [:emorley]]

Clint, what do you think would be the best approach here?
1) Make the NS_ASSERT fatal on automation only (given we're going to have focus issues for the rest of the test run anyway)
2) Catch the assertion in each of the test harnesses
3) ...?

Who might be best to drive this forwards? (This bug caused quite a bit of lost time for the devs in bug 910517). Thank you! :-)

Comment 2

[(not currently active) Ted Mielczarek]

A simple way to make these cause failures in debug builds would be to use nsIDebug's assertion():
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIDebug.idl#30

If needed we can check nsIDebug2.isDebugBuild:
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsIDebug2.idl#18

Comment 3

[(not currently active) Ted Mielczarek]

For release builds, perhaps we could rework the XUL_ASSERT_PROMPT check that's already present:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/debug.js#78

to instead report to the console like we do for release builds:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/debug.js#52

(should just be a matter of checking that env var earlier and adding it to that conditional).

I think error console output will cause mochitests to fail, not sure about other test suites. Seems better than putting up assertion dialogs that cause hangs in the test suite, certainly! (Setting XUL_ASSERT_PROMPT right now in the test environment would be a quick way to make these failures go away, at the cost of hiding the failures completely.)

Comment 4

[cmtalbert]

(In reply to Ed Morley [:edmorley UTC+1] from comment #1)
> Clint, what do you think would be the best approach here?
> 1) Make the NS_ASSERT fatal on automation only (given we're going to have
> focus issues for the rest of the test run anyway)
> 2) Catch the assertion in each of the test harnesses
> 3) ...?
> 
> Who might be best to drive this forwards? (This bug caused quite a bit of
> lost time for the devs in bug 910517). Thank you! :-)
So, what's happening is that our assertion message is getting output via that dump statement and then we're getting the dialog popup during the test which is not at all what we want.

I think what we should do is add a third case to the assert code, something we can pref on  when we are running tests.  By preffing it on, we'd then report the error through C.u.reportError allowing the harness some hope of capturing the error text.  Then the harness would be changed (if necessary, I think most of them will capture errors thrown to C.u.reportError) so that they can capture that assertion and report it as a failure of the test.

So, that's my plan.  Ted, what do you think?

It would be a pretty straightforward patch, especially if the test harnesses will pick up the C.u.reportError, which I think they will.

Comment 5

[cmtalbert]

Looks like Ted already responded, I accidentally mid-aired him. I think Ted's approach in comment 3 is essentially the same as what I'm saying.  Let's go that way.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

We don't need a third option or a pref for tests specifically, we should just change NS_ASSERT to unconditionally Cu.reportError() when the assertion fails.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(I'm also tempted to suggest that we should just remove the dumb prompt, but the extra visibility has proven to be useful in some cases.)

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Steven, want to take this?

Comment 9

[(not currently active) Ted Mielczarek]

We should avoid the prompt in automated testing, since even if we add Cu.reportError we'll still wind up with a hang from the dialog. We can re-use the existing XUL_ASSERT_PROMPT env var though, and just tweak things to work better with automated tests.

Comment 10

[Timothy Nikkel (:tnikkel)]

Bug 932419 stops NS_ASSERTs from popping up a modal dialog.

Comment 11

[Ed Morley [:emorley]]

If I've read bug 932419 correctly it means we not only now avoid the hang (due to the modal dialogue being removed), but also should correctly fail the run, due to the use of Cu.reportError.

As such, I think we're now all done here :-)