Bug 169308

Summary: Add support for Bindings EWS
Product: WebKit Reporter: Srinivasan Vijayaraghavan <webkit>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, commit-queue, glenn
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=169307
https://bugs.webkit.org/show_bug.cgi?id=168979
Bug Depends on: 169305    
Bug Blocks: 169307    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Srinivasan Vijayaraghavan 2017-03-07 14:26:48 PST
Add support for Bindings EWS
Comment 1 Srinivasan Vijayaraghavan 2017-03-07 17:09:01 PST
Created attachment 303743 [details]
Patch
Comment 2 WebKit Commit Bot 2017-03-07 17:11:13 PST
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.
Comment 3 Srinivasan Vijayaraghavan 2017-03-07 17:14:08 PST
(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 .
Comment 4 Srinivasan Vijayaraghavan 2017-03-07 17:19:50 PST
Created attachment 303745 [details]
Patch
Comment 5 WebKit Commit Bot 2017-03-07 17:22:49 PST
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.
Comment 6 Aakash Jain 2017-03-09 12:06:17 PST
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.
Comment 7 Srinivasan Vijayaraghavan 2017-03-09 12:38:14 PST
(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.
Comment 8 Srinivasan Vijayaraghavan 2017-03-09 15:29:43 PST
Created attachment 304001 [details]
Patch
Comment 9 WebKit Commit Bot 2017-03-09 15:32:15 PST
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.
Comment 10 Srinivasan Vijayaraghavan 2017-03-09 16:23:05 PST
Created attachment 304005 [details]
Patch
Comment 11 WebKit Commit Bot 2017-03-09 16:25:49 PST
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.
Comment 12 Alexey Proskuryakov 2017-03-09 16:40:38 PST
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.
Comment 13 Srinivasan Vijayaraghavan 2017-03-09 17:29:36 PST
Created attachment 304015 [details]
Patch
Comment 14 WebKit Commit Bot 2017-03-09 17:31:56 PST
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.
Comment 15 Srinivasan Vijayaraghavan 2017-03-10 10:42:47 PST
Created attachment 304054 [details]
Patch
Comment 16 WebKit Commit Bot 2017-03-10 10:45:03 PST
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.
Comment 17 WebKit Commit Bot 2017-03-10 11:24:50 PST
Comment on attachment 304054 [details]
Patch

Clearing flags on attachment: 304054

Committed r213722: <http://trac.webkit.org/changeset/213722>
Comment 18 WebKit Commit Bot 2017-03-10 11:24:57 PST
All reviewed patches have been landed.  Closing bug.