Bug 83329 - Enable webkit_unit_tests for commit queue and EWS while tracking failures
Summary: Enable webkit_unit_tests for commit queue and EWS while tracking failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-05 16:49 PDT by James Robinson
Modified: 2012-04-09 10:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (37.64 KB, patch)
2012-04-05 16:51 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (38.35 KB, patch)
2012-04-06 14:15 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch for landing (3.38 KB, patch)
2012-04-09 00:46 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-04-05 16:49:42 PDT
Enable webkit_unit_tests for commit queue and EWS while tracking failures
Comment 1 James Robinson 2012-04-05 16:51:21 PDT
Created attachment 135937 [details]
Patch
Comment 2 James Robinson 2012-04-05 16:53:11 PDT
This is a bit hacky - the point at which I gave up and just hacked it in was putting the failures in LayoutTestResults directly instead of creating another object to represent the union of different test failure types. I think it's still a valid incremental step.  This appears to do the right thing based on running the unit tests and webkit-patch build-and-test, but I'm not sure how to test what it really does in EWS or the commit queue.

If this is too big, I can attempt to post the intermediate patches on my github (if someone can help me figure it out).
Comment 3 Adam Barth 2012-04-05 21:18:02 PDT
Comment on attachment 135937 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135937&action=review

This looks fine.  I think we're missing a few error handling cases for ports that don't have unit tests and if the unit test file is broken / corrupt / etc.  There's also some amount of renaming that we should do to remove "layout" from a bunch of names, but we can deal with that in a later patch.

Thanks for taking this patch this far.  I can polish up the details tomorrow if you like.

> Tools/Scripts/webkitpy/common/net/unittestresults.py:29
> +import xml.dom.minidom

This comes with Python by default?

> Tools/Scripts/webkitpy/common/net/unittestresults.py:31
> +from webkitpy.common.system.deprecated_logging import log

Yay deprecated_logging :>)

> Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader.py:56
> +        results_path = self._tool.port().unit_tests_results_path()

Should we null check results_path (i.e., for ports that don't have unit tests)?

> Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader.py:71
> +        if layout_test_results:

what if unit_test_results is None ?  This function seems a bit confused.
Comment 4 James Robinson 2012-04-06 14:15:14 PDT
Created attachment 136065 [details]
Patch
Comment 5 James Robinson 2012-04-06 14:15:54 PDT
(In reply to comment #3)
> (From update of attachment 135937 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135937&action=review
> 
> This looks fine.  I think we're missing a few error handling cases for ports that don't have unit tests and if the unit test file is broken / corrupt / etc.  There's also some amount of renaming that we should do to remove "layout" from a bunch of names, but we can deal with that in a later patch.
> 
> Thanks for taking this patch this far.  I can polish up the details tomorrow if you like.
> 
> > Tools/Scripts/webkitpy/common/net/unittestresults.py:29
> > +import xml.dom.minidom
> 
> This comes with Python by default?
> 

Various other scripts are using it and it's in http://docs.python.org/release/2.6.6/library/index.html, so I guess so?

> > Tools/Scripts/webkitpy/common/net/unittestresults.py:31
> > +from webkitpy.common.system.deprecated_logging import log
> 
> Yay deprecated_logging :>)

If there's something better let me know what it is and I'll use it.

> 
> > Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader.py:56
> > +        results_path = self._tool.port().unit_tests_results_path()
> 
> Should we null check results_path (i.e., for ports that don't have unit tests)?
> 
> > Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader.py:71
> > +        if layout_test_results:
> 
> what if unit_test_results is None ?  This function seems a bit confused.

Fixed this up in the most recent patch (with test coverage for path==None)
Comment 6 Eric Seidel (no email) 2012-04-06 14:20:10 PDT
Comment on attachment 136065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136065&action=review

LGTM, but you should have dr. barth give you the final approval since he's been workign with you on this.

> Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader.py:61
> +        if not results_xml:
> +            return None

Seems this check isn't needed.

> Tools/Scripts/webkitpy/tool/steps/runtests.py:73
> +                args.append("--gtest_output=xml:%s/webkit_unit_tests_output.xml" % self._tool.port().results_directory)

Really sad it can't just output to stdout.
Comment 7 James Robinson 2012-04-06 14:24:47 PDT
(In reply to comment #6)
> (From update of attachment 136065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136065&action=review
> 
> LGTM, but you should have dr. barth give you the final approval since he's been workign with you on this.
> 
> > Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader.py:61
> > +        if not results_xml:
> > +            return None
> 
> Seems this check isn't needed.

I think it is if there's a valid path but we can't read the file for whatever reason (I/O error or whatnot)

> 
> > Tools/Scripts/webkitpy/tool/steps/runtests.py:73
> > +                args.append("--gtest_output=xml:%s/webkit_unit_tests_output.xml" % self._tool.port().results_directory)
> 
> Really sad it can't just output to stdout.

It outputs a human-readable log to stdout, which is useful because it includes log messages and printf()s and is a lot easier to scan than the XML.  I like having both available.
Comment 8 Adam Barth 2012-04-06 14:34:00 PDT
> If there's something better let me know what it is and I'll use it.

In principle, we should switch everything to regular python logging, but that's way outside the scope of this patch.  :)
Comment 9 Adam Barth 2012-04-06 14:37:59 PDT
Comment on attachment 136065 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136065&action=review

Ok.  Let's give this a try.  :)

> Tools/Scripts/webkitpy/tool/bot/layouttestresultsreader.py:73
> +        unit_test_results = self._create_unit_test_results()
> +        if layout_test_results and unit_test_results:

I would have just nested this inside the above if since there isn't much point in calling _create_unit_test_results if layout_test_results is falsy.
Comment 10 James Robinson 2012-04-06 14:46:06 PDT
Committed r113499: <http://trac.webkit.org/changeset/113499>
Comment 11 Eric Seidel (no email) 2012-04-09 00:45:58 PDT
Reopening to attach new patch.
Comment 12 Eric Seidel (no email) 2012-04-09 00:46:01 PDT
Created attachment 136194 [details]
Patch for landing
Comment 13 Eric Seidel (no email) 2012-04-09 00:55:19 PDT
Comment on attachment 136194 [details]
Patch for landing

Clearing flags on attachment: 136194

Committed r113559: <http://trac.webkit.org/changeset/113559>
Comment 14 Eric Seidel (no email) 2012-04-09 00:55:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Eric Seidel (no email) 2012-04-09 01:06:26 PDT
Hopefully the queues should be back on line now.  This sadly took out the Chromium EWS and commit-queues for most of the last 3 days. :(
Comment 16 Eric Seidel (no email) 2012-04-09 01:10:45 PDT
Yup. :)  The CrLinux EWS says:
Last Pass: 2 days, 9 hours ago
:(
Comment 17 James Robinson 2012-04-09 10:24:24 PDT
And it's rejecting patches that break unit tests: https://bugs.webkit.org/show_bug.cgi?id=81227#c16

Yay!  Thanks for the help.