Changeset 85871 in webkit

Timestamp:

May 5, 2011 1:18:40 PM (13 years ago)

Author:

eric@webkit.org

Message:

2011-05-05 Eric Seidel <​eric@webkit.org>

Reviewed by Adam Barth.

commit-queue should reject patches which fail ewses
​https://bugs.webkit.org/show_bug.cgi?id=47534

• Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
• Scripts/webkitpy/tool/commands/earlywarningsystem.py:
• Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/common/net/bugzilla/bugzilla.py (modified) (2 diffs)
• Scripts/webkitpy/tool/commands/earlywarningsystem.py (modified) (4 diffs)
• Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py (modified) (4 diffs)

trunk/Tools/ChangeLog

@@ +1 @@
+2011-05-05  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Adam Barth.
+
+        commit-queue should reject patches which fail ewses
+        https://bugs.webkit.org/show_bug.cgi?id=47534
+
+        * Scripts/webkitpy/common/net/bugzilla/bugzilla.py:
+        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
+        * Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
+
 2011-05-05  Chang Shu  <cshu@webkit.org>
 

trunk/Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py

@@ -617 +617 @@
         self.authenticate()
 
+        # FIXME: additional_comment_text seems useless and should be merged into comment-text.
         if additional_comment_text:
             comment_text += "\n\n%s" % additional_comment_text
@@ -660 +661 @@
         self.authenticate()
 
-        log("Adding %s to the CC list for bug %s" % (email_address_list,
-                                                     bug_id))
+        log("Adding %s to the CC list for bug %s" % (email_address_list, bug_id))
         if self.dryrun:
             return

trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py

@@ -100 +100 @@
 
     @classmethod
+    def _post_reject_message_on_bug(cls, tool, patch, status_id, extra_message_text=None):
+        results_link = tool.status_server.results_url_for_status(status_id)
+        message = "Attachment %s did not pass %s (%s):\nOutput: %s" % (patch.id(), cls.name, cls.port_name, results_link)
+        # FIXME: We might want to add some text about rejecting from the commit-queue in
+        # the case where patch.commit_queue() isn't already set to '-'.
+        if cls.watchers:
+            tool.bugs.add_cc_to_bug(patch.bug_id(), cls.watchers)
+        tool.bugs.set_flag_on_attachment(patch.bug_id(), "commit-queue", "-", message, extra_message_text)
+
+    @classmethod
     def handle_script_error(cls, tool, state, script_error):
         is_svn_apply = script_error.command_name() == "svn-apply"
@@ -105 +115 @@
         if is_svn_apply:
             QueueEngine.exit_after_handled_error(script_error)
-        results_link = tool.status_server.results_url_for_status(status_id)
-        message = "Attachment %s did not build on %s:\nBuild output: %s" % (state["patch"].id(), cls.port_name, results_link)
-        tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
+        cls._post_reject_message_on_bug(tool, state['patch'], status_id)
         exit(1)
+
 
 # FIXME: This should merge with AbstractEarlyWarningSystem once all the EWS
@@ -120 +129 @@
         self._layout_test_results_reader = LayoutTestResultsReader(self._tool, self._log_directory())
 
-    def _post_reject_message_on_bug(self, task, patch):
-        results_link = self._tool.status_server.results_url_for_status(task.failure_status_id)
-        message = "Attachment %s did not pass %s:\nOutput: %s" % (patch.id(), self.name, results_link)
+    def _failing_tests_message(self, task, patch):
         results = task.results_from_patch_test_run(patch)
         unexpected_failures = self._expected_failures.unexpected_failures(results)
-        if unexpected_failures:
-            message += "\nNew failing tests:\n%s" % "\n".join(unexpected_failures)
-        self._tool.bugs.post_comment_to_bug(patch.bug_id(), message, cc=self.watchers)
+        if not unexpected_failures:
+            return None
+        return "New failing tests:\n%s" % "\n".join(unexpected_failures)
 
     def review_patch(self, patch):
@@ -140 +147 @@
             return False
         except ScriptError, e:
-            # FIXME: This should just use CommitterValidator.reject_patch_from_commit_queue
-            self._post_reject_message_on_bug(task, patch)
+            self._post_reject_message_on_bug(self._tool, patch, task.failure_status_id, self._failing_tests_message(task, patch))
             results_archive = task.results_archive_from_patch_test_run(patch)
             if results_archive:

trunk/Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py

@@ -38 +38 @@
 
 class AbstractEarlyWarningSystemTest(QueuesTest):
+    # Needed to define port_name, used in AbstractEarlyWarningSystem.__init__
+    class TestEWS(AbstractEarlyWarningSystem):
+        name = "mock-ews"
+        port_name = "win"  # Needs to be a port which port/factory understands.
+
     def test_can_build(self):
-        # Needed to define port_name, used in AbstractEarlyWarningSystem.__init__
-        class TestEWS(AbstractEarlyWarningSystem):
-            port_name = "win"  # Needs to be a port which port/factory understands.
-
-        queue = TestEWS()
+        queue = self.TestEWS()
         queue.bind_to_tool(MockTool(log_executive=True))
         queue._options = MockOptions(port=None)
@@ -53 +54 @@
 
         queue.run_webkit_patch = mock_run_webkit_patch
-        expected_stderr = "MOCK: update_status: None Unable to perform a build\n"
+        expected_stderr = "MOCK: update_status: mock-ews Unable to perform a build\n"
         OutputCapture().assert_outputs(self, queue._can_build, [], expected_stderr=expected_stderr)
 
@@ -68 +69 @@
         expected_stderr = "MOCK: release_work_item: None 197\n"
         OutputCapture().assert_outputs(self, queue.process_work_item, [mock_patch], expected_stderr=expected_stderr, expected_exception=ScriptError)
+
+    def test_post_reject_message_on_bug(self):
+        tool = MockTool()
+        patch = tool.bugs.fetch_attachment(197)
+        expected_stderr = """MOCK setting flag 'commit-queue' to '-' on attachment '42' with comment 'Attachment 197 did not pass mock-ews (win):
+Output: http://dummy_url' and additional comment 'EXTRA'
+"""
+        OutputCapture().assert_outputs(self,
+            self.TestEWS._post_reject_message_on_bug,
+            kwargs={'tool': tool, 'patch': patch, 'status_id': 1, 'extra_message_text': "EXTRA"},
+            expected_stderr=expected_stderr)
+
+
+class AbstractTestingEWSTest(QueuesTest):
+    def test_failing_tests_message(self):
+        # Needed to define port_name, used in AbstractEarlyWarningSystem.__init__
+        class TestEWS(AbstractTestingEWS):
+            port_name = "win"  # Needs to be a port which port/factory understands.
+
+        ews = TestEWS()
+        ews.bind_to_tool(MockTool())
+        ews._options = MockOptions(port=None, confirm=False)
+        OutputCapture().assert_outputs(self, ews.begin_work_queue, expected_stderr=self._default_begin_work_queue_stderr(ews.name, ews._tool.scm().checkout_root))
+        ews._expected_failures.unexpected_failures = lambda results: set(["foo.html", "bar.html"])
+        task = Mock()
+        patch = ews._tool.bugs.fetch_attachment(197)
+        self.assertEqual(ews._failing_tests_message(task, patch), "New failing tests:\nbar.html\nfoo.html")
 
 
@@ -90 +118 @@
             "next_work_item": "",
             "process_work_item": "MOCK: update_status: %(name)s Pass\nMOCK: release_work_item: %(name)s 197\n" % string_replacemnts,
-            "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=42, cc=%(watchers)s\n--- Begin comment ---\nAttachment 197 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts,
+            "handle_script_error": """MOCK: update_status: %(name)s ScriptError error message
+MOCK setting flag 'commit-queue' to '-' on attachment '42' with comment 'Attachment 197 did not pass %(name)s (%(port)s):
+Output: http://dummy_url' and additional comment 'None'
+""" % string_replacemnts,
         }
         return expected_stderr