[Webkit-unassigned] [Bug 169308] Add support for Bindings EWS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=169308

--- Comment #7 from Sri Vijayaraghavan <svijayaraghavan at apple.com> ---
(In reply to comment #6)
> Comment on attachment 303745 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303745&action=review
> 
> overall looks good to me. Few comments inline.
> 
> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:223
> > +class MockBindingsEarlyWarningSystem(AbstractEarlyWarningSystem):
> 
> The filename is jscews_unittest.py, should either rename the file or move
> the binding test code to a new file.
Haha yes, I'll rename this to retrylogic_unittest.py

> > Tools/Scripts/webkitpy/tool/bot/jscews_unittest.py:260
> > +        with self.assertRaises(ScriptError):
> 
> why do we need this? If it is required, shouldn't it be required in all the
> similar failure test cases below (e.g.: test_ineffective_patch)?
Patches that have the same failures as the clean tree are not considered to be EWS failures.

On Line 53 of this file, a ScriptError object is created (note: not raised!). This mimicks the behavior in patchanalysistask.py.

It is only raised later if the patch introduces new test failures that weren't present in the tree (not if the failures in the patch already existed in the tree).

> > Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:194
> > +        if hasattr(self._delegate, 'group') and self._delegate.should_build:
> 
> do we need to check for 'group' here. 
> Also does all the delegates have should_build? I think it should be fine,
> but please verify for CommitQueueTaskDelegate and StyleQueueTaskDelegate.
My bad, I actually meant to check for hasattr(self._delegate, 'should_build') here. I'll change this conditional to always append the --build flag, unless a should_build attribute is present and valued False.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170309/93db2fe8/attachment-0001.html>