[webkit-reviews] review denied: [Bug 83329] Enable webkit_unit_tests for commit queue and EWS while tracking failures : [Attachment 135937] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied James Robinson
<jamesr at chromium.org>'s request for review:
Bug 83329: Enable webkit_unit_tests for commit queue and EWS while tracking
failures
https://bugs.webkit.org/show_bug.cgi?id=83329

Attachment 135937: Patch
https://bugs.webkit.org/attachment.cgi?id=135937&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.