commit-queue should check for oops in changelog entries
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 May 2013 03:58:00 +0000 (03:58 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 May 2013 03:58:00 +0000 (03:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=116395

Reviewed by Martin Robinson.

Make changelog validation fail when it contains oops!.

* Scripts/webkitpy/tool/bot/commitqueuetask.py:
(CommitQueueTask._validate_changelog):
* Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:
(CommitQueueTaskTest._run_through_task):
* Scripts/webkitpy/tool/commands/queues_unittest.py:
* Scripts/webkitpy/tool/steps/validatechangelogs.py:
(ValidateChangeLogs.options):
(ValidateChangeLogs._check_changelog_diff):
(ValidateChangeLogs._changelog_contains_oops):
(ValidateChangeLogs.run):
* Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py:
(ValidateChangeLogsTest._assert_start_line_produces_output):
(ValidateChangeLogsTest.test_check_changelog_diff):
(ValidateChangeLogsTest):
(ValidateChangeLogsTest.test_changelog_contains_oops):

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

Tools/ChangeLog
Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py
Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py
Tools/Scripts/webkitpy/tool/commands/queues_unittest.py
Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py
Tools/Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py

index b3515cd2ecdee3f87bd32fb0351665fdacd3a3f1..7faccb510a01c632c14398c3d34b2a4e108ff2f5 100644 (file)
@@ -1,3 +1,28 @@
+2013-05-20  Ryosuke Niwa  <rniwa@webkit.org>
+
+        commit-queue should check for oops in changelog entries
+        https://bugs.webkit.org/show_bug.cgi?id=116395
+
+        Reviewed by Martin Robinson.
+
+        Make changelog validation fail when it contains oops!.
+
+        * Scripts/webkitpy/tool/bot/commitqueuetask.py:
+        (CommitQueueTask._validate_changelog):
+        * Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:
+        (CommitQueueTaskTest._run_through_task):
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        * Scripts/webkitpy/tool/steps/validatechangelogs.py:
+        (ValidateChangeLogs.options):
+        (ValidateChangeLogs._check_changelog_diff):
+        (ValidateChangeLogs._changelog_contains_oops):
+        (ValidateChangeLogs.run):
+        * Scripts/webkitpy/tool/steps/validatechangelogs_unittest.py:
+        (ValidateChangeLogsTest._assert_start_line_produces_output):
+        (ValidateChangeLogsTest.test_check_changelog_diff):
+        (ValidateChangeLogsTest):
+        (ValidateChangeLogsTest.test_changelog_contains_oops):
+
 2013-05-20  Jessie Berlin  <jberlin@apple.com>
 
         Expose a way to know when forms are added to a page or when form controls are added to a form
index 5b5a5afee53172d90d7daa0a7c88e58500f50936..a95c7b103a15ae8f7f2796b2deffb43c24ff769f 100644 (file)
@@ -55,6 +55,7 @@ class CommitQueueTask(PatchAnalysisTask):
     def _validate_changelog(self):
         return self._run_command([
             "validate-changelog",
+            "--check-oops",
             "--non-interactive",
             self._patch.id(),
         ],
index ba9254b02b32c7a11cdb419099376f87956aedd8..1eabde1b59ddafe8efe396fee03bb02644eaa10e 100644 (file)
@@ -124,6 +124,7 @@ class GoldenScriptError(ScriptError):
 
 class CommitQueueTaskTest(unittest.TestCase):
     def _run_through_task(self, commit_queue, expected_logs, expected_exception=None, expect_retry=False):
+        self.maxDiff = None
         tool = MockTool(log_executive=True)
         patch = tool.bugs.fetch_attachment(10000)
         task = CommitQueueTask(commit_queue, patch)
@@ -140,7 +141,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -160,7 +161,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -218,7 +219,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_failed: failure_message='ChangeLog did not pass validation' script_error='MOCK validate failure' patch='10000'
 """
         self._run_through_task(commit_queue, expected_logs, GoldenScriptError)
@@ -237,7 +238,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_failed: failure_message='Patch does not build' script_error='MOCK build failure' patch='10000'
@@ -261,7 +262,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_failed: failure_message='Patch does not build' script_error='MOCK build failure' patch='10000'
@@ -288,7 +289,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -322,7 +323,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -358,7 +359,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -390,7 +391,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -429,7 +430,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -472,7 +473,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -511,7 +512,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
@@ -545,7 +546,7 @@ run_webkit_patch: ['update']
 command_passed: success_message='Updated working directory' patch='10000'
 run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
 command_passed: success_message='Applied patch' patch='10000'
-run_webkit_patch: ['validate-changelog', '--non-interactive', 10000]
+run_webkit_patch: ['validate-changelog', '--check-oops', '--non-interactive', 10000]
 command_passed: success_message='ChangeLog validated' patch='10000'
 run_webkit_patch: ['build', '--no-clean', '--no-update', '--build-style=both']
 command_passed: success_message='Built patch' patch='10000'
index b668122799949025c0a95a79ac0f3425e16c24cf..a09164ddeaeb679d3c44ed4e5a4cc18ba11e0c38 100644 (file)
@@ -244,7 +244,7 @@ Running: webkit-patch --status-host=example.com update --port=mac
 MOCK: update_status: commit-queue Updated working directory
 Running: webkit-patch --status-host=example.com apply-attachment --no-update --non-interactive 10000 --port=mac
 MOCK: update_status: commit-queue Applied patch
-Running: webkit-patch --status-host=example.com validate-changelog --non-interactive 10000 --port=mac
+Running: webkit-patch --status-host=example.com validate-changelog --check-oops --non-interactive 10000 --port=mac
 MOCK: update_status: commit-queue ChangeLog validated
 Running: webkit-patch --status-host=example.com build --no-clean --no-update --build-style=release --port=mac
 MOCK: update_status: commit-queue Built patch
@@ -328,7 +328,7 @@ Running: webkit-patch --status-host=example.com update --port=%(port)s
 MOCK: update_status: commit-queue Updated working directory
 Running: webkit-patch --status-host=example.com apply-attachment --no-update --non-interactive 10000 --port=%(port)s
 MOCK: update_status: commit-queue Applied patch
-Running: webkit-patch --status-host=example.com validate-changelog --non-interactive 10000 --port=%(port)s
+Running: webkit-patch --status-host=example.com validate-changelog --check-oops --non-interactive 10000 --port=%(port)s
 MOCK: update_status: commit-queue ChangeLog validated
 Running: webkit-patch --status-host=example.com build --no-clean --no-update --build-style=release --port=%(port)s
 MOCK: update_status: commit-queue Built patch
@@ -357,7 +357,7 @@ Running: webkit-patch --status-host=example.com update --port=%(port)s
 MOCK: update_status: commit-queue Updated working directory
 Running: webkit-patch --status-host=example.com apply-attachment --no-update --non-interactive 10005 --port=%(port)s
 MOCK: update_status: commit-queue Applied patch
-Running: webkit-patch --status-host=example.com validate-changelog --non-interactive 10005 --port=%(port)s
+Running: webkit-patch --status-host=example.com validate-changelog --check-oops --non-interactive 10005 --port=%(port)s
 MOCK: update_status: commit-queue ChangeLog validated
 Running: webkit-patch --status-host=example.com land-attachment --force-clean --non-interactive --parent-command=commit-queue 10005 --port=%(port)s
 MOCK: update_status: commit-queue Landed patch
@@ -399,7 +399,7 @@ Running: webkit-patch --status-host=example.com update --port=mac
 MOCK: update_status: commit-queue Updated working directory
 Running: webkit-patch --status-host=example.com apply-attachment --no-update --non-interactive 10000 --port=mac
 MOCK: update_status: commit-queue Applied patch
-Running: webkit-patch --status-host=example.com validate-changelog --non-interactive 10000 --port=mac
+Running: webkit-patch --status-host=example.com validate-changelog --check-oops --non-interactive 10000 --port=mac
 MOCK: update_status: commit-queue ChangeLog validated
 Running: webkit-patch --status-host=example.com build --no-clean --no-update --build-style=release --port=mac
 MOCK: update_status: commit-queue Built patch
index 061baa5ec8a97c2fe475b3a8f531cd0e37217684..e77e5c01e4d2c9a5f62c11912dc39d52d68858d4 100644 (file)
@@ -29,6 +29,7 @@
 import logging
 import sys
 
+from optparse import make_option
 from webkitpy.tool.steps.abstractstep import AbstractStep
 from webkitpy.tool.steps.options import Options
 from webkitpy.common.checkout.diff_parser import DiffParser
@@ -42,12 +43,11 @@ class ValidateChangeLogs(AbstractStep):
     @classmethod
     def options(cls):
         return AbstractStep.options() + [
+            make_option("--check-oops", action="store_true", default=False, help="Check there are no OOPS left in change log"),
             Options.non_interactive,
         ]
 
     def _check_changelog_diff(self, diff_file):
-        if not self._tool.checkout().is_path_to_changelog(diff_file.filename):
-            return True
         # Each line is a tuple, the first value is the deleted line number
         # Date, reviewer, bug title, bug url, and empty lines could all be
         # identical in the most recent entries.  If the diff starts any
@@ -64,6 +64,12 @@ class ValidateChangeLogs(AbstractStep):
             return True
         return False
 
+    def _changelog_contains_oops(self, diff_file):
+        for diff_line in diff_file.lines:
+            if 'OOPS!' in diff_line[2]:
+                return True
+        return False
+
     def run(self, state):
         changed_files = self.cached_lookup(state, "changed_files")
         for filename in changed_files:
@@ -76,6 +82,11 @@ class ValidateChangeLogs(AbstractStep):
             diff = self._tool.scm().diff_for_file(filename)
             parsed_diff = DiffParser(diff.splitlines())
             for filename, diff_file in parsed_diff.files.items():
+                if not self._tool.checkout().is_path_to_changelog(diff_file.filename):
+                    continue
                 if not self._check_changelog_diff(diff_file):
                     _log.error("ChangeLog entry in %s is not at the top of the file." % diff_file.filename)
                     sys.exit(1)
+                if self._options.check_oops and self._changelog_contains_oops(diff_file):
+                    _log.error("ChangeLog entry in %s contains OOPS!." % diff_file.filename)
+                    sys.exit(1)
index 8a23be49b4538e15fc4e4bcceb87cb01b5f308bf..50ecc4646c481d03482dcb9dd5ee83f7cece905c 100644 (file)
@@ -38,7 +38,6 @@ class ValidateChangeLogsTest(unittest.TestCase):
 
     def _assert_start_line_produces_output(self, start_line, should_fail=False, non_interactive=False):
         tool = MockTool()
-        tool._checkout.is_path_to_changelog = lambda path: True
         step = ValidateChangeLogs(tool, MockOptions(git_commit=None, non_interactive=non_interactive))
         diff_file = Mock()
         diff_file.filename = "mock/ChangeLog"
@@ -56,3 +55,15 @@ class ValidateChangeLogsTest(unittest.TestCase):
 
         self._assert_start_line_produces_output(1, non_interactive=False)
         self._assert_start_line_produces_output(8, non_interactive=True, should_fail=True)
+
+    def test_changelog_contains_oops(self):
+        tool = MockTool()
+        tool._checkout.is_path_to_changelog = lambda path: True
+        step = ValidateChangeLogs(tool, MockOptions(git_commit=None, non_interactive=True, check_oops=True))
+        diff_file = Mock()
+        diff_file.filename = "mock/ChangeLog"
+        diff_file.lines = [(1, 1, "foo"), (2, 2, "bar OOPS! bar"), (3, 3, "foo")]
+        self.assertTrue(OutputCapture().assert_outputs(self, step._changelog_contains_oops, [diff_file], expected_logs=''))
+
+        diff_file.lines = [(1, 1, "foo"), (2, 2, "bar OOPS bar"), (3, 3, "foo")]
+        self.assertFalse(OutputCapture().assert_outputs(self, step._changelog_contains_oops, [diff_file], expected_logs=''))