Changeset 70409 in webkit

Timestamp:

Oct 23, 2010 9:44:57 PM (13 years ago)

Author:

eric@webkit.org

Message:

2010-10-23 Eric Seidel <​eric@webkit.org>

Reviewed by Adam Barth.

EWS never removes invalid patch ids
​https://bugs.webkit.org/show_bug.cgi?id=48173

This is just sticking another finger in the dam.
However this adds more unit testing which will help
us make sure we're always releasing patches once we
redesign the release_patch API and call these from
a more central place.

• Scripts/webkitpy/tool/commands/queues.py:
• Scripts/webkitpy/tool/commands/queues_unittest.py:
• Scripts/webkitpy/tool/mocktool.py:
  • Added the ability to request invalid patches. Log a warning message to make sure we don't ever have tests use invalid patch fetches by mistake.

Location:

trunk/WebKitTools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/tool/commands/queues.py (modified) (4 diffs)
• Scripts/webkitpy/tool/commands/queues_unittest.py (modified) (2 diffs)
• Scripts/webkitpy/tool/mocktool.py (modified) (2 diffs)

trunk/WebKitTools/ChangeLog

@@ +1 @@
+2010-10-23  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Adam Barth.
+
+        EWS never removes invalid patch ids
+        https://bugs.webkit.org/show_bug.cgi?id=48173
+
+        This is just sticking another finger in the dam.
+        However this adds more unit testing which will help
+        us make sure we're always releasing patches once we
+        redesign the release_patch API and call these from
+        a more central place.
+
+        * Scripts/webkitpy/tool/commands/queues.py:
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        * Scripts/webkitpy/tool/mocktool.py:
+         - Added the ability to request invalid patches.
+           Log a warning message to make sure we don't ever have
+           tests use invalid patch fetches by mistake.
+
 2010-10-23  Dan Bernstein  <mitz@apple.com>
 

trunk/WebKitTools/Scripts/webkitpy/tool/commands/queues.py

@@ -39 +39 @@
 from StringIO import StringIO
 
-from webkitpy.common.net.bugzilla import CommitterValidator
+from webkitpy.common.net.bugzilla import CommitterValidator, Attachment
 from webkitpy.common.net.layouttestresults import path_for_layout_test, LayoutTestResults
 from webkitpy.common.net.statusserver import StatusServer
@@ -202 +202 @@
         return self._tool.status_server.update_status(self.name, message, patch, results_file)
 
-    def _fetch_next_work_item(self):
-        return self._tool.status_server.next_work_item(self.name)
+    def _next_patch(self):
+        patch_id = self._tool.status_server.next_work_item(self.name)
+        if not patch_id:
+            return None
+        patch = self._tool.bugs.fetch_attachment(patch_id)
+        if not patch:
+            # FIXME: Using a fake patch because release_work_item has the wrong API.
+            # We also don't really need to release the lock (although that's fine),
+            # mostly we just need to remove this bogus patch from our queue.
+            # If for some reason bugzilla is just down, then it will be re-fed later.
+            patch = Attachment({'id': patch_id}, None)
+            self._release_work_item(patch)
+            return None
+        return patch
 
     def _release_work_item(self, patch):
@@ -239 +251 @@
 
     def next_work_item(self):
-        patch_id = self._fetch_next_work_item()
-        if not patch_id:
-            return None
-        return self._tool.bugs.fetch_attachment(patch_id)
+        return self._next_patch()
 
     def should_proceed_with_work_item(self, patch):
@@ -396 +405 @@
 
     def next_work_item(self):
-        patch_id = self._fetch_next_work_item()
-        if not patch_id:
-            return None
-        return self._tool.bugs.fetch_attachment(patch_id)
+        return self._next_patch()
 
     def should_proceed_with_work_item(self, patch):

trunk/WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py

@@ -140 +140 @@
 
 class AbstractPatchQueueTest(CommandsTest):
-    def test_fetch_next_work_item(self):
+    def test_next_patch(self):
         queue = AbstractPatchQueue()
         tool = MockTool()
@@ -146 +146 @@
         queue._options = Mock()
         queue._options.port = None
-        self.assertEquals(queue._fetch_next_work_item(), None)
-        tool.status_server = MockStatusServer(work_items=[2, 1, 3])
-        self.assertEquals(queue._fetch_next_work_item(), 2)
+        self.assertEquals(queue._next_patch(), None)
+        tool.status_server = MockStatusServer(work_items=[2, 197])
+        expected_stdout = "MOCK: fetch_attachment: 2 is not a known attachment id\n"  # A mock-only message to prevent us from making mistakes.
+        expected_stderr = "MOCK: release_work_item: None 2\n"
+        patch_id = OutputCapture().assert_outputs(self, queue._next_patch, [], expected_stdout=expected_stdout, expected_stderr=expected_stderr)
+        self.assertEquals(patch_id, None)  # 2 is an invalid patch id
+        self.assertEquals(queue._next_patch().id(), 197)
 
 

trunk/WebKitTools/Scripts/webkitpy/tool/mocktool.py

@@ -293 +293 @@
             return self._override_patch
 
-        # This could be changed to .get() if we wish to allow failed lookups.
-        attachment_dictionary = self.attachment_cache[attachment_id]
+        attachment_dictionary = self.attachment_cache.get(attachment_id)
+        if not attachment_dictionary:
+            print "MOCK: fetch_attachment: %s is not a known attachment id" % attachment_id
+            return None
         bug = self.fetch_bug(attachment_dictionary["bug_id"])
         for attachment in bug.attachments(include_obsolete=True):
@@ -554 +556 @@
         if not self._work_items:
             return None
-        return self._work_items[0]
+        return self._work_items.pop(0)
 
     def release_work_item(self, queue_name, patch):