Closed
Bug 394250
Opened 17 years ago
Closed 17 years ago
add ep_unittest.pl to handle unit test output
Categories
(Webtools Graveyard :: Tinderbox, defect)
Webtools Graveyard
Tinderbox
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ted, Assigned: ted)
References
Details
Attachments
(2 files, 3 obsolete files)
3.39 KB,
patch
|
cls
:
review+
|
Details | Diff | Splinter Review |
1.51 KB,
patch
|
cls
:
review+
|
Details | Diff | Splinter Review |
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
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.
Assignee | ||
Comment 2•17 years ago
|
||
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•17 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.
Assignee | ||
Comment 4•17 years ago
|
||
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.
In lieu of a per tree error filter, I think a new error parser for unit tests is fine.
Assignee | ||
Updated•17 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•17 years ago
|
Summary: add ep_unittest.pl to not handle unit test output → add ep_unittest.pl to handle unit test output
Assignee | ||
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
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
Assignee | ||
Comment 8•17 years ago
|
||
justdave said he could apply this locally to tinderbox-stage.
Assignee | ||
Comment 9•17 years ago
|
||
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.
Assignee | ||
Comment 10•17 years ago
|
||
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!
Assignee | ||
Comment 11•17 years ago
|
||
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)
Attachment #279811 -
Flags: review?(cls) → review+
Comment 12•17 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.
Assignee | ||
Comment 13•17 years ago
|
||
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
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•17 years ago
|
||
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•17 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)
Assignee | ||
Comment 16•17 years ago
|
||
Whoops, that was actually the old patch. Sorry.
Attachment #280383 -
Attachment is obsolete: true
Attachment #280411 -
Flags: review?(cls)
Attachment #280411 -
Flags: review?(cls) → review+
Assignee | ||
Comment 17•17 years ago
|
||
Comment on attachment 280411 [details] [diff] [review] add xpcshell unit test support Checked in.
Updated•10 years ago
|
Product: Webtools → Webtools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•