[webkit-changes] [97322] trunk/Tools

Wed, 12 Oct 2011 17:20:41 -0700

Title: [97322] trunk/Tools
Revision
97322
Author
[email protected]
Date
2011-10-12 17:21:40 -0700 (Wed, 12 Oct 2011)

Log Message

commit-queue doesn't have a friendly error message when the reviewer line is messed up
https://bugs.webkit.org/show_bug.cgi?id=69979

Reviewed by Eric Seidel.

Rather than combining the ChangeLog validation with a more complicated
command, this patch has the commit-queue run it as a separate command,
which will give us more control over the error message.

* Scripts/webkitpy/tool/bot/commitqueuetask.py:
* Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:
* Scripts/webkitpy/tool/commands/download.py:
* Scripts/webkitpy/tool/commands/queues_unittest.py:
* Scripts/webkitpy/tool/steps/validatechangelogs.py:
* Scripts/webkitpy/tool/steps/validatereviewer.py:

Modified Paths

Diff

Modified: trunk/Tools/ChangeLog (97321 => 97322)


--- trunk/Tools/ChangeLog	2011-10-13 00:14:53 UTC (rev 97321)
+++ trunk/Tools/ChangeLog	2011-10-13 00:21:40 UTC (rev 97322)
@@ -1,3 +1,21 @@
+2011-10-12  Adam Barth  <[email protected]>
+
+        commit-queue doesn't have a friendly error message when the reviewer line is messed up
+        https://bugs.webkit.org/show_bug.cgi?id=69979
+
+        Reviewed by Eric Seidel.
+
+        Rather than combining the ChangeLog validation with a more complicated
+        command, this patch has the commit-queue run it as a separate command,
+        which will give us more control over the error message.
+
+        * Scripts/webkitpy/tool/bot/commitqueuetask.py:
+        * Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:
+        * Scripts/webkitpy/tool/commands/download.py:
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        * Scripts/webkitpy/tool/steps/validatechangelogs.py:
+        * Scripts/webkitpy/tool/steps/validatereviewer.py:
+
 2011-10-12  Eric Seidel  <[email protected]>
 
         Layout tests asserting in LayoutTestController::pathToLocalResource()

Modified: trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py (97321 => 97322)


--- trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py	2011-10-13 00:14:53 UTC (rev 97321)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py	2011-10-13 00:21:40 UTC (rev 97322)
@@ -47,10 +47,17 @@
             return False
         if self._patch.review() == "-":
             return False
-        # Reviewer is not required. Missing reviewers will be caught during
-        # the ChangeLog check during landing.
         return True
 
+    def _validate_changelog(self):
+        return self._run_command([
+            "validate-changelog",
+            "--non-interactive",
+            self._patch.id(),
+        ],
+        "ChangeLog validated",
+        "ChangeLog did not pass validation")
+
     def run(self):
         if not self.validate():
             return False
@@ -60,6 +67,8 @@
             return False
         if not self._apply():
             return self.report_failure()
+        if not self._validate_changelog():
+            return self.report_failure()
         if not self._patch.is_rollout():
             if not self._build():
                 if not self._build_without_patch():

Modified: trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py (97321 => 97322)


--- trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py	2011-10-13 00:14:53 UTC (rev 97321)
+++ trunk/Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py	2011-10-13 00:21:40 UTC (rev 97322)
@@ -133,6 +133,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
@@ -178,11 +180,30 @@
 """
         self._run_through_task(commit_queue, expected_stderr, GoldenScriptError)
 
+    def test_validate_changelog_failure(self):
+        commit_queue = MockCommitQueue([
+            None,
+            None,
+            None,
+            GoldenScriptError("MOCK validate failure"),
+        ])
+        expected_stderr = """run_webkit_patch: ['clean']
+command_passed: success_message='Cleaned working directory' patch='10000'
+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]
+command_failed: failure_message='ChangeLog did not pass validation' script_error='MOCK validate failure' patch='10000'
+"""
+        self._run_through_task(commit_queue, expected_stderr, GoldenScriptError)
+
     def test_build_failure(self):
         commit_queue = MockCommitQueue([
             None,
             None,
             None,
+            None,
             GoldenScriptError("MOCK build failure"),
         ])
         expected_stderr = """run_webkit_patch: ['clean']
@@ -191,6 +212,8 @@
 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]
+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'
 run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both']
@@ -203,6 +226,7 @@
             None,
             None,
             None,
+            None,
             ScriptError("MOCK build failure"),
             ScriptError("MOCK clean build failure"),
         ])
@@ -212,6 +236,8 @@
 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]
+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'
 run_webkit_patch: ['build', '--force-clean', '--no-update', '--build-style=both']
@@ -225,6 +251,7 @@
             None,
             None,
             None,
+            None,
             ScriptError("MOCK tests failure"),
         ])
         # CommitQueueTask will only report flaky tests if we successfully parsed
@@ -236,6 +263,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
@@ -255,6 +284,7 @@
             None,
             None,
             None,
+            None,
             ScriptError("MOCK tests failure"),
         ])
         commit_queue.layout_test_results = lambda: LayoutTestResults([])
@@ -267,6 +297,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
@@ -284,6 +316,7 @@
             None,
             None,
             None,
+            None,
             ScriptError("MOCK test failure"),
             ScriptError("MOCK test failure again"),
         ], [
@@ -300,6 +333,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
@@ -320,6 +355,7 @@
             None,
             None,
             None,
+            None,
             GoldenScriptError("MOCK test failure"),
             ScriptError("MOCK test failure again"),
         ])
@@ -329,6 +365,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
@@ -348,6 +386,7 @@
             None,
             None,
             None,
+            None,
             ScriptError("MOCK test failure"),
             ScriptError("MOCK test failure again"),
             ScriptError("MOCK clean test failure"),
@@ -365,6 +404,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
@@ -387,6 +428,7 @@
             None,
             None,
             None,
+            None,
             ScriptError("MOCK test failure"),
             ScriptError("MOCK test failure again"),
             ScriptError("MOCK clean test failure"),
@@ -405,6 +447,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
@@ -424,6 +468,7 @@
             None,
             None,
             None,
+            None,
             GoldenScriptError("MOCK test failure"),
             ScriptError("MOCK test failure again"),
             ScriptError("MOCK clean test failure"),
@@ -441,6 +486,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']
@@ -462,6 +509,7 @@
             None,
             None,
             None,
+            None,
             GoldenScriptError("MOCK land failure"),
         ])
         expected_stderr = """run_webkit_patch: ['clean']
@@ -470,6 +518,8 @@
 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]
+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'
 run_webkit_patch: ['build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']

Modified: trunk/Tools/Scripts/webkitpy/tool/commands/download.py (97321 => 97322)


--- trunk/Tools/Scripts/webkitpy/tool/commands/download.py	2011-10-13 00:14:53 UTC (rev 97321)
+++ trunk/Tools/Scripts/webkitpy/tool/commands/download.py	2011-10-13 00:21:40 UTC (rev 97322)
@@ -301,6 +301,18 @@
     show_in_main_help = True
 
 
+class ValidateChangelog(AbstractSequencedCommand):
+    name = "validate-changelog"
+    help_text = "Validate that the ChangeLogs and reviewers look reasonable"
+    long_help = """Examines the current diff to see whether the ChangeLogs
+and the reviewers listed in the ChangeLogs look reasonable.
+"""
+    steps = [
+        steps.ValidateChangeLogs,
+        steps.ValidateReviewer,
+    ]
+
+
 class AbstractRolloutPrepCommand(AbstractSequencedCommand):
     argument_names = "REVISION [REVISIONS] REASON"

Modified: trunk/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py (97321 => 97322)


--- trunk/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py	2011-10-13 00:14:53 UTC (rev 97321)
+++ trunk/Tools/Scripts/webkitpy/tool/commands/queues_unittest.py	2011-10-13 00:21:40 UTC (rev 97322)
@@ -242,6 +242,7 @@
             "process_work_item": """MOCK: update_status: commit-queue Cleaned working directory
 MOCK: update_status: commit-queue Updated working directory
 MOCK: update_status: commit-queue Applied patch
+MOCK: update_status: commit-queue ChangeLog validated
 MOCK: update_status: commit-queue Built patch
 MOCK: update_status: commit-queue Passed tests
 MOCK: update_status: commit-queue Landed patch
@@ -326,6 +327,8 @@
 MOCK: update_status: commit-queue Updated working directory
 MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--no-update', '--non-interactive', 10000], cwd=/mock-checkout
 MOCK: update_status: commit-queue Applied patch
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'validate-changelog', '--non-interactive', 10000], cwd=/mock-checkout
+MOCK: update_status: commit-queue ChangeLog validated
 MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build', '--no-clean', '--no-update', '--build-style=both'], cwd=/mock-checkout
 MOCK: update_status: commit-queue Built patch
 MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive'], cwd=/mock-checkout
@@ -355,6 +358,8 @@
 MOCK: update_status: commit-queue Updated working directory
 MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'apply-attachment', '--no-update', '--non-interactive', 10005], cwd=/mock-checkout
 MOCK: update_status: commit-queue Applied patch
+MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'validate-changelog', '--non-interactive', 10005], cwd=/mock-checkout
+MOCK: update_status: commit-queue ChangeLog validated
 MOCK run_and_throw_if_fail: ['echo', '--status-host=example.com', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 10005], cwd=/mock-checkout
 MOCK: update_status: commit-queue Landed patch
 MOCK: update_status: commit-queue Pass
@@ -389,6 +394,7 @@
         expected_stderr = """MOCK: update_status: commit-queue Cleaned working directory
 MOCK: update_status: commit-queue Updated working directory
 MOCK: update_status: commit-queue Applied patch
+MOCK: update_status: commit-queue ChangeLog validated
 MOCK: update_status: commit-queue Built patch
 MOCK: update_status: commit-queue Passed tests
 MOCK: update_status: commit-queue Retry

Modified: trunk/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py (97321 => 97322)


--- trunk/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py	2011-10-13 00:14:53 UTC (rev 97321)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/validatechangelogs.py	2011-10-13 00:21:40 UTC (rev 97322)
@@ -35,6 +35,12 @@
 # This is closely related to the ValidateReviewer step and the CommitterValidator class.
 # We may want to unify more of this code in one place.
 class ValidateChangeLogs(AbstractStep):
+    @classmethod
+    def options(cls):
+        return AbstractStep.options() + [
+            Options.non_interactive,
+        ]
+
     def _check_changelog_diff(self, diff_file):
         if not self._tool.checkout().is_path_to_changelog(diff_file.filename):
             return True

Modified: trunk/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py (97321 => 97322)


--- trunk/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py	2011-10-13 00:14:53 UTC (rev 97321)
+++ trunk/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py	2011-10-13 00:21:40 UTC (rev 97322)
@@ -37,6 +37,12 @@
 
 # FIXME: Some of this logic should probably be unified with CommitterValidator?
 class ValidateReviewer(AbstractStep):
+    @classmethod
+    def options(cls):
+        return AbstractStep.options() + [
+            Options.non_interactive,
+        ]
+
     # FIXME: This should probably move onto ChangeLogEntry
     def _has_valid_reviewer(self, changelog_entry):
         if changelog_entry.reviewer():

_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to