JS exceptions aren't being propagated to several of the test harness' onerror handlers

NEW
Unassigned

Status

Testing
General
--
major
5 years ago
9 months ago

People

(Reporter: emorley, Unassigned)

Tracking

(Depends on: 11 bugs, Blocks: 2 bugs)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tracking])

Attachments

(2 attachments)

A recentish TBPL parser change has made JS exceptions during test runs more noticeable, since the regexp used for catching Python exceptions also catches many JS exceptions.

This has made it clear that we are failing to propagate JS exceptions in automation to the test harness' onerror handlers - since these runs show as passing. This is happening for at least mochitest, reftest/jsreftest, xpcshell.

There are both exceptions dumped straight to stdout, eg bug 910275, and those that are output via xpc::SystemErrorReporter() [1] eg bug 908426.

We should make uncaught exceptions fail the test run & make the output TBPL compatible - ie more of form:
TEST-UNEXPECTED-FAIL | testname | uncaught exception - FooError: bar is null at line 123
etc


[1] http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#263
Meant to say, I imagine we may need to initially have a way to whitelist/annotate tests as causing uncaught exceptions, otherwise we'll constantly be playing catch up and never be able to land the changes (there are still many open bugs).
Clint, I don't suppose you can find someone to drive this? Once we get a WIP patch that causes runs to fail, the sheriffs are happy to pick apart the Try runs and file the bugs we need to get the failures fixed so it can land (or at least come up with a list of tests that need annotating initially) :-)
Flags: needinfo?(ctalbert)
Depends on: 920532

Comment 3

5 years ago
(In reply to Ed Morley [:edmorley UTC+1] from comment #2)
> Clint, I don't suppose you can find someone to drive this? Once we get a WIP
> patch that causes runs to fail, the sheriffs are happy to pick apart the Try
> runs and file the bugs we need to get the failures fixed so it can land (or
> at least come up with a list of tests that need annotating initially) :-)

I thought we'd discussed this on IRC and came up with a solution, so I was surprised to see this still in my queue. :-(  Do we still need a driver on this? I thought we found one and Ted filed a bug, but I'm unfortunately not finding it...
Flags: needinfo?(ctalbert)
(In reply to Clint Talbert ( :ctalbert ) from comment #3)
> I thought we'd discussed this on IRC and came up with a solution, so I was
> surprised to see this still in my queue. :-(  Do we still need a driver on
> this? I thought we found one and Ted filed a bug, but I'm unfortunately not
> finding it...

I think that was the conversation in bug 914092 maybe, which is a slightly different issue?
Flags: needinfo?(ctalbert)
Joel, don't suppose you'd know who would be best to look at this? (Given Clint's now away for a bit) 
Thank you :-)
Flags: needinfo?(jmaher)
while I don't understand this issue fully, it appears that we are not reading the output of the console in the javascript mochikit harness bits.  If that is the case, I suspect we will have hundreds of errors, especially on debug.  

if that is desired, the javascript (in browser portion) of the harness would need to examine the console logs and search for errors.  this would ideally happen on test cleanup.  Possibly jhammel, wlach, ted, dminor would be qualified candidates for this.  I suspect we print out the console log to the file, so we would get double errors printed if we turned those into log.error() calls.  the right way to do this would be have a common module to get these and define the parsing of the console messages and whitelist stuff in the same file.
Flags: needinfo?(jmaher)
Flags: needinfo?(ctalbert)
Seems like it should be the test harnesses' responsibility to catch exceptions - log parsing isn't going to be very reliable.

The SimpleTest-based harnesses are using an onerror handler (http://hg.mozilla.org/mozilla-central/annotate/cc4a3f3f899e/testing/mochitest/tests/SimpleTest/SimpleTest.js#l1148), and that misses some cases. A console observer should be able to catch them more reliably, I think.
This bug represents a not-insignificant gap in our test coverage - who is able to drive this? :-)
Severity: normal → major
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(ctalbert)
Neil, could you look into this? I guess we'd want to start by adding more robust exception catching in the test harness (comment 7) and seeing how many new failures pop out...
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(ctalbert)
Depends on: 934487
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #9)
> Neil, could you look into this? I guess we'd want to start by adding more
> robust exception catching in the test harness (comment 7) and seeing how
> many new failures pop out...
Flags: needinfo?(enndeakin)

Comment 11

5 years ago
I noticed this bugzilla entry from a discussion in dev-platform mailing list.

(In reply to Ed Morley [:edmorley UTC+0] from comment #0)

> This has made it clear that we are failing to propagate JS exceptions in
> automation to the test harness' onerror handlers - since these runs show as
> passing. This is happening for at least mochitest, reftest/jsreftest,
> xpcshell.
> 

Although I know thunderbird is no longer supported by mozilla foundation per se, and is 
now in community support mode, I cannot help report that
|make mozmill| of thunderbird test suite also does not catch
exceptions very well.

I am manually counting these exceiptions in my test run (with script of course), 
and periodically report the top-offenders in the top positions in the list of descending order of frequencies in the test run.
So whatever will come out of this bugzilla entry, 
it will be great if the exceptions in the |make mozmill| can be caught as well.
(But obviously |make mozmill| uses mozmill test harness. So it may need a different patch.)

It sure does not make sense to let a test pass when an exception is raised and not caught and thrown all the way to the top-level during its execution.

Up to now, I thought it was taken for granted because nobody seems to be complaining about it but me (?) but obviously, this was an oversight of the test harness. It finally makes sense to me.


TIA
Depends on: 951148
Neil, any news on this? :-)
Depends on: 951174
Depends on: 951182
No longer depends on: 951174
Depends on: 903274

Comment 13

5 years ago
Any progress on this matter?
If I did this for talos, we would fail almost all of our tests.  I have an example of a console listener in talos:
http://hg.mozilla.org/build/talos/file/2bcf422011d1/talos/startup_test/startup_test.html#l24

This just dumps the data out, should I start with ts and if there are errors in the console output turn that into failure?
Depends on: 955948
(In reply to Joel Maher (:jmaher) from comment #14)
> If I did this for talos, we would fail almost all of our tests.  I have an
> example of a console listener in talos:
> http://hg.mozilla.org/build/talos/file/2bcf422011d1/talos/startup_test/
> startup_test.html#l24

Yeah I fully believe us to have orange runs on most suites (including non-talos) until we fix a few things.

> This just dumps the data out, should I start with ts and if there are errors
> in the console output turn that into failure?

If you could, that would be great :-)
Depends on: 952857
Duplicate of this bug: 953113
Depends on: 957666
Depends on: 957697
Depends on: 966572
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #9)
> Neil, could you look into this? I guess we'd want to start by adding more
> robust exception catching in the test harness (comment 7) and seeing how
> many new failures pop out...

(In reply to Ed Morley [:edmorley UTC+0] from comment #12)
> Neil, any news on this? :-)

Hi Neil - no news since October, please could you give us an ETA? If you don't have time for this please let us know and I'll ask Gavin to find someone else to take a look.

Cheers!
I'll throw this in the desktop backlog and see what we come up with.
Blocks: 950073
Flags: needinfo?(enndeakin)

Updated

5 years ago
Whiteboard: [tracking]

Updated

5 years ago
Whiteboard: [tracking] → [triage] [tracking]

Comment 19

5 years ago
Created attachment 8376390 [details] [diff] [review]
consoleerrortests

This is a work in progress. A thousand or so failures occur when running tests.

https://tbpl.mozilla.org/?tree=Try&rev=7b04eca9b21a
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Depends on: 976205

Updated

4 years ago
Blocks: 981530
No longer blocks: 950073
Warrants being in backlog imo; moving back.
Blocks: 950073
No longer blocks: 981530

Updated

4 years ago
No longer blocks: 950073
Whiteboard: [triage] [tracking] → [tracking]
Neil, I don't suppose you've had a chance to look at this any more? :-)
Flags: needinfo?(enndeakin)

Comment 22

4 years ago
I did investigate but there are so many different kids of failures revealed by this patch that it would need more understanding of the tests in question to fix them.

I did fix a pile of warnings with bug 475981 though. There's also a bunch of other invalid charset type warnings that could be disabled or fixed in bulk probably.
Flags: needinfo?(enndeakin)

Comment 23

4 years ago
Created attachment 8421059 [details] [diff] [review]
Miscellaneous fixes

Attached is a few fixes I had started on for some tests in case someone else wants to take a look. I don't recall the details of the issues though.
Depends on: 1012624
Depends on: 1030951
Depends on: 1038279
Depends on: 1040932
Depends on: 1015034
Blocks: 1048775
Depends on: 1049294
Depends on: 1054210

Comment 24

4 years ago
Ed, in the case of b2g/gaia, would this bug involve trawling the gecko.log or logcat for any errors that occurred even in the case that the test itself passed?
Flags: needinfo?(emorley)
(In reply to Zac C (:zac) from comment #24)
> Ed, in the case of b2g/gaia, would this bug involve trawling the gecko.log
> or logcat for any errors that occurred even in the case that the test itself
> passed?

I would hope the harness would catch it (along the lines of the first patch attached here) :-)
Flags: needinfo?(emorley)
Depends on: 1060812
Depends on: 1080457
Depends on: 1110110
Depends on: 1124217
Depends on: 1124224
Depends on: 1139937
Depends on: 1142090
Depends on: 1160492
Depends on: 1162104
Depends on: 1162961
Duplicate of this bug: 1011538
See Also: → bug 1170418

Updated

3 years ago
Assignee: enndeakin → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.