Closed Bug 652494 Opened 14 years ago Closed 13 years ago

Uncaught JavaScript exceptions in chrome mochitests should cause test failures

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla11

People

(Reporter: heycam, Assigned: heycam)

References

(Depends on 4 open bugs, Blocks 2 open bugs, )

Details

Attachments

(1 file)

Some tests in mochitest-other output JavaScript strict warnings about MochiKit, for example: 2243 INFO TEST-START | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOMWindowCreated.xul ++DOMWINDOW == 40 (10797F58) [serial = 560] [outer = 07A1A418] pldhash: for the table at address 0B324918, the given entrySize of 48 probably favors chaining over double hashing. ++DOCSHELL 0B3248B0 == 7 ++DOMWINDOW == 41 (0FD98DC8) [serial = 561] [outer = 00000000] WARNING: Subdocument container has no content: file e:/builds/moz2_slave/cen-w32-dbg/build/layout/base/nsDocumentViewer.cpp, line 2380 ++DOMWINDOW == 42 (0FD99380) [serial = 562] [outer = 0FD98D78] WARNING: Subdocument container has no content: file e:/builds/moz2_slave/cen-w32-dbg/build/layout/base/nsDocumentViewer.cpp, line 2380 WARNING: Subdocument container has no content: file e:/builds/moz2_slave/cen-w32-dbg/build/layout/base/nsDocumentViewer.cpp, line 2380 JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 18: assignment to undeclared variable MochiKit JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 995: assignment to undeclared variable compare JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 996: assignment to undeclared variable compose JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 997: assignment to undeclared variable serializeJSON JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 1606: assignment to undeclared variable reduce JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 1783: assignment to undeclared variable printfire JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 3342: assignment to undeclared variable withWindow JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 3343: assignment to undeclared variable withDocument JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4648: assignment to undeclared variable i JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4960: assignment to undeclared variable connect JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4961: assignment to undeclared variable disconnect JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4962: assignment to undeclared variable disconnectAll JavaScript strict warning: chrome://mochikit/content/MochiKit/packed.js, line 4963: assignment to undeclared variable signal pldhash: for the table at address 0B325A58, the given entrySize of 48 probably favors chaining over double hashing. ++DOCSHELL 0B3259F0 == 8 ++DOMWINDOW == 43 (0FD80E08) [serial = 563] [outer = 00000000] WARNING: NS_ENSURE_SUCCESS(rv, 0) failed with result 0x8000FFFF: file e:/builds/moz2_slave/cen-w32-dbg/build/content/base/src/nsContentUtils.cpp, line 2915 WARNING: NS_ENSURE_TRUE(pusher.Push(aBoundElement)) failed: file e:/builds/moz2_slave/cen-w32-dbg/build/content/xbl/src/nsXBLProtoImplMethod.cpp, line 327 WARNING: NS_ENSURE_TRUE(aURI) failed: file e:/builds/moz2_slave/cen-w32-dbg/build/content/base/src/nsContentUtils.cpp, line 5023 ++DOMWINDOW == 44 (0FD7FD70) [serial = 564] [outer = 0FD80DB8] 2244 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOMWindowCreated.xul | DOMWindowCreated: [object Window @ 0x7d13e10 (native @ 0xfd7fd70)] - "DOMWindowCreated" should equal "DOMWindowCreated" 2245 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOMWindowCreated.xul | doneFunction was called 2246 INFO TEST-END | chrome://mochitests/content/chrome/dom/tests/mochitest/chrome/test_DOMWindowCreated.xul | finished in 595ms There are 12 of these in mochitest-other output these days. The lines aren't prefaced with "TEST-UNEXPECTED-FAIL" so don't cause Tinderbox failures, but it makes me wonder whether the tests are actually running properly. First, how are these few tests running the MochiKit files in strict mode? Second, is the right solution to remove the strict mode hazards from MochiKit or to force these files to be run in non-strict mode in these tests?
First, this is not ES5 strict mode, this is javascript.options.strict, which is enabled by default in debug builds. It's an older pref that covers a set of things that probably has some overlap with ES5 strict, but doesn't make anything fatal, I don't think, it just warns about them. I doubt it's causing any problems, those just look like the usual "assigning to a variable without using the var keyword" sorts of things. I don't think it's worth trying to fix MochiKit to fix things like "assignment to undeclared variable foo", unless we could drop-in a newer version as a replacement without breaking everything.
Understood. Those javascript.options.strict warnings get reported to the onerror handler, which find their way to the `function simpletestOnerror` at the end of SimpleTest.js. In there, they are reported with `ok(false, ...)` calls, but for whatever reason these aren't being translated into TEST-UNEXPECTED-FAILUREs in the output. I was doing some cleanup of the logging functions as part of bug 642175 which made these become "real" errors, which is how they came to my attention. From bug 427500 comment 7 it sounds like we have patches on top of the base MochiKit files, so I doubt we could drop in a replacement easily anyway. I think there is value in having strict warnings become real errors -- people are rarely going to notice these warning lines dumped in the logs if they don't trigger test failures. (TBH I'm wondering whether it's worth eventually moving off MochiKit. It seems to have a lot of cruft and functionality we don't need.)
Might be an option. I don't think we're actually using much of it, honestly. I believe it was just an easy place to start when sayrer first implemented Mochitest. It sure would make the test suite name ironic, though. :)
Summary: Some mochitests emit JavaScript Strict Warning messages from MochiKit/packed.js → Uncaught JavaScript exceptions in chrome mochitests should cause test failures
Depends on: 667155
No longer depends on: 667155
For what it's worth, the lack of fail-on-exception in browser-chrome tests has been a real problem for me today: I had a patch that was broken, but the test kept claiming it was all OK....
Depends on: 670831
Depends on: 670838
Depends on: 670452
Depends on: 670849
Depends on: 670851
Depends on: 670852
Depends on: 670855
Depends on: 670856
Depends on: 670857
Depends on: 670858
Depends on: 684230
Depends on: 681392
FWIW, this bug caused bug 700190, where test_aboutmemory.xul was broken but nobody realized becaukse the exception was swallowed.
Depends on: 700190
I'm told that making exceptions cause failures is difficult because it'd cause lots of new failures. A half-assed improvement would be to have an opt-in option that lets a test request exceptions-as-failures.
Better than that would be an opt-in to request exceptions to be ignored instead of reported, and for the existing known tests that do throw uncaught exceptions to be modified to opt in to this.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Depends on: 700553
Depends on: 700556
Depends on: 700563
Depends on: 700912
The patch does the following: 1. Makes uncaught JS exceptions cause a test failure in (browser-)chrome mochitests instead of just dump()ing them. 2. Introduces a SimpleTest.ignoreAllUncaughtExceptions() function that causes all uncaught JS exceptions to be reported with todo(false) instead of ok(false). 3. Adds a call to ignoreAllUncaughtExceptions() for the tests that currently have uncaught exceptions and haven't been fixed yet (the open bugs blocking this one). 4. Simplifies the searching for the TestRunner/SpecialPowers objects done at the top of SimpleTest.js. (I stuck this in an anonymous function to avoid polluting tests with the variables used as part of this.) 5. Ensures that SimpleTest.finish() is not called automatically on page load for secondary test windows that load SimpleTest.js -- this was causing multiple tests to run at once, causing confusing/incorrect test failure reporting. Once this lands I'll file a followup bug to remove the ignoreAllUncaughtExceptions() function after all the broken tests have been fixed.
Attachment #573645 - Flags: review?(jmaher)
Nice patch! Nice description of the patch! :)
Comment on attachment 573645 [details] [diff] [review] Report uncaught JS exceptions in chrome mochitests as test failures [Checked in: Comment 12] Review of attachment 573645 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the one comment addressed. ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +33,5 @@ > + parentRunner = w.wrappedJSObject.TestRunner; > + } > + } > + if (!window.SpecialPowers) { > + window.SpecialPowers = w.SpecialPowers; Can we keep the comment about which test case and scenarios require this. SpecialPowers is dicey when it comes to defining it and the one remaining change (bug 695292) requires some work on this.
Attachment #573645 - Flags: review?(jmaher) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e804728b128b I put the comments back in (reworded slightly since they're in a different position).
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 703176
Depends on: 704981
Depends on: 710863
Depends on: 717753
Depends on: 707599
Attachment #573645 - Attachment description: Report uncaught JS exceptions in chrome mochitests as test failures. → Report uncaught JS exceptions in chrome mochitests as test failures [Checked in: Comment 12]
No longer blocks: 718239
Depends on: 718239
No longer depends on: 717753
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: