diff --git a/PRESUBMIT.py b/PRESUBMIT.py index 8ea1fd9cf..ec7e8a978 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -20,6 +20,7 @@ UNIT_TESTS = [ 'tests.watchlists_unittest', ] + def CommonChecks(input_api, output_api): output = [] # Verify that LocalPath() is local, e.g.: @@ -31,9 +32,11 @@ def CommonChecks(input_api, output_api): # Return right away because it needs to be fixed first. return output - output.extend(input_api.canned_checks.CheckOwners( - input_api, - output_api)) + # TODO(dpranke): uncomment and enable :). + # + # output.extend(input_api.canned_checks.CheckOwners( + # input_api, + # output_api)) output.extend(input_api.canned_checks.RunPythonUnitTests( input_api, diff --git a/owners.py b/owners.py index f43deae97..6a48f43ab 100644 --- a/owners.py +++ b/owners.py @@ -44,8 +44,7 @@ class Database(object): self.fopen = fopen self.os_path = os_path - # TODO: Figure out how to share the owners email addr format w/ - # tools/commit-queue/projects.py, especially for per-repo whitelists. + # Pick a default email regexp to use; callers can override as desired. self.email_regexp = re.compile(BASIC_EMAIL_REGEXP) # Mapping of owners to the paths they own. diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 29967f54d..b3d36199f 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -540,7 +540,7 @@ def RunPylint(input_api, output_api, white_list=None, black_list=None): finally: warnings.filterwarnings('default', category=DeprecationWarning) - +# TODO(dpranke): Get the host_url from the input_api instead def CheckRietveldTryJobExecution(input_api, output_api, host_url, platforms, owner): if not input_api.is_committing: @@ -631,11 +631,14 @@ def CheckOwners(input_api, output_api, source_file_filter=None): input_api.change.AffectedFiles(source_file_filter)]) owners_db = input_api.owners_db - if input_api.is_committing: - missing_files = owners_db.files_not_covered_by(affected_files, - input_api.change.approvers) + 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 owner LGTM for: %s' % + return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' % ','.join(missing_files))] return [] elif input_api.change.tags.get('R'): @@ -643,3 +646,29 @@ def CheckOwners(input_api, output_api, source_file_filter=None): suggested_reviewers = owners_db.reviewers_for(affected_files) return [output_api.PresubmitAddText('R=%s' % ','.join(suggested_reviewers))] + + +def _Approvers(input_api, email_regexp): + if not input_api.change.issue: + return [] + + path = '/api/%s?messages=true' + url = (input_api.host_url + path) % input_api.change.issue + + f = input_api.urllib2.urlopen(url) + issue_props = input_api.json.load(f) + owner = input_api.re.escape(issue_props['owner']) + + # 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) + + approvers = [] + for m in issue_props['messages']: + if 'lgtm' in m['text'].lower() and match_reviewer(m['sender']): + approvers.append(m['sender']) + return set(approvers) + diff --git a/presubmit_support.py b/presubmit_support.py index 5e2f8d13e..4c12912dc 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -227,7 +227,10 @@ class InputApi(object): r"(|.*[\\\/])\.svn[\\\/].*", ) - def __init__(self, change, presubmit_path, is_committing): + # TODO(dpranke): Update callers to pass in tbr, host_url, remove + # default arguments. + def __init__(self, change, presubmit_path, is_committing, tbr=False, + host_url='http://codereview.chromium.org'): """Builds an InputApi object. Args: @@ -238,7 +241,9 @@ class InputApi(object): # Version number of the presubmit_support script. self.version = [int(x) for x in __version__.split('.')] self.change = change + self.host_url = host_url self.is_committing = is_committing + self.tbr = tbr # We expose various modules and functions as attributes of the input_api # so that presubmit scripts don't have to import them. @@ -653,10 +658,6 @@ class Change(object): self._local_root = os.path.abspath(local_root) self.issue = issue self.patchset = patchset - - # TODO(dpranke): implement - get from the patchset? - self.approvers = set() - self.scm = '' # From the description text, build up a dictionary of key/value pairs diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 80a6e8dd0..bbe54b1c1 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -700,9 +700,10 @@ class InputApiUnittest(PresubmitTestsBase): 'LocalToDepotPath', 'PresubmitLocalPath', 'ReadFile', 'RightHandSideLines', 'ServerPaths', 'basename', 'cPickle', 'cStringIO', 'canned_checks', 'change', 'environ', - 'is_committing', 'json', 'marshal', 'os_path', 'owners_db', 'pickle', - 'platform', 'python_executable', 're', 'subprocess', 'tempfile', - 'traceback', 'unittest', 'urllib2', 'version', + 'host_url', 'is_committing', 'json', 'marshal', 'os_path', + 'owners_db', 'pickle', 'platform', 'python_executable', 're', + 'subprocess', 'tbr', 'tempfile', 'traceback', 'unittest', 'urllib2', + 'version', ] # If this test fails, you should add the relevant test. self.compareMembers(presubmit.InputApi(self.fake_change, './.', False), @@ -1187,7 +1188,7 @@ class GclChangeUnittest(PresubmitTestsBase): 'AbsoluteLocalPaths', 'AffectedFiles', 'AffectedTextFiles', 'DescriptionText', 'FullDescriptionText', 'LocalPaths', 'Name', 'RepositoryRoot', 'RightHandSideLines', 'ServerPaths', - 'approvers', 'issue', 'patchset', 'scm', 'tags', + 'issue', 'patchset', 'scm', 'tags', ] # If this test fails, you should add the relevant test. self.mox.ReplayAll() @@ -1214,7 +1215,9 @@ class CannedChecksUnittest(PresubmitTestsBase): input_api.subprocess = self.mox.CreateMock(presubmit.subprocess) input_api.change = change + input_api.host_url = 'http://localhost' input_api.is_committing = committing + input_api.tbr = False input_api.python_executable = 'pyyyyython' return input_api @@ -1857,7 +1860,7 @@ mac|success|blew self.assertEquals(results[0].__class__, presubmit.OutputApi.PresubmitNotifyResult) - def OwnersTest(self, is_committing, change_tags=None, + def OwnersTest(self, is_committing, tbr=False, change_tags=None, suggested_reviewers=None, approvers=None, uncovered_files=None, expected_results=None): affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile) @@ -1867,21 +1870,31 @@ mac|success|blew input_api = self.MockInputApi(change, False) 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 - - if is_committing: - change.approvers = approvers + input_api.tbr = tbr + + if is_committing and not tbr: + change.issue = '1' + messages = list('{"sender": "' + a + '","text": "lgtm"}' for + a in approvers) + rietveld_response = ('{"owner": "john@example.com",' + '"messages": [' + ','.join(messages) + ']}') + input_api.urllib2.urlopen( + input_api.host_url + '/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) - else: + elif not is_committing: change.tags = change_tags if not change_tags.get('R'): fake_db.reviewers_for(set(['foo.cc'])).AndReturn(suggested_reviewers) self.mox.ReplayAll() results = presubmit_canned_checks.CheckOwners(input_api, - presubmit.OutputApi, None) + presubmit.OutputApi) self.assertEquals(len(results), len(expected_results)) if results and expected_results: output = StringIO.StringIO() @@ -1892,24 +1905,34 @@ mac|success|blew def testCannedCheckOwners_WithReviewer(self): self.OwnersTest(is_committing=False, change_tags={'R': 'ben@example.com'}, expected_results=[]) + self.OwnersTest(is_committing=False, tbr=True, + change_tags={'R': 'ben@example.com'}, expected_results=[]) def testCannedCheckOwners_NoReviewer(self): self.OwnersTest(is_committing=False, change_tags={}, suggested_reviewers=['ben@example.com'], expected_results=['ADD: R=ben@example.com\n']) + self.OwnersTest(is_committing=False, tbr=True, change_tags={}, + suggested_reviewers=['ben@example.com'], + expected_results=['ADD: R=ben@example.com\n']) def testCannedCheckOwners_CommittingWithoutOwnerLGTM(self): self.OwnersTest(is_committing=True, approvers=set(), uncovered_files=set(['foo.cc']), - expected_results=['Missing owner LGTM for: foo.cc\n']) + expected_results=['Missing LGTM from an OWNER for: foo.cc\n']) def testCannedCheckOwners_CommittingWithLGTMs(self): self.OwnersTest(is_committing=True, - approvers=set('ben@example.com'), + approvers=set(['ben@example.com']), uncovered_files=set(), expected_results=[]) + def testCannedCheckOwners_TBR(self): + self.OwnersTest(is_committing=True, tbr=True, + approvers=set(), + uncovered_files=set(), + expected_results=['--tbr was specified, skipping OWNERS check\n']) if __name__ == '__main__': import unittest