diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 5b54a5b9a..2adc371a8 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -627,35 +627,37 @@ def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings, return [] -def CheckOwners(input_api, output_api, email_regexp=None, - source_file_filter=None): - affected_files = set([f.LocalPath() for f in - input_api.change.AffectedFiles(source_file_filter)]) - owners_db = input_api.owners_db - if email_regexp: - owners_db.email_regexp = input_api.re.compile(email_regexp) - - if input_api.is_committing and input_api.tbr: - return [output_api.PresubmitNotifyResult( - '--tbr was specified, skipping OWNERS check')] - elif input_api.is_committing: - approvers = _Approvers(input_api, owners_db.email_regexp) - missing_files = owners_db.files_not_covered_by(affected_files, approvers) - if missing_files: - return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' % - ','.join(missing_files))] - return [] - elif input_api.change.tags.get('R'): +def CheckOwners(input_api, output_api, source_file_filter=None): + if not input_api.is_committing: return [] + if input_api.tbr: + return [output_api.PresubmitNotifyResult('--tbr was specified, ' + 'skipping OWNERS check')] + if not input_api.change.issue: + return [output_api.PresubmitError('Change not uploaded for review')] - suggested_reviewers = owners_db.reviewers_for(affected_files) - return [output_api.PresubmitAddReviewers(suggested_reviewers)] + affected_files = set([f.LocalPath() for f in + input_api.change.AffectedFiles(source_file_filter)]) + owners_db = input_api.owners_db + owner_email, approvers = _RietveldOwnerAndApprovers(input_api, + owners_db.email_regexp) + approvers_plus_owner = approvers.union(set([owner_email])) + + missing_files = owners_db.files_not_covered_by(affected_files, + approvers_plus_owner) + if missing_files: + return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' % + ','.join(missing_files))] + + if not approvers: + return [output_api.PresubmitError('Missing LGTM from someone other than %s' + % owner_email)] + return [] -def _Approvers(input_api, email_regexp): - if not input_api.change.issue: - return [] +def _RietveldOwnerAndApprovers(input_api, email_regexp): + """Return the owner and approvers of a change, if any.""" # TODO(dpranke): Should figure out if input_api.host_url is supposed to # be a host or a scheme+host and normalize it there. host = input_api.host_url @@ -665,19 +667,56 @@ def _Approvers(input_api, email_regexp): f = input_api.urllib2.urlopen(url) issue_props = input_api.json.load(f) - owner = input_api.re.escape(issue_props['owner']) + messages = issue_props.get('messages', []) + owner_email = issue_props['owner_email'] + owner_regexp = input_api.re.compile(input_api.re.escape(owner_email)) + approvers = _GetApprovers(messages, email_regexp, owner_regexp) + + return (owner_email, set(approvers)) + + +def _IsApprovingComment(text): + """Implements the logic for parsing a change comment for approval.""" + + # Any comment that contains a non-quoted line containing an 'lgtm' is an + # approval. + # + # TODO(dpranke): this differs from the logic used inside Google in a few + # ways. Inside Google, + # + # 1) the approving phrase must appear at the beginning of the first non + # quoted-line in the comment.' + # 2) "LG", "Looks Good", and "Looks Good to Me" are also acceptable. + # 3) Subsequent comments from the reviewer can rescind approval, unless + # the phrase "LGTM++" was used. + # We should consider implementing some or all of this here. + for l in text.splitlines(): + l = l.strip().lower() + if l.startswith('>'): + continue + + if 'lgtm' in l: + return True + return False + + +def _GetApprovers(messages, email_regexp, owner_regexp): + """Returns the set of approvers for a change given the owner and messages. + + Messages should be a list of dicts containing 'sender' and 'text' keys.""" # TODO(dpranke): This mimics the logic in # /tools/commit-queue/verifiers/reviewer_lgtm.py # We should share the code and/or remove the check there where it is # redundant (since the commit queue also enforces the presubmit checks). def match_reviewer(r): - return email_regexp.match(r) and not input_api.re.match(owner, r) + return email_regexp.match(r) and not owner_regexp.match(r) approvers = [] - for m in issue_props.get('messages', []): - if 'lgtm' in m['text'].lower() and match_reviewer(m['sender']): - approvers.append(m['sender']) + for m in messages: + sender = m['sender'] + if _IsApprovingComment(m['text']) and match_reviewer(sender): + approvers.append(sender) return set(approvers) @@ -774,10 +813,8 @@ def PanProjectChecks(input_api, output_api, text_files = lambda x: input_api.FilterSourceFile(x, black_list=black_list, white_list=white_list) - # TODO(dpranke): enable upload as well - if input_api.is_committing: - results.extend(input_api.canned_checks.CheckOwners( - input_api, output_api, source_file_filter=sources)) + results.extend(input_api.canned_checks.CheckOwners( + input_api, output_api, source_file_filter=sources)) results.extend(input_api.canned_checks.CheckLongLines( input_api, output_api, source_file_filter=sources)) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 1ecadb9c1..d47e1d0b1 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1884,43 +1884,46 @@ mac|success|blew self.assertEquals(results[0].__class__, presubmit.OutputApi.PresubmitNotifyResult) - def OwnersTest(self, is_committing, tbr=False, change_tags=None, - suggested_reviewers=None, approvers=None, - uncovered_files=None, expected_reviewers=None, expected_output='', - host_url=None): - affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile) - affected_file.LocalPath().AndReturn('foo.cc') - change = self.mox.CreateMock(presubmit.Change) - change.AffectedFiles(None).AndReturn([affected_file]) + def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None, + rietveld_response=None, host_url=None, + uncovered_files=None, expected_output=''): + if approvers is None: + approvers = set() + if uncovered_files is None: + uncovered_files = set() + change = self.mox.CreateMock(presubmit.Change) + change.issue = issue + affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile) input_api = self.MockInputApi(change, False) - expected_host = 'http://localhost' - if host_url: - input_api.host_url = host_url - if host_url.startswith('https'): - expected_host = host_url fake_db = self.mox.CreateMock(owners.Database) fake_db.email_regexp = input_api.re.compile(owners.BASIC_EMAIL_REGEXP) input_api.owners_db = fake_db - input_api.is_committing = is_committing + input_api.is_committing = True input_api.tbr = tbr - if is_committing and not tbr: - change.issue = '1' + if not tbr and issue: + affected_file.LocalPath().AndReturn('foo.cc') + change.AffectedFiles(None).AndReturn([affected_file]) + + expected_host = 'http://localhost' + if host_url: + input_api.host_url = host_url + if host_url.startswith('https'): + expected_host = host_url + + owner_email = 'john@example.com' messages = list('{"sender": "' + a + '","text": "lgtm"}' for a in approvers) - rietveld_response = ('{"owner": "john@example.com",' - '"messages": [' + ','.join(messages) + ']}') + if not rietveld_response: + rietveld_response = ('{"owner_email": "' + owner_email + '",' + '"messages": [' + ','.join(messages) + ']}') input_api.urllib2.urlopen( expected_host + '/api/1?messages=true').AndReturn( StringIO.StringIO(rietveld_response)) input_api.json = presubmit.json - fake_db.files_not_covered_by(set(['foo.cc']), approvers).AndReturn( - uncovered_files) - elif not is_committing: - change.tags = change_tags - if not change_tags.get('R'): - fake_db.reviewers_for(set(['foo.cc'])).AndReturn(suggested_reviewers) + fake_db.files_not_covered_by(set(['foo.cc']), + approvers.union(set([owner_email]))).AndReturn(uncovered_files) self.mox.ReplayAll() output = presubmit.PresubmitOutput() @@ -1928,49 +1931,77 @@ mac|success|blew presubmit.OutputApi) if results: results[0].handle(output) - if expected_reviewers is not None: - self.assertEquals(output.reviewers, expected_reviewers) self.assertEquals(output.getvalue(), expected_output) - def testCannedCheckOwners_WithReviewer(self): - self.OwnersTest(is_committing=False, change_tags={'R': 'ben@example.com'}) - self.OwnersTest(is_committing=False, tbr=True, - change_tags={'R': 'ben@example.com'}) - - def testCannedCheckOwners_NoReviewer(self): - self.OwnersTest(is_committing=False, change_tags={}, - suggested_reviewers=['ben@example.com'], - expected_reviewers=['ben@example.com']) - self.OwnersTest(is_committing=False, tbr=True, change_tags={}, - suggested_reviewers=['ben@example.com'], - expected_reviewers=['ben@example.com']) - - def testCannedCheckOwners_CommittingWithoutOwnerLGTM(self): - self.OwnersTest(is_committing=True, - approvers=set(), - uncovered_files=set(['foo.cc']), - expected_output='Missing LGTM from an OWNER for: foo.cc\n') + def testCannedCheckOwners_LGTMPhrases(self): + def phrase_test(phrase, approvers=None, expected_output=''): + if approvers is None: + approvers = set(['ben@example.com']) + self.AssertOwnersWorks(approvers=approvers, + rietveld_response='{"owner_email": "john@example.com",' + + '"messages": [{"sender": "ben@example.com",' + + '"text": "' + phrase + '"}]}', + expected_output=expected_output) + + phrase_test('LGTM') + phrase_test('\\nlgtm') + phrase_test('> foo\\n> bar\\nlgtm\\n') + phrase_test('> LGTM', approvers=set(), + expected_output='Missing LGTM from someone other than ' + 'john@example.com\n') + + # TODO(dpranke): these probably should pass. + phrase_test('Looks Good To Me', approvers=set(), + expected_output='Missing LGTM from someone other than ' + 'john@example.com\n') + phrase_test('looks good to me', approvers=set(), + expected_output='Missing LGTM from someone other than ' + 'john@example.com\n') + + # TODO(dpranke): this probably shouldn't pass. + phrase_test('no lgtm for you') + + def testCannedCheckOwners_HTTPS_HostURL(self): + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + host_url='https://localhost') - def testCannedCheckOwners_CommittingWithLGTMs(self): - self.OwnersTest(is_committing=True, - approvers=set(['ben@example.com']), - uncovered_files=set()) + def testCannedCheckOwners_MissingSchemeInHostURL(self): + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + host_url='localhost') + + def testCannedCheckOwners_NoIssue(self): + self.AssertOwnersWorks(issue=None, + expected_output='Change not uploaded for review\n') + + def testCannedCheckOwners_NoLGTM(self): + self.AssertOwnersWorks(expected_output='Missing LGTM from someone ' + 'other than john@example.com\n') + + def testCannedCheckOwners_OnlyOwnerLGTM(self): + self.AssertOwnersWorks(approvers=set(['john@example.com']), + expected_output='Missing LGTM from someone ' + 'other than john@example.com\n') def testCannedCheckOwners_TBR(self): - self.OwnersTest(is_committing=True, tbr=True, - approvers=set(), - uncovered_files=set(), + self.AssertOwnersWorks(tbr=True, expected_output='--tbr was specified, skipping OWNERS check\n') - def testCannedCheckOwners_MissingSchemeInHostURL(self): - self.OwnersTest(is_committing=True, - approvers=set(['ben@example.com']), - uncovered_files=set(), host_url='localhost') + def testCannedCheckOwners_Upload(self): + class FakeInputAPI(object): + is_committing = False + + results = presubmit_canned_checks.CheckOwners(FakeInputAPI(), + presubmit.OutputApi) + self.assertEqual(results, []) + + def testCannedCheckOwners_WithoutOwnerLGTM(self): + self.AssertOwnersWorks(uncovered_files=set(['foo.cc']), + expected_output='Missing LGTM from an OWNER for: foo.cc\n') + + def testCannedCheckOwners_WithLGTMs(self): + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + uncovered_files=set()) - def testCannedCheckOwners_HTTPS_HostURL(self): - self.OwnersTest(is_committing=True, - approvers=set(['ben@example.com']), - uncovered_files=set(), host_url='https://localhost') if __name__ == '__main__':