From cbf61c704488b388f40f9d2873ae5c82e44ab2a9 Mon Sep 17 00:00:00 2001 From: Yiwei Zhang Date: Fri, 7 Mar 2025 09:46:45 -0800 Subject: [PATCH] CheckNewDEPSHooksHasRequiredReviewers checks approval from required reviewers The check mandates approval from required reviewers during submission. Without it, the CL author could land the CL without explicitly approval from required reviewers. Bug: 396736534 Change-Id: I4671f123419b8f94a969b6caccf13a2064511a0b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6313625 Commit-Queue: Josip Sokcevic Auto-Submit: Yiwei Zhang Reviewed-by: Josip Sokcevic --- presubmit_canned_checks.py | 19 ++++++++------ tests/presubmit_canned_checks_test.py | 37 ++++++++++++++++++++++++--- 2 files changed, 44 insertions(+), 12 deletions(-) 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,