diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index fbf465ab6..45870961a 100644 --- a/presubmit_canned_checks.py +++ b/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.""" diff --git a/testing_support/presubmit_canned_checks_test_mocks.py b/testing_support/presubmit_canned_checks_test_mocks.py index 9dc01e0ea..4d2010ff1 100644 --- a/testing_support/presubmit_canned_checks_test_mocks.py +++ b/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 diff --git a/tests/presubmit_canned_checks_test.py b/tests/presubmit_canned_checks_test.py old mode 100644 new mode 100755 index 96549f82e..4f6df0feb --- a/tests/presubmit_canned_checks_test.py +++ b/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()