From 3e331bdf1636e333f14a6c91b4535ccbcf9ee77b Mon Sep 17 00:00:00 2001 From: "dpranke@chromium.org" Date: Thu, 24 Mar 2011 23:13:04 +0000 Subject: [PATCH] Clean up the parsing of approvals for OWNERS checks. This fixes bugs 76724. We will now allow approvals from only non-owners to be sufficient if the patch is from an OWNER. Also, this strips out the code for suggesting reviewers during upload completely. I've come to believe that this should be done by gcl and git-cl directly rather than through a presubmit hook, when the change is being initially created. CheckOwners() is now a no-op on upload. (We might need to add a new value to the codereview.settings file instead to indicate if we want this to happen). Review URL: http://codereview.chromium.org/6730020 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@79339 0039d316-1c4b-4281-b951-d872f2087c98 --- presubmit_canned_checks.py | 103 +++++++++++++++++-------- tests/presubmit_unittest.py | 149 ++++++++++++++++++++++-------------- 2 files changed, 160 insertions(+), 92 deletions(-) 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__':