add ep_unittest.pl to handle unit test output

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
4 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

Details

Attachments

(2 attachments, 3 obsolete attachments)

The unit test boxes currently spit out a lot of expected errors, which get caught by tinderbox's log scraping.  They look like this:
*** 11532 INFO TODO | Text threw exception: TypeError: attr has no properties | 

ep_unix.pl has, as one of its regexes, /Error: /, which is catching these lines.  That regex used to be / Error: /, it was changed in rev 1.5 without much description as to why.  It looks like this regex doesn't even catch most of our compile errors nowadays, the gmake regex above is all we seem to trigger, since gcc's error messages contain "error:" (with a lowercase e), for example:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1188338460.1261.gz

I think we should consider changing this back to get rid of these false positives.  We would miss cases like this, however, but maybe we can change that to fit:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1188375660.1188378805.4780.gz

Comment 1

11 years ago
The comment changed along with the regex.  It added 'or smoketest' so I'm taking it that the leading space was preventing smoketest errors from being caught.  That will definitely be the case for us locally if you change that back.  I'd rather catch a few false positives than miss errors that should be caught.  

It's going to hard to add a regex that will catch generic errors but ignore the js exceptions.  I'm not convinced that we even should make that change since the desire to ignore those errors are limited to a specific set of tests which is not necessarily the general case.  It may be difficult to do securely, but a better alternative would be to add a way to add warning regexes via the admin page so that different trees can catch different errors.
Ok, I can buy that.  What would you think about adding a new error parser specifically for these unit test boxes?  If we added an ep_unittest.pl, we could modify the regexes to not catch these false positives, and additionally we could add regexes to catch the existing failure output of the various unit tests.  You're probably right that some sort of "per-tree error filter" that was configurable via the admin interface would be nice, but that sounds like a whole lot of work for what we really need.

Comment 3

11 years ago
It's not like it's hard to add some error string to test output; we could easily prefix with "Error: " or something without much trouble, so long as we know what the string is and don't need to change it frequently or anything.  Pick something and I'm sure we can deal.
Right, that's easy, and I figured we'd handle that in bug 379327.  I wanted to deal with the false positives specifically in here.

Comment 5

11 years ago
In lieu of a per tree error filter, I think a new error parser for unit tests is fine.
(Assignee)

Updated

11 years ago
Summary: tweak ep_unix.pl to not catch expected errors in unit tests → add ep_unittest.pl to not handle unit test output
(Assignee)

Updated

11 years ago
Summary: add ep_unittest.pl to not handle unit test output → add ep_unittest.pl to handle unit test output
Created attachment 279252 [details]
first cut at ep_unittest.pl

This adds error handling for mochitest, reftest, and browser chrome failure output.  TUnit is kinda hard, we may need to look at the output.  Tihs also linkifies the testcase to bonsai for reftests, since the test is listed on the FAIL line.  We can probably modify MochiTest/browser chrome tests to output the test file name on a FAIL line, and get the same nice behavior.  I've tested this manually by pasting log input into a test script, but I'd like to get this installed on tinderbox-stage so I could throw some real logs at it.
Created attachment 279600 [details] [diff] [review]
updated to link to test filenames

Ok, updated ep_unittest.pl, using the patches from bug 379327 to link to the failed test file for mochitest and browser chrome tests.  Still need to figure out xpcshell tests.
Attachment #279252 - Attachment is obsolete: true
justdave said he could apply this locally to tinderbox-stage.
This is an old log, so it doesn't have my patches from bug 379327, but the error output looks a lot better anyway:
http://tinderbox-stage.mozilla.org/showlog.cgi?tree=Firefox&logfile=1188675840.1188677653.320.gz&buildname=Linux%20qm-centos5-01%20dep%20unit%20test&buildtime=1188675840&errorparser=unittest

I'm going to setup my own unit test buildbot to test the buildbot patch on the other bug, and to test some error output.
Log from a run I just did:
http://tinderbox-stage.mozilla.org/showlog.cgi?log=MozillaTest/1188960032.1189002561.26869.gz

Looks like I need to handle linkifying mochichrome tests, but otherwise it looks pretty good!
Created attachment 279811 [details] [diff] [review]
add ep_unittest.pl

Ok, I'm happy with this for the time being, at least until I get bug 394875 fixed.
Attachment #279600 - Attachment is obsolete: true
Attachment #279811 - Flags: review?(cls)

Updated

11 years ago
Attachment #279811 - Flags: review?(cls) → review+

Comment 12

11 years ago
Comment on attachment 279811 [details] [diff] [review]
add ep_unittest.pl

I forgot to mention that you need the corresponding Makefile change to make sure that the new parser is actually installed.
Checked in with the Makefile change.  Thanks!

This still might need a tweak to handle the xpcshell-based unit tests properly, but that shouldn't be a big deal.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Created attachment 280383 [details] [diff] [review]
add xpcshell unit test support

Should have added this in the first pass.  This adds support for the xpcshell unit test harness.  The source file linking is not really correct yet, I need to fix bug 394875 to get that, but it might take me a bit to get there, and I'd rather just get this in first.
Attachment #280383 - Flags: review?(cls)

Comment 15

11 years ago
Comment on attachment 280383 [details] [diff] [review]
add xpcshell unit test support

Since it's already checked in, can we just get a patch against the existing version?
Attachment #280383 - Flags: review?(cls)
Created attachment 280411 [details] [diff] [review]
add xpcshell unit test support

Whoops, that was actually the old patch.  Sorry.
Attachment #280383 - Attachment is obsolete: true
Attachment #280411 - Flags: review?(cls)

Updated

11 years ago
Attachment #280411 - Flags: review?(cls) → review+
Comment on attachment 280411 [details] [diff] [review]
add xpcshell unit test support

Checked in.
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.