diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index cf358b10c..0981f1abe 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -1167,17 +1167,16 @@ def CheckOwners( affected_files = set([f.LocalPath() for f in input_api.change.AffectedFiles(file_filter=source_file_filter)]) - owners_db = input_api.owners_db - owners_db.override_files = input_api.change.OriginalOwnersFiles() owner_email, reviewers = GetCodereviewOwnerAndReviewers( - input_api, - owners_db.email_regexp, - approval_needed=input_api.is_committing) + input_api, None, approval_needed=input_api.is_committing) owner_email = owner_email or input_api.change.author_email - missing_files = owners_db.files_not_covered_by( - affected_files, reviewers.union([owner_email])) + approval_status = input_api.owners_client.GetFilesApprovalStatus( + affected_files, reviewers.union([owner_email]), []) + missing_files = [ + f for f in affected_files + if approval_status[f] != input_api.owners_client.APPROVED] affects_owners = any('OWNERS' in name for name in missing_files) if input_api.is_committing: @@ -1206,7 +1205,7 @@ def CheckOwners( if tbr and affects_owners: output_list.append(output_fn('TBR for OWNERS files are ignored.')) if not input_api.is_committing: - suggested_owners = owners_db.reviewers_for(missing_files, owner_email) + suggested_owners = input_api.owners_client.SuggestOwners(missing_files) output_list.append(output_fn('Suggested OWNERS: ' + '(Use "git-cl owners" to interactively select owners.)\n %s' % ('\n '.join(suggested_owners)))) @@ -1217,12 +1216,14 @@ def CheckOwners( return [] -def GetCodereviewOwnerAndReviewers(input_api, email_regexp, approval_needed): +def GetCodereviewOwnerAndReviewers(input_api, _email_regexp, approval_needed): """Return the owner and reviewers of a change, if any. If approval_needed is True, only reviewers who have approved the change will be returned. """ + # Recognizes 'X@Y' email addresses. Very simplistic. + EMAIL_REGEXP = input_api.re.compile(r'^[\w\-\+\%\.]+\@[\w\-\+\%\.]+$') issue = input_api.change.issue if not issue: return None, (set() if approval_needed else @@ -1231,7 +1232,7 @@ def GetCodereviewOwnerAndReviewers(input_api, email_regexp, approval_needed): owner_email = input_api.gerrit.GetChangeOwner(issue) reviewers = set( r for r in input_api.gerrit.GetChangeReviewers(issue, approval_needed) - if _match_reviewer_email(r, owner_email, email_regexp)) + if _match_reviewer_email(r, owner_email, EMAIL_REGEXP)) input_api.logging.debug('owner: %s; approvals given by: %s', owner_email, ', '.join(sorted(reviewers))) return owner_email, reviewers diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 6588fcf70..d745c7c40 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -44,6 +44,7 @@ import git_cl import git_common as git import json import owners +import owners_client import owners_finder import presubmit_support as presubmit import rdb_wrapper @@ -2693,90 +2694,67 @@ the current line as well! self.assertEqual(1, len(results)) self.assertIsInstance(results[0], presubmit.OutputApi.PresubmitError) - def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None, - reviewers=None, is_committing=True, - response=None, uncovered_files=None, expected_output='', - manually_specified_reviewers=None, dry_run=None, - modified_file='foo/xyz.cc', allow_tbr=True): - if approvers is None: - # The set of people who lgtm'ed a change. - approvers = set() - if reviewers is None: - # The set of people needed to lgtm a change. We default to - # the same list as the people who approved it. We use 'reviewers' - # to avoid a name collision w/ owners.py. - reviewers = approvers - if uncovered_files is None: - uncovered_files = set() - if manually_specified_reviewers is None: - manually_specified_reviewers = [] + def AssertOwnersWorks( + self, tbr=False, issue='1', approvers=None, modified_files=None, + owners_by_path=None, is_committing=True, response=None, + expected_output='', manually_specified_reviewers=None, dry_run=None, + allow_tbr=True): + # The set of people who lgtm'ed a change. + approvers = approvers or set() + manually_specified_reviewers = manually_specified_reviewers or [] + modified_files = modified_files or ['foo/xyz.cc'] + owners_by_path = owners_by_path or {'foo/xyz.cc': ['john@example.com']} + response = response or { + "owner": {"email": 'john@example.com'}, + "labels": {"Code-Review": { + u'all': [ + { + u'email': a, + u'value': +1 + } for a in approvers + ], + u'default_value': 0, + u'values': {u' 0': u'No score', + u'+1': u'Looks good to me', + u'-1': u"I would prefer that you didn't submit this"} + }}, + "reviewers": {"REVIEWER": [{u'email': a}] for a in approvers}, + } change = mock.MagicMock(presubmit.Change) - change.issue = issue + change.OriginalOwnersFiles.return_value = {} + change.RepositoryRoot.return_value = None + change.ReviewersFromDescription.return_value = manually_specified_reviewers + change.TBRsFromDescription.return_value = [] change.author_email = 'john@example.com' - change.ReviewersFromDescription = lambda: manually_specified_reviewers - change.TBRsFromDescription = lambda: [] - change.RepositoryRoot = lambda: None - affected_file = mock.MagicMock(presubmit.GitAffectedFile) - input_api = self.MockInputApi(change, False) - input_api.gerrit = presubmit.GerritAccessor('host') + change.issue = issue - fake_db = mock.MagicMock(owners.Database) - fake_db.email_regexp = input_api.re.compile(owners.BASIC_EMAIL_REGEXP) - fake_db.files_not_covered_by.return_value = uncovered_files + affected_files = [] + for f in modified_files: + affected_file = mock.MagicMock(presubmit.GitAffectedFile) + affected_file.LocalPath.return_value = f + affected_files.append(affected_file) + change.AffectedFiles.return_value = affected_files - input_api.owners_db = fake_db + input_api = self.MockInputApi(change, False) + input_api.gerrit = presubmit.GerritAccessor('host') input_api.is_committing = is_committing input_api.tbr = tbr input_api.dry_run = dry_run + input_api.gerrit._FetchChangeDetail = lambda _: response - affected_file.LocalPath.return_value = modified_file - change.AffectedFiles.return_value = [affected_file] - if not is_committing or issue or ('OWNERS' in modified_file): - change.OriginalOwnersFiles.return_value = {} - if issue and not response: - response = { - "owner": {"email": change.author_email}, - "labels": {"Code-Review": { - u'all': [ - { - u'email': a, - u'value': +1 - } for a in approvers - ], - u'default_value': 0, - u'values': {u' 0': u'No score', - u'+1': u'Looks good to me', - u'-1': u"I would prefer that you didn't submit this"} - }}, - "reviewers": {"REVIEWER": [{u'email': a}] for a in approvers}, - } - - if is_committing: - people = approvers - else: - people = reviewers - - if issue: - input_api.gerrit._FetchChangeDetail = lambda _: response - - people.add(change.author_email) - if not is_committing and uncovered_files: - fake_db.reviewers_for.return_value = change.author_email - - results = presubmit_canned_checks.CheckOwners( - input_api, presubmit.OutputApi, allow_tbr=allow_tbr) + input_api.owners_client = owners_client.DepotToolsClient('root', 'branch') + + with mock.patch('owners_client.DepotToolsClient.ListOwners', + side_effect=lambda f: owners_by_path.get(f, [])): + results = presubmit_canned_checks.CheckOwners( + input_api, presubmit.OutputApi, allow_tbr=allow_tbr) for result in results: result.handle() if expected_output: self.assertRegexpMatches(sys.stdout.getvalue(), expected_output) else: self.assertEqual(sys.stdout.getvalue(), expected_output) - - files_not_covered_by_calls = fake_db.files_not_covered_by.mock_calls - if len(files_not_covered_by_calls): - actual_reviewers = fake_db.files_not_covered_by.mock_calls[0][1][1] - self.assertIn(change.author_email, actual_reviewers) sys.stdout.truncate(0) def testCannedCheckOwners_DryRun(self): @@ -2797,15 +2775,16 @@ the current line as well! }}, "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]}, } - self.AssertOwnersWorks(approvers=set(), + self.AssertOwnersWorks( + approvers=set(), dry_run=True, response=response, - reviewers=set(["ben@example.com"]), expected_output='This is a dry run, but these failures would be ' + 'reported on commit:\nMissing LGTM from someone ' + 'other than john@example.com\n') - self.AssertOwnersWorks(approvers=set(['ben@example.com']), + self.AssertOwnersWorks( + approvers=set(['ben@example.com']), is_committing=False, response=response, expected_output='') @@ -2827,12 +2806,16 @@ the current line as well! }}, "reviewers": {"REVIEWER": [{u'email': u'bot@example.com'}]}, } - self.AssertOwnersWorks(approvers=set(), + self.AssertOwnersWorks( + approvers=set(), + modified_files={'foo/xyz.cc': ['john@example.com']}, response=response, is_committing=True, expected_output='') - self.AssertOwnersWorks(approvers=set(), + self.AssertOwnersWorks( + approvers=set(), + modified_files={'foo/xyz.cc': ['john@example.com']}, is_committing=False, response=response, expected_output='') @@ -2854,12 +2837,14 @@ the current line as well! }}, "reviewers": {"REVIEWER": [{u'email': u'sheriff@example.com'}]}, } - self.AssertOwnersWorks(approvers=set(), + self.AssertOwnersWorks( + approvers=set(), response=response, is_committing=True, expected_output='') - self.AssertOwnersWorks(approvers=set(), + self.AssertOwnersWorks( + approvers=set(), is_committing=False, response=response, expected_output='') @@ -2888,12 +2873,14 @@ the current line as well! }}, "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]}, } - self.AssertOwnersWorks(approvers=set(['ben@example.com']), + self.AssertOwnersWorks( + approvers=set(['ben@example.com']), response=response, is_committing=True, expected_output='') - self.AssertOwnersWorks(approvers=set(['ben@example.com']), + self.AssertOwnersWorks( + approvers=set(['ben@example.com']), is_committing=False, response=response, expected_output='') @@ -2916,7 +2903,8 @@ the current line as well! }}, "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]}, } - self.AssertOwnersWorks(approvers=set(['ben@example.com']), + self.AssertOwnersWorks( + approvers=set(['ben@example.com']), response=response, is_committing=True, expected_output='') @@ -2947,7 +2935,6 @@ the current line as well! } self.AssertOwnersWorks( approvers=set(), - reviewers=set(["ben@example.com"]), response=response, is_committing=True, expected_output= @@ -2955,7 +2942,6 @@ the current line as well! self.AssertOwnersWorks( approvers=set(), - reviewers=set(["ben@example.com"]), is_committing=False, response=response, expected_output='') @@ -2980,7 +2966,6 @@ the current line as well! } self.AssertOwnersWorks( approvers=set(), - reviewers=set(["ben@example.com"]), response=response, is_committing=True, expected_output= @@ -2999,14 +2984,12 @@ the current line as well! } self.AssertOwnersWorks( approvers=set(), - reviewers=set(), response=response, expected_output= 'Missing LGTM from someone other than john@example.com\n') self.AssertOwnersWorks( approvers=set(), - reviewers=set(), is_committing=False, response=response, expected_output='') @@ -3020,36 +3003,37 @@ the current line as well! def testCannedCheckOwners_NoIssue(self): self.AssertOwnersWorks(issue=None, - uncovered_files=set(['foo']), + modified_files=['foo'], expected_output="OWNERS check failed: this CL has no Gerrit " "change number, so we can't check it for approvals.\n") self.AssertOwnersWorks(issue=None, is_committing=False, - uncovered_files=set(['foo']), + modified_files=['foo'], expected_output=re.compile( 'Missing OWNER reviewers for these files:\n' ' foo\n', re.MULTILINE)) def testCannedCheckOwners_NoIssueLocalReviewers(self): - self.AssertOwnersWorks(issue=None, - reviewers=set(['jane@example.com']), + self.AssertOwnersWorks( + issue=None, manually_specified_reviewers=['jane@example.com'], expected_output="OWNERS check failed: this CL has no Gerrit " "change number, so we can't check it for approvals.\n") - self.AssertOwnersWorks(issue=None, - reviewers=set(['jane@example.com']), + self.AssertOwnersWorks( + issue=None, manually_specified_reviewers=['jane@example.com'], is_committing=False, expected_output='') def testCannedCheckOwners_NoIssueLocalReviewersDontInferEmailDomain(self): - self.AssertOwnersWorks(issue=None, - reviewers=set(['jane']), + self.AssertOwnersWorks( + issue=None, manually_specified_reviewers=['jane@example.com'], expected_output="OWNERS check failed: this CL has no Gerrit " "change number, so we can't check it for approvals.\n") - self.AssertOwnersWorks(issue=None, - uncovered_files=set(['foo']), + self.AssertOwnersWorks( + issue=None, + modified_files=['foo'], manually_specified_reviewers=['jane'], is_committing=False, expected_output=re.compile( @@ -3089,8 +3073,7 @@ the current line as well! def testCannedCheckOwners_TBROWNERSFile(self): self.AssertOwnersWorks( tbr=True, - uncovered_files=set(['foo/OWNERS']), - modified_file='foo/OWNERS', + modified_files=['foo/OWNERS'], expected_output='Missing LGTM from an OWNER for these files:\n' ' foo/OWNERS\n' 'TBR for OWNERS files are ignored.\n') @@ -3098,26 +3081,27 @@ the current line as well! def testCannedCheckOwners_TBRNonOWNERSFile(self): self.AssertOwnersWorks( tbr=True, - uncovered_files=set(['foo/xyz.cc']), - modified_file='foo/OWNERS', + modified_files=['foo/OWNERS', 'foo/xyz.cc'], + owners_by_path={'foo/OWNERS': ['john@example.com'], + 'foo/xyz.cc': []}, expected_output='--tbr was specified, skipping OWNERS check\n') def testCannedCheckOwners_WithoutOwnerLGTM(self): - self.AssertOwnersWorks(uncovered_files=set(['foo']), + self.AssertOwnersWorks( + modified_files=['foo'], expected_output='Missing LGTM from an OWNER for these files:\n' ' foo\n') - self.AssertOwnersWorks(uncovered_files=set(['foo']), + self.AssertOwnersWorks( + modified_files=['foo'], is_committing=False, expected_output=re.compile( 'Missing OWNER reviewers for these files:\n' ' foo\n', re.MULTILINE)) def testCannedCheckOwners_WithLGTMs(self): + self.AssertOwnersWorks(approvers=set(['ben@example.com'])) self.AssertOwnersWorks(approvers=set(['ben@example.com']), - uncovered_files=set()) - self.AssertOwnersWorks(approvers=set(['ben@example.com']), - is_committing=False, - uncovered_files=set()) + is_committing=False) @mock.patch(BUILTIN_OPEN, mock.mock_open()) def testCannedRunUnitTests(self):