Bug 83329 – Enable webkit_unit_tests for commit queue and EWS while tracking failures

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Enable webkit_unit_tests for commit queue and EWS while tracking failures

[James Robinson]

Created attachment 135937 [details]
Patch

[James Robinson]

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).

[Adam Barth]

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.

[James Robinson]

Created attachment 136065 [details]
Patch

[James Robinson]

(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)

[Eric Seidel (no email)]

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.

[James Robinson]

(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.

[Adam Barth]

> 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.  :)

[Adam Barth]

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.

[James Robinson]

Committed r113499: <http://trac.webkit.org/changeset/113499>

[Eric Seidel (no email)]

Reopening to attach new patch.

[Eric Seidel (no email)]

Created attachment 136194 [details]
Patch for landing

[Eric Seidel (no email)]

Comment on attachment 136194 [details]
Patch for landing

Clearing flags on attachment: 136194

Committed r113559: <http://trac.webkit.org/changeset/113559>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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. :(

[Eric Seidel (no email)]

Yup. :)  The CrLinux EWS says:
Last Pass: 2 days, 9 hours ago
:(

[James Robinson]

And it's rejecting patches that break unit tests: https://bugs.webkit.org/show_bug.cgi?id=81227#c16

Yay!  Thanks for the help.