Add CheckNewDEPSHooksHasRequiredReviewers canned check

This will later be used in chromium PRESUBMIT to ensure newly added DEPS
hook MUST be reviewed by a dedicated reviewer.

Bug: 396736534
Change-Id: I3859814c6316d4e576d12114277671ed5db80e33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6284565
Commit-Queue: Yiwei Zhang <yiwzhang@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@google.com>

presubmit_canned_checks.py

@@ -2221,6 +2221,43 @@ def CheckNoNewGitFilesAddedInDependencies(input_api, output_api):
     return errors
 
 
+def CheckNewDEPSHooksHasRequiredReviewers(input_api, output_api,
+                                          required_reviewers: list[str]):
+    """Ensure CL to add new DEPS hook(s) has at least one required reviewer."""
+    if not required_reviewers:
+        raise ValueError('required_reviewers must be non-empty')
+    if not input_api.change.issue:
+        return []  # Gerrit CL not yet uploaded.
+    deps_affected_files = [
+        f for f in input_api.AffectedFiles() if f.LocalPath() == 'DEPS'
+    ]
+    if not deps_affected_files:
+        return []  # Not a DEPS change.
+    deps_affected_file = deps_affected_files[0]
+
+    def _get_hooks_names(dep_contents):
+        deps = _ParseDeps('\n'.join(dep_contents))
+        hooks = deps.get('hooks', [])
+        return set(hook.get('name') for hook in hooks)
+
+    old_hooks = _get_hooks_names(deps_affected_file.OldContents())
+    new_hooks = _get_hooks_names(deps_affected_file.NewContents())
+    if new_hooks.issubset(old_hooks):
+        return []  # No new hooks added.
+
+    reviewers = input_api.gerrit.GetChangeReviewers(input_api.change.issue,
+                                                    approving_only=False)
+
+    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:')
+    for r in required_reviewers:
+        msg += f'\n * {r}'
+    return [output_api.PresubmitError(msg)]
+
+
 @functools.lru_cache(maxsize=None)
 def _ParseDeps(contents):
     """Simple helper for parsing DEPS files."""

testing_support/presubmit_canned_checks_test_mocks.py

@@ -258,10 +258,12 @@ class MockChange(object):
     This class can be used in presubmit unittests to mock the query of the
     current change.
     """
-    def __init__(self, changed_files, description=''):
+
+    def __init__(self, changed_files, description='', issue=0):
         self._changed_files = changed_files
         self.footers = defaultdict(list)
         self._description = description
+        self.issue = issue
 
     def LocalPaths(self):
         return self._changed_files

tests/presubmit_canned_checks_test.py

@@ -534,5 +534,104 @@ class CheckNoNewGitFilesAddedInDependenciesTest(unittest.TestCase):
         self.assertEqual(0, len(results))
 
 
+class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase):
+
+    def setUp(self):
+        self.input_api = MockInputApi()
+        self.input_api.change = MockChange([], issue=123)
+        self.input_api.files = [
+            MockAffectedFile('DEPS', 'content'),
+        ]
+
+    def test_no_gerrit_cl(self):
+        self.input_api.change = MockChange([], issue=None)
+        results = presubmit_canned_checks.CheckNewDEPSHooksHasRequiredReviewers(
+            self.input_api,
+            MockOutputApi(),
+            required_reviewers=['foo@chromium.org'])
+        self.assertEqual(0, len(results))
+
+    def test_no_deps_file_change(self):
+        self.input_api.files = [
+            MockAffectedFile('foo.py', 'content'),
+        ]
+        results = presubmit_canned_checks.CheckNewDEPSHooksHasRequiredReviewers(
+            self.input_api,
+            MockOutputApi(),
+            required_reviewers=['foo@chromium.org'])
+        self.assertEqual(0, len(results))
+
+    def test_new_deps_hook(self):
+        gerrit_mock = mock.Mock()
+        self.input_api.gerrit = gerrit_mock
+        test_cases = [
+            {
+                'name': 'no new hooks',
+                'old_contents': ['hooks = []'],
+                'new_contents': ['hooks = []'],
+                'reviewers': [],
+            },
+            {
+                'name':
+                'add new hook but reviewer missing',
+                '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'
+            },
+            {
+                'name':
+                'add new hook and reviewer is already added',
+                'old_contents': ['hooks = [{"name": "old_hook"}]'],
+                'new_contents': [
+                    'hooks = [{"name": "old_hook"}, {"name": "new_hook"},  {"name": "new_hook_2"}]'
+                ],
+                'reviewers': ['foo@chromium.org'],
+            },
+            {
+                'name':
+                'change existing hook',
+                'old_contents': [
+                    'hooks = [{"name": "existing_hook", "action": ["run", "./test.sh"]}]'
+                ],
+                'new_contents': [
+                    'hooks = [{"name": "existing_hook", "action": ["run", "./test_v2.sh"]}]'
+                ],
+                'reviewers': [],
+            },
+            {
+                'name':
+                'remove hook',
+                'old_contents':
+                ['hooks = [{"name": "old_hook"}, {"name": "hook_to_remove"}]'],
+                'new_contents': ['hooks = [{"name": "old_hook"}]'],
+                'reviewers': [],
+            },
+        ]
+        for case in test_cases:
+            with self.subTest(case_name=case['name']):
+                self.input_api.files = [
+                    MockAffectedFile('DEPS',
+                                     old_contents=case['old_contents'],
+                                     new_contents=case['new_contents']),
+                ]
+                gerrit_mock.GetChangeReviewers.return_value = case['reviewers']
+                results = presubmit_canned_checks.CheckNewDEPSHooksHasRequiredReviewers(
+                    self.input_api,
+                    MockOutputApi(),
+                    required_reviewers=['foo@chromium.org', 'bar@chromium.org'])
+                if 'expected_error_msg' in case:
+                    self.assertEqual(1, len(results))
+                    self.assertEqual(case['expected_error_msg'],
+                                     results[0].message)
+                else:
+                    self.assertEqual(0, len(results))
+
+
 if __name__ == '__main__':
     unittest.main()