diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 16c6fa308..204c503b6 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -2304,16 +2304,19 @@ def CheckNewDEPSHooksHasRequiredReviewers(input_api, output_api): if not required_reviewers: return [] - # TODO - crbug/396736534: return error if CL is not approved by the - # required reviewers during submission. + submitting = input_api.is_committing and not input_api.dry_run reviewers = input_api.gerrit.GetChangeReviewers(input_api.change.issue, - approving_only=False) - + approving_only=submitting) if set(r for r in reviewers if r in required_reviewers): - return [] # At least one required reviewer is present. - msg = (f'New DEPS {"hook" if len(new_hooks-old_hooks) == 1 else "hooks"} ' - f'({", ".join(sorted(new_hooks-old_hooks))}) are found. Please add ' - 'one of the following reviewers:') + return [] # passing the check + added_hooks = new_hooks - old_hooks + msg = (f'New DEPS {"hook" if len(added_hooks) == 1 else "hooks"} ' + f'({", ".join(sorted(new_hooks-old_hooks))}) ' + f'{"is" if len(added_hooks) == 1 else "are"} found. ') + if submitting: + msg += 'The CL must be approved by one of the following reviewers:' + else: + msg += 'Please request review from one of the following reviewers:' for r in required_reviewers: msg += f'\n * {r}' return [output_api.PresubmitError(msg)] diff --git a/tests/presubmit_canned_checks_test.py b/tests/presubmit_canned_checks_test.py index 985e06d12..a9be5a32a 100755 --- a/tests/presubmit_canned_checks_test.py +++ b/tests/presubmit_canned_checks_test.py @@ -567,16 +567,31 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase): }, { 'name': - 'add new hook but reviewer missing', + 'add new hook and require review', 'old_contents': ['hooks = [{"name": "old_hook"}]'], 'new_contents': [ 'hooks = [{"name": "old_hook"}, {"name": "new_hook"}, {"name": "new_hook_2"}]' ], 'reviewers': [], 'expected_error_msg': - 'New DEPS hooks (new_hook, new_hook_2) are found. Please add ' - 'one of the following reviewers:\n * foo@chromium.org\n ' - '* bar@chromium.org\n * baz@chromium.org' + 'New DEPS hooks (new_hook, new_hook_2) are found. Please ' + 'request review from one of the following reviewers:\n ' + '* foo@chromium.org\n * bar@chromium.org\n * baz@chromium.org' + }, + { + 'name': + 'add new hook and require approval', + 'old_contents': ['hooks = [{"name": "old_hook"}]'], + 'new_contents': [ + 'hooks = [{"name": "old_hook"}, {"name": "new_hook"}, {"name": "new_hook_2"}]' + ], + 'submitting': + True, + 'reviewers': ['not_relevant@chromium.org'], + 'expected_error_msg': + 'New DEPS hooks (new_hook, new_hook_2) are found. The CL must ' + 'be approved by one of the following reviewers:\n' + ' * foo@chromium.org\n * bar@chromium.org\n * baz@chromium.org' }, { 'name': @@ -587,6 +602,17 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase): ], 'reviewers': ['baz@chromium.org'], }, + { + 'name': + 'add new hook and reviewer already approves', + 'old_contents': ['hooks = [{"name": "old_hook"}]'], + 'new_contents': [ + 'hooks = [{"name": "old_hook"}, {"name": "new_hook"}, {"name": "new_hook_2"}]' + ], + 'submitting': + True, + 'reviewers': ['foo@chromium.org'], + }, { 'name': 'change existing hook', @@ -618,6 +644,9 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase): old_contents=case['old_contents'], new_contents=case['new_contents']), ] + if case.get('submitting', False): + self.input_api.is_committing = True + self.input_api.dry_run = False gerrit_mock.GetChangeReviewers.return_value = case['reviewers'] results = presubmit_canned_checks.CheckNewDEPSHooksHasRequiredReviewers( self.input_api,