2010-06-10 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Jun 2010 21:55:34 +0000 (21:55 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Jun 2010 21:55:34 +0000 (21:55 +0000)
        Reviewed by Adam Barth.

        fix handle_script_error in rietveld upload queue and add testing for handle_script_error
        https://bugs.webkit.org/show_bug.cgi?id=40436

        * Scripts/webkitpy/common/system/outputcapture.py:
        * Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
        * Scripts/webkitpy/tool/commands/queues.py:
        * Scripts/webkitpy/tool/commands/queues_unittest.py:
        * Scripts/webkitpy/tool/commands/queuestest.py:
        * Scripts/webkitpy/tool/mocktool.py:

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

WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/common/system/outputcapture.py
WebKitTools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
WebKitTools/Scripts/webkitpy/tool/commands/queues.py
WebKitTools/Scripts/webkitpy/tool/commands/queues_unittest.py
WebKitTools/Scripts/webkitpy/tool/commands/queuestest.py
WebKitTools/Scripts/webkitpy/tool/mocktool.py

index 615ed9c..a345e06 100644 (file)
@@ -1,3 +1,17 @@
+2010-06-10  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        fix handle_script_error in rietveld upload queue and add testing for handle_script_error
+        https://bugs.webkit.org/show_bug.cgi?id=40436
+
+        * Scripts/webkitpy/common/system/outputcapture.py:
+        * Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
+        * Scripts/webkitpy/tool/commands/queues.py:
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        * Scripts/webkitpy/tool/commands/queuestest.py:
+        * Scripts/webkitpy/tool/mocktool.py:
+
 2010-06-10  Jarkko Sakkinen  <jarkko.j.sakkinen@gmail.com>
 
         Reviewed by Simon Hausmann.
index 592a669..68a3919 100644 (file)
@@ -52,9 +52,12 @@ class OutputCapture(object):
     def restore_output(self):
         return (self._restore_output_with_name("stdout"), self._restore_output_with_name("stderr"))
 
-    def assert_outputs(self, testcase, function, args=[], kwargs={}, expected_stdout="", expected_stderr=""):
+    def assert_outputs(self, testcase, function, args=[], kwargs={}, expected_stdout="", expected_stderr="", expected_exception=None):
         self.capture_output()
-        return_value = function(*args, **kwargs)
+        if expected_exception:
+            return_value = testcase.assertRaises(expected_exception, function, *args, **kwargs)
+        else:
+            return_value = function(*args, **kwargs)
         (stdout_string, stderr_string) = self.restore_output()
         testcase.assertEqual(stdout_string, expected_stdout)
         testcase.assertEqual(stderr_string, expected_stderr)
index 27e09ba..67393d8 100644 (file)
@@ -43,17 +43,23 @@ class EarlyWarningSytemTest(QueuesTest):
         string_replacemnts = {
             "name": ews.name,
             "checkout_dir": os.getcwd(),  # FIXME: Use of os.getcwd() is wrong, should be scm.checkout_root
+            "port": ews.port_name,
+            "watchers": ews.watchers,
         }
         expected_stderr = {
             "begin_work_queue": "CAUTION: %(name)s will discard all local changes in \"%(checkout_dir)s\"\nRunning WebKit %(name)s.\n" % string_replacemnts,
             "handle_unexpected_error": "Mock error message\n",
             "next_work_item": "MOCK: update_work_items: %(name)s [103]\n" % string_replacemnts,
             "process_work_item": "MOCK: update_status: %(name)s Pass\n" % string_replacemnts,
+            "handle_script_error": "MOCK: update_status: %(name)s ScriptError error message\nMOCK bug comment: bug_id=345, cc=%(watchers)s\n--- Begin comment ---\\Attachment 1234 did not build on %(port)s:\nBuild output: http://dummy_url\n--- End comment ---\n\n" % string_replacemnts,
         }
         return expected_stderr
 
     def _test_ews(self, ews):
-        self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews))
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
+        }
+        self.assert_queue_outputs(ews, expected_stderr=self._default_expected_stderr(ews), expected_exceptions=expected_exceptions)
 
     # FIXME: If all EWSes are going to output the same text, we
     # could test them all in one method with a for loop over an array.
@@ -73,4 +79,7 @@ class EarlyWarningSytemTest(QueuesTest):
         ews = MacEWS()
         expected_stderr = self._default_expected_stderr(ews)
         expected_stderr["process_work_item"] = "MOCK: update_status: mac-ews Error: mac-ews cannot process patches from non-committers :(\n"
-        self.assert_queue_outputs(ews, expected_stderr=expected_stderr)
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
+        }
+        self.assert_queue_outputs(ews, expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
index e2957d3..d5f6dd9 100644 (file)
@@ -331,18 +331,21 @@ class RietveldUploadQueue(AbstractPatchQueue, StepSequenceErrorHandler):
                 self._did_fail(patch)
             raise e
 
-    def _reject_patch(self, patch_id, message):
-        self.tool.bugs.set_flag_on_attachment(patch_id, "in-rietveld", "-")
+    @classmethod
+    def _reject_patch(cls, tool, patch_id):
+        tool.bugs.set_flag_on_attachment(patch_id, "in-rietveld", "-")
 
     def handle_unexpected_error(self, patch, message):
-        self._reject_patch(patch.id(), message)
+        log(message)
+        self._reject_patch(self.tool, patch.id())
 
     # StepSequenceErrorHandler methods
 
     @classmethod
     def handle_script_error(cls, tool, state, script_error):
-        status_id = cls._update_status_for_script_error(tool, state, script_error)
-        cls._reject_patch(state["patch"].id())
+        log(script_error.message_with_output())
+        cls._update_status_for_script_error(tool, state, script_error)
+        cls._reject_patch(tool, state["patch"].id())
 
 
 class AbstractReviewQueue(AbstractPatchQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler):
index 6388372..b32dfa8 100644 (file)
@@ -128,6 +128,7 @@ MOCK: update_work_items: commit-queue [106, 197]
 """,
             "process_work_item" : "MOCK: update_status: commit-queue Pass\n",
             "handle_unexpected_error" : "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), expected_stderr=expected_stderr)
 
@@ -147,6 +148,7 @@ MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://buil
 """,
             "process_work_item" : "MOCK: update_status: commit-queue Builders [\"Builder2\"] are red. See http://build.webkit.org\n",
             "handle_unexpected_error" : "MOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, expected_stderr=expected_stderr)
 
@@ -167,6 +169,7 @@ MOCK: update_status: commit-queue Builders ["Builder2"] are red. See http://buil
 """,
             "process_work_item": "MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 76543]\nMOCK: update_status: commit-queue Pass\n",
             "handle_unexpected_error": "MOCK setting flag 'commit-queue' to '-' on attachment '76543' with comment 'Rejecting patch 76543 from commit-queue.' and additional comment 'Mock error message'\n",
+            "handle_script_error": "MOCK: update_status: commit-queue ScriptError error message\nMOCK setting flag 'commit-queue' to '-' on attachment '1234' with comment 'Rejecting patch 1234 from commit-queue.' and additional comment 'ScriptError error message'\n",
         }
         self.assert_queue_outputs(CommitQueue(), tool=tool, work_item=rollout_patch, expected_stderr=expected_stderr)
 
@@ -205,7 +208,8 @@ class RietveldUploadQueueTest(QueuesTest):
             "begin_work_queue": "CAUTION: rietveld-upload-queue will discard all local changes in \"%s\"\nRunning WebKit rietveld-upload-queue.\n" % MockSCM.fake_checkout_root,
             "should_proceed_with_work_item": "MOCK: update_status: rietveld-upload-queue Uploading patch\n",
             "process_work_item": "MOCK: update_status: rietveld-upload-queue Pass\n",
-            "handle_unexpected_error": "MOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
+            "handle_unexpected_error": "Mock error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
+            "handle_script_error": "ScriptError error message\nMOCK: update_status: rietveld-upload-queue ScriptError error message\nMOCK setting flag 'in-rietveld' to '-' on attachment '1234' with comment 'None' and additional comment 'None'\n",
         }
         self.assert_queue_outputs(RietveldUploadQueue(), expected_stderr=expected_stderr)
 
@@ -218,5 +222,9 @@ class StyleQueueTest(QueuesTest):
             "should_proceed_with_work_item": "MOCK: update_status: style-queue Checking style\n",
             "process_work_item" : "MOCK: update_status: style-queue Pass\n",
             "handle_unexpected_error" : "Mock error message\n",
+            "handle_script_error": "MOCK: update_status: style-queue ScriptError error message\nMOCK bug comment: bug_id=345, cc=[]\n--- Begin comment ---\\Attachment 1234 did not pass style-queue:\n\nScriptError error message\n\nIf any of these errors are false positives, please file a bug against check-webkit-style.\n--- End comment ---\n\n",
         }
-        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr)
+        expected_exceptions = {
+            "handle_script_error": SystemExit,
+        }
+        self.assert_queue_outputs(StyleQueue(), expected_stderr=expected_stderr, expected_exceptions=expected_exceptions)
index bf7e32a..9e17c5c 100644 (file)
@@ -30,6 +30,7 @@ import unittest
 
 from webkitpy.common.net.bugzilla import Attachment
 from webkitpy.common.system.outputcapture import OutputCapture
+from webkitpy.common.system.executive import ScriptError
 from webkitpy.thirdparty.mock import Mock
 from webkitpy.tool.mocktool import MockTool
 
@@ -42,6 +43,14 @@ class MockQueueEngine(object):
         pass
 
 
+class MockPatch():
+    def id(self):
+        return 1234
+
+    def bug_id(self):
+        return 345
+
+
 class QueuesTest(unittest.TestCase):
     mock_work_item = Attachment({
         "id": 1234,
@@ -50,7 +59,19 @@ class QueuesTest(unittest.TestCase):
         "attacher_email": "adam@example.com",
     }, None)
 
-    def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, options=Mock(), tool=MockTool()):
+    def assert_outputs(self, func, func_name, args, expected_stdout, expected_stderr, expected_exceptions):
+        exception = None
+        if expected_exceptions and func_name in expected_exceptions:
+            exception = expected_exceptions[func_name]
+
+        OutputCapture().assert_outputs(self,
+                func,
+                args=args,
+                expected_stdout=expected_stdout.get(func_name, ""),
+                expected_stderr=expected_stderr.get(func_name, ""),
+                expected_exception=exception)
+
+    def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, expected_exceptions=None, options=Mock(), tool=MockTool()):
         if not expected_stdout:
             expected_stdout = {}
         if not expected_stderr:
@@ -63,38 +84,12 @@ class QueuesTest(unittest.TestCase):
 
         queue.execute(options, args, tool, engine=MockQueueEngine)
 
-        OutputCapture().assert_outputs(self,
-                queue.queue_log_path,
-                expected_stdout=expected_stdout.get("queue_log_path", ""),
-                expected_stderr=expected_stderr.get("queue_log_path", ""))
-        OutputCapture().assert_outputs(self,
-                queue.work_item_log_path,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("work_item_log_path", ""),
-                expected_stderr=expected_stderr.get("work_item_log_path", ""))
-        OutputCapture().assert_outputs(self,
-                queue.begin_work_queue,
-                expected_stdout=expected_stdout.get("begin_work_queue", ""),
-                expected_stderr=expected_stderr.get("begin_work_queue", ""))
-        OutputCapture().assert_outputs(self,
-                queue.should_continue_work_queue,
-                expected_stdout=expected_stdout.get("should_continue_work_queue", ""), expected_stderr=expected_stderr.get("should_continue_work_queue", ""))
-        OutputCapture().assert_outputs(self,
-                queue.next_work_item,
-                expected_stdout=expected_stdout.get("next_work_item", ""),
-                expected_stderr=expected_stderr.get("next_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.should_proceed_with_work_item,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("should_proceed_with_work_item", ""),
-                expected_stderr=expected_stderr.get("should_proceed_with_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.process_work_item,
-                args=[work_item],
-                expected_stdout=expected_stdout.get("process_work_item", ""),
-                expected_stderr=expected_stderr.get("process_work_item", ""))
-        OutputCapture().assert_outputs(self,
-                queue.handle_unexpected_error,
-                args=[work_item, "Mock error message"],
-                expected_stdout=expected_stdout.get("handle_unexpected_error", ""),
-                expected_stderr=expected_stderr.get("handle_unexpected_error", ""))
+        self.assert_outputs(queue.queue_log_path, "queue_log_path", [], expected_stdout, expected_stderr, expected_exceptions)
+        self.assert_outputs(queue.work_item_log_path, "work_item_log_path", [work_item], expected_stdout, expected_stderr, expected_exceptions)
+        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)
+        self.assert_outputs(queue.handle_script_error, "handle_script_error", [tool, {"patch": MockPatch()}, ScriptError(message="ScriptError error message", script_args="MockErrorCommand")], expected_stdout, expected_stderr, expected_exceptions)
index bef1a69..d88190f 100644 (file)
@@ -512,6 +512,8 @@ class MockStatusServer(object):
     def update_svn_revision(self, svn_revision, broken_bot):
         return 191
 
+    def results_url_for_status(self, status_id):
+        return "http://dummy_url"
 
 class MockExecute(Mock):
     def __init__(self, should_log):