should_proceed_with_work_item is unused and can be removed
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 24 Feb 2012 07:52:51 +0000 (07:52 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 24 Feb 2012 07:52:51 +0000 (07:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=79416

Reviewed by Eric Seidel.

We used to use this function to check whether the tree is red.  Now, we
don't use external measures of whether to proceed with work items.
Instead, we analyze them with the idea in mind that the tree might be
red.

* Scripts/webkitpy/tool/bot/queueengine.py:
(QueueEngineDelegate.next_work_item):
(QueueEngine.run):
* Scripts/webkitpy/tool/bot/queueengine_unittest.py:
(LoggingDelegate):
(LoggingDelegate.next_work_item):
(RaisingDelegate.process_work_item):
(QueueEngineTest.test_terminating_error):
* Scripts/webkitpy/tool/commands/earlywarningsystem.py:
(AbstractEarlyWarningSystem.__init__):
* Scripts/webkitpy/tool/commands/queues.py:
(AbstractQueue.next_work_item):
(FeederQueue.next_work_item):
(CommitQueue.next_work_item):
(AbstractReviewQueue.next_work_item):
(StyleQueue.__init__):
* Scripts/webkitpy/tool/commands/queues_unittest.py:
(FeederQueueTest.test_feeder_queue):
(CommitQueueTest.test_commit_queue):
(test_commit_queue_failure):
(test_commit_queue_failure_with_failing_tests):
(test_rollout):
(test_rollout_lands):
(StyleQueueTest.test_style_queue_with_style_exception):
(test_style_queue_with_watch_list_exception):
* Scripts/webkitpy/tool/commands/queuestest.py:
(QueuesTest.assert_queue_outputs):
* Scripts/webkitpy/tool/commands/sheriffbot.py:
(SheriffBot.next_work_item):

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

Tools/ChangeLog
Tools/Scripts/webkitpy/tool/bot/queueengine.py
Tools/Scripts/webkitpy/tool/bot/queueengine_unittest.py
Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
Tools/Scripts/webkitpy/tool/commands/queues.py
Tools/Scripts/webkitpy/tool/commands/queues_unittest.py
Tools/Scripts/webkitpy/tool/commands/queuestest.py
Tools/Scripts/webkitpy/tool/commands/sheriffbot.py

index 0d58c6a..887333c 100644 (file)
@@ -1,3 +1,45 @@
+2012-02-23  Adam Barth  <abarth@webkit.org>
+
+        should_proceed_with_work_item is unused and can be removed
+        https://bugs.webkit.org/show_bug.cgi?id=79416
+
+        Reviewed by Eric Seidel.
+
+        We used to use this function to check whether the tree is red.  Now, we
+        don't use external measures of whether to proceed with work items.
+        Instead, we analyze them with the idea in mind that the tree might be
+        red.
+
+        * Scripts/webkitpy/tool/bot/queueengine.py:
+        (QueueEngineDelegate.next_work_item):
+        (QueueEngine.run):
+        * Scripts/webkitpy/tool/bot/queueengine_unittest.py:
+        (LoggingDelegate):
+        (LoggingDelegate.next_work_item):
+        (RaisingDelegate.process_work_item):
+        (QueueEngineTest.test_terminating_error):
+        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
+        (AbstractEarlyWarningSystem.__init__):
+        * Scripts/webkitpy/tool/commands/queues.py:
+        (AbstractQueue.next_work_item):
+        (FeederQueue.next_work_item):
+        (CommitQueue.next_work_item):
+        (AbstractReviewQueue.next_work_item):
+        (StyleQueue.__init__):
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        (FeederQueueTest.test_feeder_queue):
+        (CommitQueueTest.test_commit_queue):
+        (test_commit_queue_failure):
+        (test_commit_queue_failure_with_failing_tests):
+        (test_rollout):
+        (test_rollout_lands):
+        (StyleQueueTest.test_style_queue_with_style_exception):
+        (test_style_queue_with_watch_list_exception):
+        * Scripts/webkitpy/tool/commands/queuestest.py:
+        (QueuesTest.assert_queue_outputs):
+        * Scripts/webkitpy/tool/commands/sheriffbot.py:
+        (SheriffBot.next_work_item):
+
 2012-02-23  Adrienne Walker  <enne@google.com>
 
         Unreviewed, add Stephen Chenney to committers.py as a contributor.
index 6a523a8..1d75359 100644 (file)
@@ -58,10 +58,6 @@ class QueueEngineDelegate:
     def next_work_item(self):
         raise NotImplementedError, "subclasses must implement"
 
-    def should_proceed_with_work_item(self, work_item):
-        # returns (safe_to_proceed, waiting_message, patch)
-        raise NotImplementedError, "subclasses must implement"
-
     def process_work_item(self, work_item):
         raise NotImplementedError, "subclasses must implement"
 
@@ -98,9 +94,6 @@ class QueueEngine:
                 if not work_item:
                     self._sleep("No work item.")
                     continue
-                if not self._delegate.should_proceed_with_work_item(work_item):
-                    self._sleep("Not proceeding with work item.")
-                    continue
 
                 # FIXME: Work logs should not depend on bug_id specificaly.
                 #        This looks fixed, no?
index d860c6c..f959ee1 100644 (file)
@@ -50,7 +50,6 @@ class LoggingDelegate(QueueEngineDelegate):
         'begin_work_queue',
         'should_continue_work_queue',
         'next_work_item',
-        'should_proceed_with_work_item',
         'work_item_log_path',
         'process_work_item',
         'should_continue_work_queue',
@@ -82,12 +81,6 @@ class LoggingDelegate(QueueEngineDelegate):
         self.record("next_work_item")
         return "work_item"
 
-    def should_proceed_with_work_item(self, work_item):
-        self.record("should_proceed_with_work_item")
-        self._test.assertEquals(work_item, "work_item")
-        fake_patch = {'bug_id': 50000}
-        return (True, "waiting_message", fake_patch)
-
     def process_work_item(self, work_item):
         self.record("process_work_item")
         self._test.assertEquals(work_item, "work_item")
@@ -112,13 +105,6 @@ class RaisingDelegate(LoggingDelegate):
         raise self._exception
 
 
-class NotSafeToProceedDelegate(LoggingDelegate):
-    def should_proceed_with_work_item(self, work_item):
-        self.record("should_proceed_with_work_item")
-        self._test.assertEquals(work_item, "work_item")
-        return False
-
-
 class FastQueueEngine(QueueEngine):
     def __init__(self, delegate):
         QueueEngine.__init__(self, "fast-queue", delegate, threading.Event())
@@ -179,14 +165,6 @@ class QueueEngineTest(unittest.TestCase):
         self._test_terminating_queue(KeyboardInterrupt(), "User terminated queue.")
         self._test_terminating_queue(TerminateQueue(), "TerminateQueue exception received.")
 
-    def test_not_safe_to_proceed(self):
-        delegate = NotSafeToProceedDelegate(self)
-        self._run_engine(delegate, engine=FastQueueEngine(delegate))
-        expected_callbacks = LoggingDelegate.expected_callbacks[:]
-        expected_callbacks.remove('work_item_log_path')
-        expected_callbacks.remove('process_work_item')
-        self.assertEquals(delegate._callbacks, expected_callbacks)
-
     def test_now(self):
         """Make sure there are no typos in the QueueEngine.now() method."""
         engine = QueueEngine("test", None, None)
index 6f01823..4085e36 100644 (file)
@@ -50,9 +50,6 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue, EarlyWarningSystemTaskDele
         AbstractReviewQueue.__init__(self, options=options)
         self.port = DeprecatedPort.port(self.port_name)
 
-    def should_proceed_with_work_item(self, patch):
-        return True
-
     def begin_work_queue(self):
         # FIXME: This violates abstraction
         self._tool._deprecated_port = self.port
index 4edccd4..fc5a832 100644 (file)
@@ -128,9 +128,6 @@ class AbstractQueue(Command, QueueEngineDelegate):
     def next_work_item(self):
         raise NotImplementedError, "subclasses must implement"
 
-    def should_proceed_with_work_item(self, work_item):
-        raise NotImplementedError, "subclasses must implement"
-
     def process_work_item(self, work_item):
         raise NotImplementedError, "subclasses must implement"
 
@@ -185,9 +182,6 @@ class FeederQueue(AbstractQueue):
         # understands work items.
         return "synthetic-work-item"
 
-    def should_proceed_with_work_item(self, work_item):
-        return True
-
     def process_work_item(self, work_item):
         for feeder in self.feeders:
             feeder.feed()
@@ -275,11 +269,6 @@ class CommitQueue(AbstractPatchQueue, StepSequenceErrorHandler, CommitQueueTaskD
     def next_work_item(self):
         return self._next_patch()
 
-    def should_proceed_with_work_item(self, patch):
-        patch_text = "rollout patch" if patch.is_rollout() else "patch"
-        self._update_status("Processing %s" % patch_text, patch)
-        return True
-
     def process_work_item(self, patch):
         self._cc_watchers(patch.bug_id())
         task = CommitQueueTask(self, patch)
@@ -382,9 +371,6 @@ class AbstractReviewQueue(AbstractPatchQueue, StepSequenceErrorHandler):
     def next_work_item(self):
         return self._next_patch()
 
-    def should_proceed_with_work_item(self, patch):
-        raise NotImplementedError("subclasses must implement")
-
     def process_work_item(self, patch):
         try:
             if not self.review_patch(patch):
@@ -416,9 +402,6 @@ class StyleQueue(AbstractReviewQueue, StyleQueueTaskDelegate):
     def __init__(self):
         AbstractReviewQueue.__init__(self)
 
-    def should_proceed_with_work_item(self, patch):
-        return True
-
     def review_patch(self, patch):
         task = StyleQueueTask(self, patch)
         try:
index 532d7bf..79d970f 100644 (file)
@@ -132,7 +132,6 @@ class FeederQueueTest(QueuesTest):
         tool = MockTool(log_executive=True)
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("feeder-queue"),
-            "should_proceed_with_work_item": "",
             "next_work_item": "",
             "process_work_item": """Warning, attachment 10001 on bug 50000 has invalid committer (non-committer@example.com)
 Warning, attachment 10001 on bug 50000 has invalid committer (non-committer@example.com)
@@ -235,7 +234,6 @@ class CommitQueueTest(QueuesTest):
         tool.filesystem.write_text_file('/mock-results/full_results.json', '')  # Otherwise the commit-queue will hit a KeyError trying to read the results from the MockFileSystem.
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue"),
-            "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n",
             "next_work_item": "",
             "process_work_item": """MOCK: update_status: commit-queue Cleaned working directory
 MOCK: update_status: commit-queue Updated working directory
@@ -255,7 +253,6 @@ MOCK: release_work_item: commit-queue 10000
     def test_commit_queue_failure(self):
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue"),
-            "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n",
             "next_work_item": "",
             "process_work_item": """MOCK: update_status: commit-queue Cleaned working directory
 MOCK: update_status: commit-queue Updated working directory
@@ -283,7 +280,6 @@ MOCK: release_work_item: commit-queue 10000
     def test_commit_queue_failure_with_failing_tests(self):
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue"),
-            "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n",
             "next_work_item": "",
             "process_work_item": """MOCK: update_status: commit-queue Cleaned working directory
 MOCK: update_status: commit-queue Updated working directory
@@ -317,7 +313,6 @@ MOCK: release_work_item: commit-queue 10000
         tool.buildbot.light_tree_on_fire()
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue"),
-            "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing patch\n",
             "next_work_item": "",
             "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'clean'], cwd=/mock-checkout
 MOCK: update_status: commit-queue Cleaned working directory
@@ -348,7 +343,6 @@ MOCK: release_work_item: commit-queue 10000
         assert(rollout_patch.is_rollout())
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("commit-queue"),
-            "should_proceed_with_work_item": "MOCK: update_status: commit-queue Processing rollout patch\n",
             "next_work_item": "",
             "process_work_item": """MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'clean'], cwd=/mock-checkout
 MOCK: update_status: commit-queue Cleaned working directory
@@ -448,7 +442,6 @@ class StyleQueueTest(QueuesTest):
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("style-queue"),
             "next_work_item": "",
-            "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
@@ -481,7 +474,6 @@ MOCK: release_work_item: style-queue 10000
         expected_stderr = {
             "begin_work_queue": self._default_begin_work_queue_stderr("style-queue"),
             "next_work_item": "",
-            "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
index cb16b53..dd8d4a4 100644 (file)
@@ -91,7 +91,6 @@ class QueuesTest(unittest.TestCase):
         self.assert_outputs(queue.begin_work_queue, "begin_work_queue", [], expected_stdout, expected_stderr, expected_exceptions)
         self.assert_outputs(queue.should_continue_work_queue, "should_continue_work_queue", [], expected_stdout, expected_stderr, expected_exceptions)
         self.assert_outputs(queue.next_work_item, "next_work_item", [], expected_stdout, expected_stderr, expected_exceptions)
-        self.assert_outputs(queue.should_proceed_with_work_item, "should_proceed_with_work_item", [work_item], expected_stdout, expected_stderr, expected_exceptions)
         self.assert_outputs(queue.process_work_item, "process_work_item", [work_item], expected_stdout, expected_stderr, expected_exceptions)
         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?
index adbbf66..81f4353 100644 (file)
@@ -58,10 +58,6 @@ class SheriffBot(AbstractQueue, StepSequenceErrorHandler):
         self._irc_bot.process_pending_messages()
         return
 
-    def should_proceed_with_work_item(self, failure_map):
-        # Currently, we don't have any reasons not to proceed with work items.
-        return True
-
     def process_work_item(self, failure_map):
         return True