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 <sokcevic@chromium.org>
Auto-Submit: Yiwei Zhang <yiwzhang@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

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)]

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,