style-queue shouldn't spam bugs if it fails to update its working copy
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Feb 2012 23:59:44 +0000 (23:59 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Feb 2012 23:59:44 +0000 (23:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=79412

Reviewed by Eric Seidel.

This patch moves style-queue over to the new PatchAnalysisTask
infrastructure, which is smarter about retrying patches when update
fails.

* Scripts/webkitpy/common/system/executive_mock.py:
(MockExecutive.run_and_throw_if_fail):
* Scripts/webkitpy/tool/bot/earlywarningsystemtask.py:
* Scripts/webkitpy/tool/bot/patchanalysistask.py:
(UnableToApplyPatch):
(UnableToApplyPatch.__init__):
(PatchAnalysisTask.__init__):
* Scripts/webkitpy/tool/bot/stylequeuetask.py: Added.
(StyleQueueTaskDelegate):
(StyleQueueTaskDelegate.parent_command):
(StyleQueueTask):
(StyleQueueTask.validate):
(StyleQueueTask._check_style):
(StyleQueueTask._apply_watch_list):
(StyleQueueTask.run):
* Scripts/webkitpy/tool/commands/earlywarningsystem.py:
* Scripts/webkitpy/tool/commands/queues.py:
(StyleQueue):
(StyleQueue.should_proceed_with_work_item):
(StyleQueue.review_patch):
(StyleQueue.run_command):
(StyleQueue.command_passed):
(StyleQueue.command_failed):
(StyleQueue.expected_failures):
(StyleQueue.refetch_patch):
* Scripts/webkitpy/tool/commands/queues_unittest.py:
(StyleQueueTest.test_style_queue_with_style_exception):
(test_style_queue_with_watch_list_exception):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@108691 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/common/system/executive_mock.py
Tools/Scripts/webkitpy/tool/bot/earlywarningsystemtask.py
Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py
Tools/Scripts/webkitpy/tool/bot/stylequeuetask.py [new file with mode: 0644]
Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
Tools/Scripts/webkitpy/tool/commands/queues.py
Tools/Scripts/webkitpy/tool/commands/queues_unittest.py

index 83ddf6b..5d5348a 100644 (file)
@@ -1,3 +1,43 @@
+2012-02-23  Adam Barth  <abarth@webkit.org>
+
+        style-queue shouldn't spam bugs if it fails to update its working copy
+        https://bugs.webkit.org/show_bug.cgi?id=79412
+
+        Reviewed by Eric Seidel.
+
+        This patch moves style-queue over to the new PatchAnalysisTask
+        infrastructure, which is smarter about retrying patches when update
+        fails.
+
+        * Scripts/webkitpy/common/system/executive_mock.py:
+        (MockExecutive.run_and_throw_if_fail):
+        * Scripts/webkitpy/tool/bot/earlywarningsystemtask.py:
+        * Scripts/webkitpy/tool/bot/patchanalysistask.py:
+        (UnableToApplyPatch):
+        (UnableToApplyPatch.__init__):
+        (PatchAnalysisTask.__init__):
+        * Scripts/webkitpy/tool/bot/stylequeuetask.py: Added.
+        (StyleQueueTaskDelegate):
+        (StyleQueueTaskDelegate.parent_command):
+        (StyleQueueTask):
+        (StyleQueueTask.validate):
+        (StyleQueueTask._check_style):
+        (StyleQueueTask._apply_watch_list):
+        (StyleQueueTask.run):
+        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
+        * Scripts/webkitpy/tool/commands/queues.py:
+        (StyleQueue):
+        (StyleQueue.should_proceed_with_work_item):
+        (StyleQueue.review_patch):
+        (StyleQueue.run_command):
+        (StyleQueue.command_passed):
+        (StyleQueue.command_failed):
+        (StyleQueue.expected_failures):
+        (StyleQueue.refetch_patch):
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        (StyleQueueTest.test_style_queue_with_style_exception):
+        (test_style_queue_with_watch_list_exception):
+
 2012-02-23  Zoltan Horvath  <zoltan@webkit.org>
 
         [Qt] Allow to use WebCore imagedecoders
index 9ed5048..a762681 100644 (file)
@@ -63,7 +63,7 @@ class MockExecutive(object):
                 env_string = ", env=%s" % env
             log("MOCK run_and_throw_if_fail: %s, cwd=%s%s" % (args, cwd, env_string))
         if self._should_throw_when_run.intersection(args):
-            raise ScriptError("Exception for %s" % args)
+            raise ScriptError("Exception for %s" % args, output="MOCK command output")
         return "MOCK output of child process"
 
     def run_command(self,
index 65a71a7..b66cfbc 100644 (file)
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-from webkitpy.tool.bot.patchanalysistask import PatchAnalysisTask, PatchAnalysisTaskDelegate
-
-
-class UnableToApplyPatch(Exception):
-    def __init__(self, patch):
-        Exception.__init__(self)
-        self.patch = patch
+from webkitpy.tool.bot.patchanalysistask import PatchAnalysisTask, PatchAnalysisTaskDelegate, UnableToApplyPatch
 
 
 class EarlyWarningSystemTaskDelegate(PatchAnalysisTaskDelegate):
index 47e23a1..bcd3d30 100644 (file)
@@ -30,6 +30,12 @@ from webkitpy.common.system.executive import ScriptError
 from webkitpy.common.net.layouttestresults import LayoutTestResults
 
 
+class UnableToApplyPatch(Exception):
+    def __init__(self, patch):
+        Exception.__init__(self)
+        self.patch = patch
+
+
 class PatchAnalysisTaskDelegate(object):
     def parent_command(self):
         raise NotImplementedError("subclasses must implement")
@@ -71,7 +77,6 @@ class PatchAnalysisTask(object):
         self._results_archive_from_patch_test_run = None
         self._results_from_patch_test_run = None
         self._expected_failures = delegate.expected_failures()
-        assert(self._expected_failures)
 
     def _run_command(self, command, success_message, failure_message):
         try:
diff --git a/Tools/Scripts/webkitpy/tool/bot/stylequeuetask.py b/Tools/Scripts/webkitpy/tool/bot/stylequeuetask.py
new file mode 100644 (file)
index 0000000..54d6fa2
--- /dev/null
@@ -0,0 +1,70 @@
+# Copyright (c) 2012 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the name of Google Inc. nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+from webkitpy.tool.bot.patchanalysistask import PatchAnalysisTask, PatchAnalysisTaskDelegate, UnableToApplyPatch
+
+
+class StyleQueueTaskDelegate(PatchAnalysisTaskDelegate):
+    def parent_command(self):
+        return "style-queue"
+
+
+class StyleQueueTask(PatchAnalysisTask):
+    def validate(self):
+        return True
+
+    def _check_style(self):
+        return self._run_command([
+            "check-style",
+            "--no-clean",
+            "--no-update",
+            "--non-interactive",
+            self._patch.id(),
+        ],
+        "ChangeLog validated",
+        "ChangeLog did not pass validation")
+
+    def _apply_watch_list(self):
+        return self._run_command([
+            "apply-watchlist-local",
+            self._patch.bug_id(),
+        ],
+        "ChangeLog validated",
+        "ChangeLog did not pass validation")
+
+    def run(self):
+        if not self._clean():
+            return False
+        if not self._update():
+            return False
+        if not self._apply():
+            raise UnableToApplyPatch(self._patch)
+        self._apply_watch_list()
+        if not self._check_style():
+            return self.report_failure()
+        return True
index c20e452..6f01823 100644 (file)
@@ -32,10 +32,11 @@ from webkitpy.common.config.committers import CommitterList
 from webkitpy.common.config.ports import DeprecatedPort
 from webkitpy.common.system.deprecated_logging import error, log
 from webkitpy.common.system.executive import ScriptError
+from webkitpy.tool.bot.earlywarningsystemtask import EarlyWarningSystemTask, EarlyWarningSystemTaskDelegate
 from webkitpy.tool.bot.expectedfailures import ExpectedFailures
 from webkitpy.tool.bot.layouttestresultsreader import LayoutTestResultsReader
+from webkitpy.tool.bot.patchanalysistask import UnableToApplyPatch
 from webkitpy.tool.bot.queueengine import QueueEngine
-from webkitpy.tool.bot.earlywarningsystemtask import EarlyWarningSystemTask, EarlyWarningSystemTaskDelegate, UnableToApplyPatch
 from webkitpy.tool.commands.queues import AbstractReviewQueue
 
 
index ed85178..4edccd4 100644 (file)
@@ -46,9 +46,11 @@ from webkitpy.tool.bot.botinfo import BotInfo
 from webkitpy.tool.bot.commitqueuetask import CommitQueueTask, CommitQueueTaskDelegate
 from webkitpy.tool.bot.expectedfailures import ExpectedFailures
 from webkitpy.tool.bot.feeders import CommitQueueFeeder, EWSFeeder
+from webkitpy.tool.bot.flakytestreporter import FlakyTestReporter
 from webkitpy.tool.bot.layouttestresultsreader import LayoutTestResultsReader
+from webkitpy.tool.bot.patchanalysistask import UnableToApplyPatch
 from webkitpy.tool.bot.queueengine import QueueEngine, QueueEngineDelegate
-from webkitpy.tool.bot.flakytestreporter import FlakyTestReporter
+from webkitpy.tool.bot.stylequeuetask import StyleQueueTask, StyleQueueTaskDelegate
 from webkitpy.tool.commands.stepsequence import StepSequenceErrorHandler
 from webkitpy.tool.multicommandtool import Command, TryAgain
 
@@ -408,35 +410,43 @@ class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler):
         log(script_error.message_with_output())
 
 
-class StyleQueue(AbstractReviewQueue):
+class StyleQueue(AbstractReviewQueue, StyleQueueTaskDelegate):
     name = "style-queue"
+
     def __init__(self):
         AbstractReviewQueue.__init__(self)
 
     def should_proceed_with_work_item(self, patch):
-        self._update_status("Checking style", patch)
         return True
 
     def review_patch(self, patch):
+        task = StyleQueueTask(self, patch)
         try:
-            # Run the style checks.
-            self.run_webkit_patch(["check-style", "--force-clean", "--non-interactive", "--parent-command=style-queue", patch.id()])
-        finally:
-            # Apply the watch list.
-            try:
-                self.run_webkit_patch(["apply-watchlist-local", patch.bug_id()])
-            except ScriptError, e:
-                # Don't turn the style bot block red due to watchlist errors.
-                pass
-
+            return task.run()
+        except UnableToApplyPatch, e:
+            self._did_error(patch, "%s unable to apply patch." % self.name)
+            return False
+        except ScriptError, e:
+            message = "Attachment %s did not pass %s:\n\n%s\n\nIf any of these errors are false positives, please file a bug against check-webkit-style." % (patch.id(), self.name, e.output)
+            self._tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=self.watchers)
+            self._did_fail(patch)
+            return False
         return True
 
-    @classmethod
-    def handle_script_error(cls, tool, state, script_error):
-        is_svn_apply = script_error.command_name() == "svn-apply"
-        status_id = cls._update_status_for_script_error(tool, state, script_error, is_error=is_svn_apply)
-        if is_svn_apply:
-            QueueEngine.exit_after_handled_error(script_error)
-        message = "Attachment %s did not pass %s:\n\n%s\n\nIf any of these errors are false positives, please file a bug against check-webkit-style." % (state["patch"].id(), cls.name, script_error.message_with_output(output_limit=3*1024))
-        tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
-        sys.exit(1)
+    # StyleQueueTaskDelegate methods
+
+    def run_command(self, command):
+        self.run_webkit_patch(command)
+
+    def command_passed(self, message, patch):
+        self._update_status(message, patch=patch)
+
+    def command_failed(self, message, script_error, patch):
+        failure_log = self._log_from_script_error_for_upload(script_error)
+        return self._update_status(message, patch=patch, results_file=failure_log)
+
+    def expected_failures(self):
+        return None
+
+    def refetch_patch(self, patch):
+        return self._tool.bugs.fetch_attachment(patch.id())
index 8f8f198..d723ac6 100644 (file)
@@ -448,35 +448,55 @@ class StyleQueueTest(QueuesTest):
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("style-queue"),
             "next_work_item": "",
-            "should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n",
-            "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'check-style', '--force-clean', '--non-interactive', '--parent-command=style-queue', 10000], cwd=/mock-checkout
+            "should_proceed_with_work_item": "",
+            "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'clean'], cwd=/mock-checkout
+MOCK: update_status: style-queue Cleaned working directory
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'update'], cwd=/mock-checkout
+MOCK: update_status: style-queue Updated working directory
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--no-update', '--non-interactive', 10000], cwd=/mock-checkout
+MOCK: update_status: style-queue Applied patch
 MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-watchlist-local', 50000], cwd=/mock-checkout
+MOCK: update_status: style-queue ChangeLog validated
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'check-style', '--no-clean', '--no-update', '--non-interactive', 10000], cwd=/mock-checkout
+MOCK: update_status: style-queue ChangeLog did not pass validation
+MOCK bug comment: bug_id=50000, cc=[]
+--- Begin comment ---
+Attachment 10000 did not pass style-queue:
+
+MOCK command output
+
+If any of these errors are false positives, please file a bug against check-webkit-style.
+--- End comment ---
+
 MOCK: update_status: style-queue Fail
-MOCK: release_work_item: style-queue 10000\n""",
+MOCK: release_work_item: style-queue 10000
+""",
             "handle_unexpected_error": "Mock error message\n",
-            "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=50000, cc=[]\n--- Begin comment ---\nAttachment 10000 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n",
-        }
-        expected_exceptions = {
-            "process_work_item": ScriptError,
-            "handle_script_error": SystemExit,
+            "handle_script_error": "ScriptError error message\n",
         }
         tool = MockTool(log_executive=True, executive_throws_when_run=set(['check-style']))
-        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, expected_exceptions=expected_exceptions, tool=tool)
+        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, tool=tool)
 
     def test_style_queue_with_watch_list_exception(self):
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("style-queue"),
             "next_work_item": "",
-            "should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n",
-            "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'check-style', '--force-clean', '--non-interactive', '--parent-command=style-queue', 10000], cwd=/mock-checkout
+            "should_proceed_with_work_item": "",
+            "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'clean'], cwd=/mock-checkout
+MOCK: update_status: style-queue Cleaned working directory
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'update'], cwd=/mock-checkout
+MOCK: update_status: style-queue Updated working directory
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--no-update', '--non-interactive', 10000], cwd=/mock-checkout
+MOCK: update_status: style-queue Applied patch
 MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-watchlist-local', 50000], cwd=/mock-checkout
+MOCK: update_status: style-queue ChangeLog did not pass validation
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'check-style', '--no-clean', '--no-update', '--non-interactive', 10000], cwd=/mock-checkout
+MOCK: update_status: style-queue ChangeLog validated
 MOCK: update_status: style-queue Pass
-MOCK: release_work_item: style-queue 10000\n""",
+MOCK: release_work_item: style-queue 10000
+""",
             "handle_unexpected_error": "Mock error message\n",
-            "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=50000, cc=[]\n--- Begin comment ---\nAttachment 10000 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n",
-        }
-        expected_exceptions = {
-            "handle_script_error": SystemExit,
+            "handle_script_error": "ScriptError error message\n",
         }
         tool = MockTool(log_executive=True, executive_throws_when_run=set(['apply-watchlist-local']))
-        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, expected_exceptions=expected_exceptions, tool=tool)
+        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, tool=tool)