Bug 169308 – Add support for Bindings EWS

• Component: New Bugs
• Status: RESOLVED FIXED

[Srinivasan Vijayaraghavan]

Add support for Bindings EWS

[Srinivasan Vijayaraghavan]

Created attachment 303743 [details]
Patch

[WebKit Commit Bot]

Attachment 303743 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:94:  [AbstractEarlyWarningSystem._create_task] Instance of 'AbstractEarlyWarningSystem' has no 'should_build' member  [pylint/E1101] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

(In reply to comment #2)
> Attachment 303743 [details] did not pass style-queue:
> 
> 
> ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:94: 
> [AbstractEarlyWarningSystem._create_task] Instance of
> 'AbstractEarlyWarningSystem' has no 'should_build' member  [pylint/E1101] [5]
> Total errors found: 1 in 11 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This is a false positive, similar to the error for '_group' on 162458 .

[Srinivasan Vijayaraghavan]

Created attachment 303745 [details]
Patch

[WebKit Commit Bot]

Attachment 303745 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:94:  [AbstractEarlyWarningSystem._create_task] Instance of 'AbstractEarlyWarningSystem' has no 'should_build' member  [pylint/E1101] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Aakash Jain]

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.

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

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

[Srinivasan Vijayaraghavan]

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

[Srinivasan Vijayaraghavan]

Created attachment 304001 [details]
Patch

[WebKit Commit Bot]

Attachment 304001 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:94:  [AbstractEarlyWarningSystem._create_task] Instance of 'AbstractEarlyWarningSystem' has no 'should_build' member  [pylint/E1101] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 304005 [details]
Patch

[WebKit Commit Bot]

Attachment 304005 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:94:  [AbstractEarlyWarningSystem._create_task] Instance of 'AbstractEarlyWarningSystem' has no 'should_build' member  [pylint/E1101] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 304005 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304005&action=review

> Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py:194
> +        if not hasattr(self._delegate, 'should_build') or self._delegate.should_build:

Can you use getattr with a default value here?

> Tools/Scripts/webkitpy/tool/bot/retrylogic_unittest.py:1
> +# Copyright (C) 2017 Apple Inc. All rights reserved.

Looks like the file was not moved, but re-added, thus breaking source history. Please re-do the same change, but using a move.

> Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:94
> +        return EarlyWarningSystemTask(self, patch, self._options.run_tests, self.should_build)

Whenever we use multiple boolean arguments, we should really use named arguments instead.

In WebKit C++ code, we do a similar thing by creating distinct types for boolean arguments, that way it's impossible to mix up the order.

[Srinivasan Vijayaraghavan]

Created attachment 304015 [details]
Patch

[WebKit Commit Bot]

Attachment 304015 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:94:  [AbstractEarlyWarningSystem._create_task] Instance of 'AbstractEarlyWarningSystem' has no 'should_build' member  [pylint/E1101] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Srinivasan Vijayaraghavan]

Created attachment 304054 [details]
Patch

[WebKit Commit Bot]

Attachment 304054 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py:94:  [AbstractEarlyWarningSystem._create_task] Instance of 'AbstractEarlyWarningSystem' has no 'should_build' member  [pylint/E1101] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 304054 [details]
Patch

Clearing flags on attachment: 304054

Committed r213722: <http://trac.webkit.org/changeset/213722>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.