diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index a48a0c627..878ee70a6 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -859,7 +859,9 @@ def _ReviewersFromChange(change): reviewers.update(set([r.strip() for r in change.R.split(',')])) if change.TBR: reviewers.update(set([r.strip() for r in change.TBR.split(',')])) - return reviewers + + # Drop reviewers that aren't specified in email address format. + return set(reviewer for reviewer in reviewers if '@' in reviewer) def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False): diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 4658a698a..d97e7fa7a 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -2572,7 +2572,8 @@ class CannedChecksUnittest(PresubmitTestsBase): def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None, reviewers=None, is_committing=True, rietveld_response=None, - uncovered_files=None, expected_output='', author_counts_as_owner=True): + uncovered_files=None, expected_output='', author_counts_as_owner=True, + manually_specified_reviewers=None): if approvers is None: # The set of people who lgtm'ed a change. approvers = set() @@ -2583,11 +2584,13 @@ class CannedChecksUnittest(PresubmitTestsBase): reviewers = approvers if uncovered_files is None: uncovered_files = set() + if manually_specified_reviewers is None: + manually_specified_reviewers = [] change = self.mox.CreateMock(presubmit.Change) change.issue = issue change.author_email = 'john@example.com' - change.R = '' + change.R = ','.join(manually_specified_reviewers) change.TBR = '' affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile) input_api = self.MockInputApi(change, False) @@ -2614,7 +2617,6 @@ class CannedChecksUnittest(PresubmitTestsBase): people = approvers else: people = reviewers - change.R = ','.join(reviewers) if issue: input_api.rietveld.get_issue_properties( @@ -2729,13 +2731,28 @@ class CannedChecksUnittest(PresubmitTestsBase): def testCannedCheckOwners_NoIssueLocalReviewers(self): self.AssertOwnersWorks(issue=None, reviewers=set(['jane@example.com']), + manually_specified_reviewers=['jane@example.com'], expected_output="OWNERS check failed: this change has no Rietveld " "issue number, so we can't check it for approvals.\n") self.AssertOwnersWorks(issue=None, reviewers=set(['jane@example.com']), + manually_specified_reviewers=['jane@example.com'], is_committing=False, expected_output='') + def testCannedCheckOwners_NoIssueLocalReviewersDontInferEmailDomain(self): + self.AssertOwnersWorks(issue=None, + reviewers=set(['jane']), + manually_specified_reviewers=['jane@example.com'], + expected_output="OWNERS check failed: this change has no Rietveld " + "issue number, so we can't check it for approvals.\n") + self.AssertOwnersWorks(issue=None, + uncovered_files=set(['foo']), + manually_specified_reviewers=['jane'], + is_committing=False, + expected_output='Missing OWNER reviewers for these files:\n' + ' foo\n') + def testCannedCheckOwners_NoLGTM(self): self.AssertOwnersWorks(expected_output='Missing LGTM from someone ' 'other than john@example.com\n')