diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 137358a79ad62d..2c400e8ea66839 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -3582,8 +3582,33 @@ def StripDeps(path): else: return path + submodule_paths = set(input_api.ListSubmodules()) + def is_from_submodules(path, submodule_paths): + path = input_api.os_path.normpath(path) + while path: + if path in submodule_paths: + return True + + # All deps should be a relative path from the checkout. + # i.e., shouldn't start with "/" or "c:\", for example. + # + # That said, this is to prevent an infinite loop, just in case + # an input dep path starts with "/", because + # os.path.dirname("/") => "/" + parent = input_api.os_path.dirname(path) + if parent == path: + break + path = parent + + return False + unapproved_dependencies = [ "'+%s'," % StripDeps(path) for path in missing_files + # if a newly added dep is from a submodule, it becomes trickier + # to get suggested owners, especially it is from a different host. + # + # skip the review enforcement for cross-repo deps. + if not is_from_submodules(path, submodule_paths) ] if unapproved_dependencies: diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py index b5841f6fce386a..d96eeb84ec5e14 100755 --- a/PRESUBMIT_test.py +++ b/PRESUBMIT_test.py @@ -424,6 +424,88 @@ def testCalculateAddedDepsIgnoresPermutations(self): self.calculate(old_include_rules, {}, new_include_rules, {})) + class FakeOwnersClient(object): + APPROVED = "APPROVED" + PENDING = "PENDING" + returns = {} + + def ListOwners(self, *args, **kwargs): + return self.returns.get(self.ListOwners.__name__, "") + + def mockListOwners(self, owners): + self.returns[self.ListOwners.__name__] = owners + + def GetFilesApprovalStatus(self, *args, **kwargs): + return self.returns.get(self.GetFilesApprovalStatus.__name__, {}) + + def mockGetFilesApprovalStatus(self, status): + self.returns[self.GetFilesApprovalStatus.__name__] = status + + def SuggestOwners(self, *args, **kwargs): + return ["eng1", "eng2", "eng3"] + + class fakeGerrit(object): + def IsOwnersOverrideApproved(self, issue): + return False + + def setUp(self): + self.input_api = input_api = MockInputApi() + input_api.environ = {} + input_api.owners_client = self.FakeOwnersClient() + input_api.gerrit = self.fakeGerrit() + input_api.change.issue = 123 + self.mockOwnersAndReviewers("owner", set(["reviewer"])) + self.mockListSubmodules([]) + + def mockOwnersAndReviewers(self, owner, reviewers): + def mock(*args, **kwargs): + return [owner, reviewers] + self.input_api.canned_checks.GetCodereviewOwnerAndReviewers = mock + + def mockListSubmodules(self, paths): + def mock(*args, **kwargs): + return paths + self.input_api.ListSubmodules = mock + + def testApprovedAdditionalDep(self): + old_deps = """include_rules = []""".splitlines() + new_deps = """include_rules = ["+v8/123"]""".splitlines() + self.input_api.files = [MockAffectedFile("pdf/DEPS", new_deps, old_deps)] + + # mark the additional dep as approved. + os_path = self.input_api.os_path + self.input_api.owners_client.mockGetFilesApprovalStatus( + {os_path.join('v8/123', 'DEPS'): self.FakeOwnersClient.APPROVED} + ) + results = PRESUBMIT.CheckAddedDepsHaveTargetApprovals( + self.input_api, MockOutputApi()) + # Then, the check should pass. + self.assertEqual([], results) + + def testUnapprovedAdditionalDep(self): + old_deps = """include_rules = []""".splitlines() + new_deps = """include_rules = ["+v8/123"]""".splitlines() + self.input_api.files = [ + MockAffectedFile('pdf/DEPS', new_deps, old_deps), + ] + + # pending. + os_path = self.input_api.os_path + self.input_api.owners_client.mockGetFilesApprovalStatus( + {os_path.join('v8/123', 'DEPS'): self.FakeOwnersClient.PENDING} + ) + results = PRESUBMIT.CheckAddedDepsHaveTargetApprovals( + self.input_api, MockOutputApi()) + # the check should fail + self.assertIn('You need LGTM', results[0].message) + self.assertIn('+v8/123', results[0].message) + + # unless the added dep is from a submodule. + self.mockListSubmodules(['v8']) + results = PRESUBMIT.CheckAddedDepsHaveTargetApprovals( + self.input_api, MockOutputApi()) + self.assertEqual([], results) + class JSONParsingTest(unittest.TestCase): def testSuccess(self):