From 6cd15afffad4691dde42dee2a99c80f8fd2735c8 Mon Sep 17 00:00:00 2001 From: "abarth@webkit.org" Date: Fri, 24 Feb 2012 08:33:50 +0000 Subject: [PATCH] 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 --- Tools/ChangeLog | 31 ++++++++++++++++++++++ Tools/Scripts/webkitpy/tool/bot/stylequeuetask.py | 13 ++++++--- Tools/Scripts/webkitpy/tool/commands/download.py | 8 ++++++ .../tool/commands/earlywarningsystem_unittest.py | 4 +-- Tools/Scripts/webkitpy/tool/commands/queues.py | 5 +++- .../webkitpy/tool/commands/queues_unittest.py | 31 ++++++++-------------- Tools/Scripts/webkitpy/tool/commands/queuestest.py | 2 +- 7 files changed, 66 insertions(+), 28 deletions(-) diff --git a/Tools/ChangeLog b/Tools/ChangeLog index 887333c..78a3ff0 100644 --- a/Tools/ChangeLog +++ b/Tools/ChangeLog @@ -1,3 +1,34 @@ +2012-02-24 Adam Barth + + 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 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 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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) -- 2.7.4