style-queue tries to apply the patch twice

https://bugs.webkit.org/show_bug.cgi?id=79459

Reviewed by Eric Seidel.

Previously, we applied the patch using both apply-patch and
check-style.  This patch introduces a check-style-local to mirror the
apply-watchlist-local, which operates on the current working diff.

This patch also cleans up some other bugs I found by running the queue
locally for a while.  The queue still prints out a slightly
less-than-ideal message on the bugs when it find an error, but it's ok
for now.  We'll need to iterate a bit.

* Scripts/webkitpy/tool/bot/stylequeuetask.py:
(StyleQueueTask.validate):
(StyleQueueTask._check_style):
* Scripts/webkitpy/tool/commands/download.py:
(CheckStyleLocal):
* Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
(EarlyWarningSytemTest._default_expected_stderr):
(EarlyWarningSytemTest._test_testing_ews):
* Scripts/webkitpy/tool/commands/queues.py:
(AbstractReviewQueue.handle_script_error):
(StyleQueue.review_patch):
* Scripts/webkitpy/tool/commands/queues_unittest.py:
* Scripts/webkitpy/tool/commands/queuestest.py:
(QueuesTest.assert_queue_outputs):

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

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
index 887333c..78a3ff0 100644 (file)
--- a/Tools/ChangeLog
+++ b/Tools/ChangeLog
@@ -1,3 +1,34 @@
+2012-02-24  Adam Barth  <abarth@webkit.org>
+
+        style-queue tries to apply the patch twice
+        https://bugs.webkit.org/show_bug.cgi?id=79459
+
+        Reviewed by Eric Seidel.
+
+        Previously, we applied the patch using both apply-patch and
+        check-style.  This patch introduces a check-style-local to mirror the
+        apply-watchlist-local, which operates on the current working diff.
+
+        This patch also cleans up some other bugs I found by running the queue
+        locally for a while.  The queue still prints out a slightly
+        less-than-ideal message on the bugs when it find an error, but it's ok
+        for now.  We'll need to iterate a bit.
+
+        * Scripts/webkitpy/tool/bot/stylequeuetask.py:
+        (StyleQueueTask.validate):
+        (StyleQueueTask._check_style):
+        * Scripts/webkitpy/tool/commands/download.py:
+        (CheckStyleLocal):
+        * Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
+        (EarlyWarningSytemTest._default_expected_stderr):
+        (EarlyWarningSytemTest._test_testing_ews):
+        * Scripts/webkitpy/tool/commands/queues.py:
+        (AbstractReviewQueue.handle_script_error):
+        (StyleQueue.review_patch):
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        * Scripts/webkitpy/tool/commands/queuestest.py:
+        (QueuesTest.assert_queue_outputs):
+
 2012-02-23  Adam Barth  <abarth@webkit.org>
 
         should_proceed_with_work_item is unused and can be removed

diff --git a/Tools/Scripts/webkitpy/tool/bot/stylequeuetask.py b/Tools/Scripts/webkitpy/tool/bot/stylequeuetask.py
index 261e677..01f7f72 100644 (file)
--- a/Tools/Scripts/webkitpy/tool/bot/stylequeuetask.py
+++ b/Tools/Scripts/webkitpy/tool/bot/stylequeuetask.py
@@ -36,15 +36,20 @@ class StyleQueueTaskDelegate(PatchAnalysisTaskDelegate):
 
 class StyleQueueTask(PatchAnalysisTask):
     def validate(self):
+        self._patch = self._delegate.refetch_patch(self._patch)
+        if self._patch.is_obsolete():
+            return False
+        if self._patch.bug().is_closed():
+            return False
+        if self._patch.review() == "-":
+            return False
         return True
 
     def _check_style(self):
         return self._run_command([
-            "check-style",
-            "--no-clean",
-            "--no-update",
+            "check-style-local",
             "--non-interactive",
-            self._patch.id(),
+            "--quiet",
         ],
         "Style checked",
         "Patch did not pass style check")

diff --git a/Tools/Scripts/webkitpy/tool/commands/download.py b/Tools/Scripts/webkitpy/tool/commands/download.py
index afdf784..611ca92 100644 (file)
--- a/Tools/Scripts/webkitpy/tool/commands/download.py
+++ b/Tools/Scripts/webkitpy/tool/commands/download.py
@@ -128,6 +128,14 @@ class LandCowboy(AbstractSequencedCommand):
         options.check_style_filter = "-changelog"
 
 
+class CheckStyleLocal(AbstractSequencedCommand):
+    name = "check-style-local"
+    help_text = "Run check-webkit-style on the current working directory diff"
+    steps = [
+        steps.CheckStyle,
+    ]
+
+
 class AbstractPatchProcessingCommand(AbstractDeclarativeCommand):
     # Subclasses must implement the methods below.  We don't declare them here
     # because we want to be able to implement them with mix-ins.

diff --git a/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py b/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
index 24dcf88..cb34941 100644 (file)
--- a/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
+++ b/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
@@ -61,7 +61,7 @@ class EarlyWarningSytemTest(QueuesTest):
             "handle_unexpected_error": "Mock error message\n",
             "next_work_item": "",
             "process_work_item": "MOCK: update_status: %(name)s Pass\nMOCK: release_work_item: %(name)s 10000\n" % string_replacemnts,
-            "handle_script_error": "ScriptError error message\n",
+            "handle_script_error": "ScriptError error message\n\nMOCK output\n",
         }
         return expected_stderr
 
@@ -76,7 +76,7 @@ class EarlyWarningSytemTest(QueuesTest):
         ews.layout_test_results = lambda: None
         ews.bind_to_tool(MockTool())
         expected_stderr = self._default_expected_stderr(ews)
-        expected_stderr["handle_script_error"] = "ScriptError error message\n"
+        expected_stderr["handle_script_error"] = "ScriptError error message\n\nMOCK output\n"
         self.assert_queue_outputs(ews, expected_stderr=expected_stderr)
 
     def test_builder_ewses(self):

diff --git a/Tools/Scripts/webkitpy/tool/commands/queues.py b/Tools/Scripts/webkitpy/tool/commands/queues.py
index fc5a832..18ee4cb 100644 (file)
--- a/Tools/Scripts/webkitpy/tool/commands/queues.py
+++ b/Tools/Scripts/webkitpy/tool/commands/queues.py
@@ -393,7 +393,7 @@ class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler):
 
     @classmethod
     def handle_script_error(cls, tool, state, script_error):
-        log(script_error.message_with_output())
+        log(script_error.output)
 
 
 class StyleQueue(AbstractReviewQueue, StyleQueueTaskDelegate):
@@ -404,6 +404,9 @@ class StyleQueue(AbstractReviewQueue, StyleQueueTaskDelegate):
 
     def review_patch(self, patch):
         task = StyleQueueTask(self, patch)
+        if not task.validate():
+            self._did_error(patch, "%s did not process patch." % self.name)
+            return False
         try:
             return task.run()
         except UnableToApplyPatch, e:

diff --git a/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py b/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py
index 79d970f..f2c60d9 100644 (file)
--- a/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py
+++ b/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py
@@ -246,7 +246,7 @@ MOCK: update_status: commit-queue Pass
 MOCK: release_work_item: commit-queue 10000
 """,
             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10000' with comment 'Rejecting attachment 10000 from commit-queue.' and additional comment 'Mock error message'\n",
-            "handle_script_error": "ScriptError error message\n",
+            "handle_script_error": "ScriptError error message\n\nMOCK output\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr)
 
@@ -263,7 +263,7 @@ MOCK: update_status: commit-queue Fail
 MOCK: release_work_item: commit-queue 10000
 """,
             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10000' with comment 'Rejecting attachment 10000 from commit-queue.' and additional comment 'Mock error message'\n",
-            "handle_script_error": "ScriptError error message\n",
+            "handle_script_error": "ScriptError error message\n\nMOCK output\n",
         }
         queue = CommitQueue()
 
@@ -292,7 +292,7 @@ MOCK: update_status: commit-queue Fail
 MOCK: release_work_item: commit-queue 10000
 """,
             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10000' with comment 'Rejecting attachment 10000 from commit-queue.' and additional comment 'Mock error message'\n",
-            "handle_script_error": "ScriptError error message\n",
+            "handle_script_error": "ScriptError error message\n\nMOCK output\n",
         }
         queue = CommitQueue()
 
@@ -332,7 +332,7 @@ MOCK: update_status: commit-queue Pass
 MOCK: release_work_item: commit-queue 10000
 """,
             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10000' with comment 'Rejecting attachment 10000 from commit-queue.' and additional comment 'Mock error message'\n",
-            "handle_script_error": "ScriptError error message\n",
+            "handle_script_error": "ScriptError error message\n\nMOCK output\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr)
 
@@ -358,7 +358,7 @@ MOCK: update_status: commit-queue Pass
 MOCK: release_work_item: commit-queue 10005
 """,
             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '10005' with comment 'Rejecting attachment 10005 from commit-queue.' and additional comment 'Mock error message'\n",
-            "handle_script_error": "ScriptError error message\n",
+            "handle_script_error": "ScriptError error message\n\nMOCK output\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr)
 
@@ -450,22 +450,13 @@ MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachm
 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 Watchlist applied
-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 Patch did not pass style check
-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 run_and_throw_if_fail: ['echo', '--status-host=example.com', 'check-style-local', '--non-interactive', '--quiet'], cwd=/mock-checkout
+MOCK: update_status: style-queue Style checked
+MOCK: update_status: style-queue Pass
 MOCK: release_work_item: style-queue 10000
 """,
             "handle_unexpected_error": "Mock error message\n",
-            "handle_script_error": "ScriptError error message\n",
+            "handle_script_error": "MOCK output\n",
         }
         tool = MockTool(log_executive=True, executive_throws_when_run=set(['check-style']))
         self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, tool=tool)
@@ -482,13 +473,13 @@ MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachm
 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 Unabled to apply watchlist
-MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'check-style', '--no-clean', '--no-update', '--non-interactive', 10000], cwd=/mock-checkout
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'check-style-local', '--non-interactive', '--quiet'], cwd=/mock-checkout
 MOCK: update_status: style-queue Style checked
 MOCK: update_status: style-queue Pass
 MOCK: release_work_item: style-queue 10000
 """,
             "handle_unexpected_error": "Mock error message\n",
-            "handle_script_error": "ScriptError error message\n",
+            "handle_script_error": "MOCK output\n",
         }
         tool = MockTool(log_executive=True, executive_throws_when_run=set(['apply-watchlist-local']))
         self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, tool=tool)

diff --git a/Tools/Scripts/webkitpy/tool/commands/queuestest.py b/Tools/Scripts/webkitpy/tool/commands/queuestest.py
index dd8d4a4..b99302c 100644 (file)
--- a/Tools/Scripts/webkitpy/tool/commands/queuestest.py
+++ b/Tools/Scripts/webkitpy/tool/commands/queuestest.py
@@ -95,4 +95,4 @@ class QueuesTest(unittest.TestCase):
         self.assert_outputs(queue.handle_unexpected_error, "handle_unexpected_error", [work_item, "Mock error message"], expected_stdout, expected_stderr, expected_exceptions)
         # Should we have a different function for testing StepSequenceErrorHandlers?
         if isinstance(queue, StepSequenceErrorHandler):
-            self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": self.mock_work_item}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions)
+            self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": self.mock_work_item}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand", output="MOCK output")], expected_stdout, expected_stderr, expected_exceptions)