Bug 169308

Created attachment 303743 [details] Patch

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.

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

Created attachment 303745 [details] Patch

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

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

Created attachment 304001 [details] Patch

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.

Created attachment 304005 [details] Patch

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

Created attachment 304015 [details] Patch

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.

Created attachment 304054 [details] Patch

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 on attachment 304054 [details] Patch Clearing flags on attachment: 304054 Committed r213722: <http://trac.webkit.org/changeset/213722>

All reviewed patches have been landed. Closing bug.