From e4067ab7a252fe330e02243551ef9a544d4dbb58 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Fri, 3 Jun 2011 01:07:35 +0000 Subject: [PATCH] Switch CheckOwners to use input_api.rietveld. This enables private issue checking and simplifies testing. R=dpranke@chromium.org Review URL: http://codereview.chromium.org/7058025 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@87743 0039d316-1c4b-4281-b951-d872f2087c98 --- presubmit_canned_checks.py | 16 ++++--- rietveld.py | 1 - tests/presubmit_unittest.py | 85 +++++++++++++++++-------------------- 3 files changed, 49 insertions(+), 53 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 734292519..aedd741e1 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -741,6 +741,10 @@ def CheckOwners(input_api, output_api, source_file_filter=None): owners_db = input_api.owners_db owner_email, approvers = _RietveldOwnerAndApprovers(input_api, owners_db.email_regexp) + if not owner_email: + return [output_api.PresubmitWarning( + 'The issue was not uploaded so you have no OWNER approval.')] + approvers_plus_owner = approvers.union(set([owner_email])) missing_files = owners_db.files_not_covered_by(affected_files, @@ -757,13 +761,11 @@ def CheckOwners(input_api, output_api, source_file_filter=None): 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 - if not host.startswith('http://') and not host.startswith('https://'): - host = 'http://' + host - url = '%s/api/%s?messages=true' % (host, input_api.change.issue) - issue_props = input_api.json.load(input_api.urllib2.urlopen(url)) + if not input_api.change.issue: + return None, None + + issue_props = input_api.rietveld.get_issue_properties( + int(input_api.change.issue), True) owner_email = issue_props['owner_email'] def match_reviewer(r): diff --git a/rietveld.py b/rietveld.py index 4c4e8a3d7..b3895d639 100644 --- a/rietveld.py +++ b/rietveld.py @@ -41,7 +41,6 @@ upload.logging.setLevel(logging.WARNING) # pylint: disable=E1103 class Rietveld(object): """Accesses rietveld.""" def __init__(self, url, email, password, extra_headers=None): - self.issue = None self.url = url if email and password: get_creds = lambda: (email, password) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index f07e2fda3..09182db3f 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -16,6 +16,7 @@ from super_mox import mox, SuperMoxTestBase import owners import presubmit_support as presubmit +import rietveld # Shortcut. presubmit_canned_checks = presubmit.presubmit_canned_checks @@ -1338,10 +1339,12 @@ class CannedChecksUnittest(PresubmitTestsBase): def MockInputApi(self, change, committing): input_api = self.mox.CreateMock(presubmit.InputApi) input_api.cStringIO = presubmit.cStringIO + input_api.json = presubmit.json input_api.os_listdir = self.mox.CreateMockAnything() input_api.os_walk = self.mox.CreateMockAnything() input_api.os_path = presubmit.os.path input_api.re = presubmit.re + input_api.rietveld = self.mox.CreateMock(rietveld.Rietveld) input_api.traceback = presubmit.traceback input_api.urllib2 = self.mox.CreateMock(presubmit.urllib2) input_api.unittest = unittest @@ -1815,7 +1818,6 @@ class CannedChecksUnittest(PresubmitTestsBase): def testCannedCheckJsonTreeIsOpenOpen(self): input_api = self.MockInputApi(None, True) - input_api.json = presubmit.json connection = self.mox.CreateMockAnything() input_api.urllib2.urlopen('url_to_open').AndReturn(connection) status = { @@ -1832,7 +1834,6 @@ class CannedChecksUnittest(PresubmitTestsBase): def testCannedCheckJsonTreeIsOpenClosed(self): input_api = self.MockInputApi(None, True) - input_api.json = presubmit.json connection = self.mox.CreateMockAnything() input_api.urllib2.urlopen('url_to_closed').AndReturn(connection) status = { @@ -1968,7 +1969,6 @@ mac|success|blew def testCheckBuildbotPendingBuildsBad(self): input_api = self.MockInputApi(None, True) - input_api.json = presubmit.json connection = self.mox.CreateMockAnything() input_api.urllib2.urlopen('uurl').AndReturn(connection) connection.read().AndReturn('foo') @@ -1983,7 +1983,6 @@ mac|success|blew def testCheckBuildbotPendingBuildsGood(self): input_api = self.MockInputApi(None, True) - input_api.json = presubmit.json connection = self.mox.CreateMockAnything() input_api.urllib2.urlopen('uurl').AndReturn(connection) connection.read().AndReturn(""" @@ -2002,8 +2001,7 @@ mac|success|blew presubmit.OutputApi.PresubmitNotifyResult) def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None, - rietveld_response=None, host_url=None, - uncovered_files=None, expected_output=''): + rietveld_response=None, uncovered_files=None, expected_output=''): if approvers is None: approvers = set() if uncovered_files is None: @@ -2022,26 +2020,19 @@ mac|success|blew if not tbr and issue: affected_file.LocalPath().AndReturn('foo.cc') change.AffectedFiles(file_filter=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' if not rietveld_response: - messages = ( - '{"sender": "%s", "text": "I approve", "approval": true}' % a - for a in approvers) - rietveld_response = '{"owner_email": "%s", "messages": [%s]}' % ( - owner_email, ','.join(messages)) - input_api.urllib2.urlopen( - expected_host + '/api/1?messages=true').AndReturn( - StringIO.StringIO(rietveld_response)) - input_api.json = presubmit.json + rietveld_response = { + "owner_email": owner_email, + "messages": [ + {"sender": a, "text": "I approve", "approval": True} + for a in approvers + ], + } + input_api.rietveld.get_issue_properties( + int(input_api.change.issue), True).AndReturn(rietveld_response) fake_db.files_not_covered_by(set(['foo.cc']), - approvers.union(set([owner_email]))).AndReturn(uncovered_files) + approvers.union(set([owner_email]))).AndReturn(uncovered_files) self.mox.ReplayAll() output = presubmit.PresubmitOutput() @@ -2051,29 +2042,33 @@ mac|success|blew results[0].handle(output) self.assertEquals(output.getvalue(), expected_output) - def testCannedCheckOwners_LGTMPhrases(self): - def phrase_test(approval, 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": "foo", "approval": %s}]}') % approval, - expected_output=expected_output) - - phrase_test('true') - phrase_test('false', approvers=set(), - expected_output='Missing LGTM from someone other than ' - 'john@example.com\n') - - def testCannedCheckOwners_HTTPS_HostURL(self): - self.AssertOwnersWorks(approvers=set(['ben@example.com']), - host_url='https://localhost') - - def testCannedCheckOwners_MissingSchemeInHostURL(self): + def testCannedCheckOwners_Approved(self): + response = { + "owner_email": "john@example.com", + "messages": [ + { + "sender": "ben@example.com", "text": "foo", "approval": True, + }, + ], + } self.AssertOwnersWorks(approvers=set(['ben@example.com']), - host_url='localhost') + rietveld_response=response, + expected_output='') + + def testCannedCheckOwners_NotApproved(self): + response = { + "owner_email": "john@example.com", + "messages": [ + { + "sender": "ben@example.com", "text": "foo", "approval": False, + }, + ], + } + self.AssertOwnersWorks( + approvers=set(), + rietveld_response=response, + expected_output= + 'Missing LGTM from someone other than john@example.com\n') def testCannedCheckOwners_NoIssue(self): self.AssertOwnersWorks(issue=None,