Summary: | Enable webkit_unit_tests for commit queue and EWS while tracking failures | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||||
Component: | New Bugs | Assignee: | James Robinson <jamesr> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, dpranke, enne, eric, ojan, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
James Robinson
2012-04-05 16:49:42 PDT
Created attachment 135937 [details]
Patch
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 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. Created attachment 136065 [details]
Patch
(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 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. (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. > 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 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. Committed r113499: <http://trac.webkit.org/changeset/113499> Reopening to attach new patch. Created attachment 136194 [details]
Patch for landing
Comment on attachment 136194 [details] Patch for landing Clearing flags on attachment: 136194 Committed r113559: <http://trac.webkit.org/changeset/113559> All reviewed patches have been landed. Closing bug. 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. :( Yup. :) The CrLinux EWS says: Last Pass: 2 days, 9 hours ago :( And it's rejecting patches that break unit tests: https://bugs.webkit.org/show_bug.cgi?id=81227#c16 Yay! Thanks for the help. |