Commit 3cd00c86 authored by abarth@webkit.org's avatar abarth@webkit.org
Browse files

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:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@97322 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 02a046a3
2011-10-12 Adam Barth <abarth@webkit.org>
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 <eric@webkit.org>
 
Layout tests asserting in LayoutTestController::pathToLocalResource()
......@@ -47,10 +47,17 @@ class CommitQueueTask(PatchAnalysisTask):
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 @@ class CommitQueueTask(PatchAnalysisTask):
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():
......
......@@ -133,6 +133,8 @@ 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_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']
......@@ -175,6 +177,24 @@ run_webkit_patch: ['update']
command_passed: success_message='Updated working directory' patch='10000'
run_webkit_patch: ['apply-attachment', '--no-update', '--non-interactive', 10000]
command_failed: failure_message='Patch does not apply' script_error='MOCK apply failure' patch='10000'
"""
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)
......@@ -183,6 +203,7 @@ command_failed: failure_message='Patch does not apply' script_error='MOCK apply
None,
None,
None,
None,
GoldenScriptError("MOCK build failure"),
])
expected_stderr = """run_webkit_patch: ['clean']
......@@ -191,6 +212,8 @@ 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_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 @@ command_passed: success_message='Able to build without patch' patch='10000'
None,
None,
None,
None,
ScriptError("MOCK build failure"),
ScriptError("MOCK clean build failure"),
])
......@@ -212,6 +236,8 @@ 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_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 @@ command_failed: failure_message='Unable to build without patch' script_error='MO
None,
None,
None,
None,
ScriptError("MOCK tests failure"),
])
# CommitQueueTask will only report flaky tests if we successfully parsed
......@@ -236,6 +263,8 @@ 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_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 @@ command_passed: success_message='Landed patch' patch='10000'
None,
None,
None,
None,
ScriptError("MOCK tests failure"),
])
commit_queue.layout_test_results = lambda: LayoutTestResults([])
......@@ -267,6 +297,8 @@ 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_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 @@ command_passed: success_message='Landed patch' patch='10000'
None,
None,
None,
None,
ScriptError("MOCK test failure"),
ScriptError("MOCK test failure again"),
], [
......@@ -300,6 +333,8 @@ 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_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 @@ command_failed: failure_message='Patch does not pass tests' script_error='MOCK t
None,
None,
None,
None,
GoldenScriptError("MOCK test failure"),
ScriptError("MOCK test failure again"),
])
......@@ -329,6 +365,8 @@ 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_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 @@ command_passed: success_message='Able to pass tests without patch' patch='10000'
None,
None,
None,
None,
ScriptError("MOCK test failure"),
ScriptError("MOCK test failure again"),
ScriptError("MOCK clean test failure"),
......@@ -365,6 +404,8 @@ 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_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 @@ command_passed: success_message='Landed patch' patch='10000'
None,
None,
None,
None,
ScriptError("MOCK test failure"),
ScriptError("MOCK test failure again"),
ScriptError("MOCK clean test failure"),
......@@ -405,6 +447,8 @@ 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_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 @@ command_failed: failure_message='Unable to pass tests without patch (tree is red
None,
None,
None,
None,
GoldenScriptError("MOCK test failure"),
ScriptError("MOCK test failure again"),
ScriptError("MOCK clean test failure"),
......@@ -441,6 +486,8 @@ 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_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 @@ command_failed: failure_message='Unable to pass tests without patch (tree is red
None,
None,
None,
None,
GoldenScriptError("MOCK land failure"),
])
expected_stderr = """run_webkit_patch: ['clean']
......@@ -470,6 +518,8 @@ 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_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']
......
......@@ -301,6 +301,18 @@ class LandFromBug(AbstractPatchLandingCommand, ProcessBugsMixin):
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"
......
......@@ -242,6 +242,7 @@ class CommitQueueTest(QueuesTest):
"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 run_and_throw_if_fail: ['echo', '--status-host=example.com', 'update'], cwd
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 run_and_throw_if_fail: ['echo', '--status-host=example.com', 'update'], cwd
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 @@ MOCK: release_work_item: commit-queue 10005
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
......
......@@ -35,6 +35,12 @@ from webkitpy.common.system.deprecated_logging import error, log
# 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
......
......@@ -37,6 +37,12 @@ from webkitpy.common.system.deprecated_logging import error, log
# 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():
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment