Closed Bug 550481 Opened 14 years ago Closed 14 years ago

Exceptions from the do_execute_soon callback should be logged and fail the test

Categories

(Testing :: XPCShell Harness, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mossop, Assigned: mossop)

Details

Attachments

(1 file, 1 obsolete file)

Currently if the callback for do_execute_soon throws an exception other than the do_check_ tests nothing is displayed and the test hangs (because do_test_finished won't get called). Instead we should catch any exception, log it and end the test.
Attached patch patch rev 1 (obsolete) — Splinter Review
This catches any exception, for those that weren't triggered by do_throw in the first place we log the exception and any available stack (it's only available for pure JS exceptions, XPCOM exceptions won't have any stack info).

This also adds an automatic do_test_pending and do_test_finished saving the caller from doing so (though there is no harm in them still doing so).

Ran this through the try server and everything still passes. If we're lucky this might show more info for some of the intermittent failures in xpcshell tests, but worse case it still makes debugging these tests much easier.
Attachment #430709 - Flags: review?(ted.mielczarek)
Comment on attachment 430709 [details] [diff] [review]
patch rev 1

Actually let me change this a bit
Attachment #430709 - Attachment is obsolete: true
Attachment #430709 - Flags: review?(ted.mielczarek)
Attached patch patch rev 1Splinter Review
Nicer version that does the stack trace for exceptions from run_test too.
Attachment #430729 - Flags: review?(ted.mielczarek)
Comment on attachment 430729 [details] [diff] [review]
patch rev 1

>diff --git a/testing/xpcshell/example/unit/test_execute_soon.js b/testing/xpcshell/example/unit/test_execute_soon.js
>new file mode 100644
>--- /dev/null
>+++ b/testing/xpcshell/example/unit/test_execute_soon.js
>@@ -0,0 +1,52 @@
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1

It's acceptable to license test files in the Public Domain nowadays:
https://wiki.mozilla.org/License_Policy#Acceptable_Licenses


>diff --git a/testing/xpcshell/head.js b/testing/xpcshell/head.js
>--- a/testing/xpcshell/head.js
>+++ b/testing/xpcshell/head.js
>@@ -187,22 +206,42 @@ function _load_files(aFiles) {
> 
> function do_timeout(delay, func) {
>   var timer = Components.classes["@mozilla.org/timer;1"]
>                         .createInstance(Components.interfaces.nsITimer);
>   timer.initWithCallback(new _TimerCallback(func, timer), delay, timer.TYPE_ONE_SHOT);
> }
> 
> function do_execute_soon(callback) {
>+  do_test_pending();
>   var tm = Components.classes["@mozilla.org/thread-manager;1"]
>                      .getService(Components.interfaces.nsIThreadManager);
> 
>   tm.mainThread.dispatch({
>     run: function() {
>-      callback();
>+      try {
>+        callback();
>+      } catch (e) {
>+        // Print exception, but not do_throw() result.
>+        // Hopefully, this won't mask other NS_ERROR_ABORTs.
>+        if (!_quit || e != Components.results.NS_ERROR_ABORT) {
>+          dump("TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | " + e);

Hm, do you really want the !_quit here? I admit, I find that if statement hard to follow even in the execute_test() bit you copied it from.

This is good stuff, should help diagnose random orange in these cases. Also the extra do_test_{pending,cleanup} pair is a nice touch.
Attachment #430729 - Flags: review?(ted.mielczarek) → review+
(In reply to comment #4)
> (From update of attachment 430729 [details] [diff] [review])
> >diff --git a/testing/xpcshell/example/unit/test_execute_soon.js b/testing/xpcshell/example/unit/test_execute_soon.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/testing/xpcshell/example/unit/test_execute_soon.js
> >@@ -0,0 +1,52 @@
> >+/* vim:set ts=2 sw=2 sts=2 et: */
> >+/* ***** BEGIN LICENSE BLOCK *****
> >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> 
> It's acceptable to license test files in the Public Domain nowadays:
> https://wiki.mozilla.org/License_Policy#Acceptable_Licenses

Fine by me, I'll update it.

> >+        // Print exception, but not do_throw() result.
> >+        // Hopefully, this won't mask other NS_ERROR_ABORTs.
> >+        if (!_quit || e != Components.results.NS_ERROR_ABORT) {
> >+          dump("TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | " + e);
> 
> Hm, do you really want the !_quit here? I admit, I find that if statement hard
> to follow even in the execute_test() bit you copied it from.

I think yes. If _quit is false then no test fail has been caught by a do_check_... test so we certainly should log the exception. Likewise if e is not NS_ERROR_ABORT then this exception did not come from a do_check_... fail (could be an exception that happened after such a test fail) so we should log it. The only ones we don't log are those where _quit is true and e is NS_ERROR_ABORT in which case the exception is likely just the one from the do_check_... failure and has already been logged, though as the comment suggests it isn't impossible for us to be hiding other exceptions there.
http://hg.mozilla.org/mozilla-central/rev/29cb33290f7e
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: