<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Add support for Bindings EWS"
href="https://bugs.webkit.org/show_bug.cgi?id=169308#c7">Comment # 7</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Add support for Bindings EWS"
href="https://bugs.webkit.org/show_bug.cgi?id=169308">bug 169308</a>
from <span class="vcard"><a class="email" href="mailto:svijayaraghavan@apple.com" title="Sri Vijayaraghavan <svijayaraghavan@apple.com>"> <span class="fn">Sri Vijayaraghavan</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=169308#c6">comment #6</a>)
<span class="quote">> Comment on <span class=""><a href="attachment.cgi?id=303745&action=diff" name="attach_303745" title="Patch">attachment 303745</a> <a href="attachment.cgi?id=303745&action=edit" title="Patch">[details]</a></span>
> Patch
>
> View in context:
> <a href="https://bugs.webkit.org/attachment.cgi?id=303745&action=review">https://bugs.webkit.org/attachment.cgi?id=303745&action=review</a>
>
> 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.</span >
Haha yes, I'll rename this to retrylogic_unittest.py
<span class="quote">> > 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)?</span >
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).
<span class="quote">> > 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.</span >
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.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>