From aa6ddc6017794d9c25ad782e6c8a7c953e4023b4 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Mon, 2 Apr 2018 14:54:30 -0700 Subject: [PATCH] Revert "Remove Rietveld support from presubmit" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit f3eed0016e3400b43ef4d1f6d42e252250077910. Reason for revert: Some PRESUBMIT checks still expect CheckRietveldTryJobExecution to exist. Original change's description: > Remove Rietveld support from presubmit > > Since no one can upload or land changes from Rietveld anymore, > PRESUBMIT and its support files no longer need to be able to > support it. > > R=​tandrii@chromium.org > > Bug: 770408 > Change-Id: I749092b66fdca16d5cef77e8cefc905aa5375b50 > Reviewed-on: https://chromium-review.googlesource.com/693380 > Commit-Queue: Aaron Gable > Reviewed-by: Andrii Shyshkalov TBR=agable@chromium.org,tandrii@chromium.org Change-Id: I72e29bd8a9739406f29190adbeb7eb7718ed21cd No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 770408 Reviewed-on: https://chromium-review.googlesource.com/991012 Commit-Queue: Aaron Gable Reviewed-by: Aaron Gable --- git_cl.py | 27 ++++++ presubmit_canned_checks.py | 76 +++++++++++++--- presubmit_support.py | 71 +++++++++++++-- tests/presubmit_unittest.py | 177 ++++++++++++++++++++++-------------- 4 files changed, 263 insertions(+), 88 deletions(-) diff --git a/git_cl.py b/git_cl.py index e90b93f5c..d56d0721c 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1525,6 +1525,7 @@ class Changelist(object): return presubmit_support.DoPresubmitChecks(change, committing, verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin, default_presubmit=None, may_prompt=may_prompt, + rietveld_obj=self._codereview_impl.GetRietveldObjForPresubmit(), gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit()) except presubmit_support.PresubmitFailure as e: DieWithError('%s\nMaybe your depot_tools is out of date?' % e) @@ -1806,6 +1807,11 @@ class _ChangelistCodereviewBase(object): """Which branch-specific properties to erase when unsetting issue.""" return [] + def GetRietveldObjForPresubmit(self): + # This is an unfortunate Rietveld-embeddedness in presubmit. + # For non-Rietveld code reviews, this probably should return a dummy object. + raise NotImplementedError() + def GetGerritObjForPresubmit(self): # None is valid return value, otherwise presubmit_support.GerritAccessor. return None @@ -2123,6 +2129,9 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): def CodereviewServerConfigKey(cls): return 'rietveldserver' + def GetRietveldObjForPresubmit(self): + return self.RpcServer() + def SetLabels(self, enable_auto_submit, use_commit_queue, cq_dry_run): raise NotImplementedError() @@ -2495,6 +2504,24 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): """Which branch-specific properties to erase when unsetting issue.""" return ['gerritsquashhash'] + def GetRietveldObjForPresubmit(self): + class ThisIsNotRietveldIssue(object): + def __nonzero__(self): + # This is a hack to make presubmit_support think that rietveld is not + # defined, yet still ensure that calls directly result in a decent + # exception message below. + return False + + def __getattr__(self, attr): + print( + 'You aren\'t using Rietveld at the moment, but Gerrit.\n' + 'Using Rietveld in your PRESUBMIT scripts won\'t work.\n' + 'Please, either change your PRESUBMIT to not use rietveld_obj.%s,\n' + 'or use Rietveld for codereview.\n' + 'See also http://crbug.com/579160.' % attr) + raise NotImplementedError() + return ThisIsNotRietveldIssue() + def GetGerritObjForPresubmit(self): return presubmit_support.GerritAccessor(self._GetGerritHost()) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index f1f61834f..d009c9fe1 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -793,6 +793,15 @@ def RunPylint(input_api, *args, **kwargs): return input_api.RunTests(GetPylint(input_api, *args, **kwargs), False) +def CheckRietveldTryJobExecution(dummy_input_api, output_api, + dummy_host_url, dummy_platforms, + dummy_owner): + return [ + output_api.PresubmitNotifyResult( + 'CheckRietveldTryJobExecution is deprecated, please remove it.') + ] + + def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings, ignored): try: @@ -886,25 +895,25 @@ def CheckOwners(input_api, output_api, source_file_filter=None): return [output_fn('Missing LGTM from someone other than %s' % owner_email)] return [] - def GetCodereviewOwnerAndReviewers(input_api, email_regexp, approval_needed): """Return the owner and reviewers of a change, if any. If approval_needed is True, only reviewers who have approved the change will be returned. """ - issue = input_api.change.issue - if not issue: - return None, (set() if approval_needed else - _ReviewersFromChange(input_api.change)) + # Rietveld is default. + func = _RietveldOwnerAndReviewers + if input_api.gerrit: + func = _GerritOwnerAndReviewers + return func(input_api, email_regexp, approval_needed) - owner_email = input_api.gerrit.GetChangeOwner(issue) - reviewers = set( - r for r in input_api.gerrit.GetChangeReviewers(issue, approval_needed) - if _match_reviewer_email(r, owner_email, email_regexp)) - input_api.logging.debug('owner: %s; approvals given by: %s', - owner_email, ', '.join(sorted(reviewers))) - return owner_email, reviewers + +def _GetRietveldIssueProps(input_api, messages): + """Gets the issue properties from rietveld.""" + issue = input_api.change.issue + if issue and input_api.rietveld: + return input_api.rietveld.get_issue_properties( + issue=int(issue), messages=messages) def _ReviewersFromChange(change): @@ -920,6 +929,49 @@ def _ReviewersFromChange(change): def _match_reviewer_email(r, owner_email, email_regexp): return email_regexp.match(r) and r != owner_email +def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False): + """Return the owner and reviewers of a change, if any. + + If approval_needed is True, only reviewers who have approved the change + will be returned. + """ + issue_props = _GetRietveldIssueProps(input_api, True) + if not issue_props: + return None, (set() if approval_needed else + _ReviewersFromChange(input_api.change)) + + if not approval_needed: + return issue_props['owner_email'], set(issue_props['reviewers']) + + owner_email = issue_props['owner_email'] + + messages = issue_props.get('messages', []) + approvers = set( + m['sender'] for m in messages + if m.get('approval') and _match_reviewer_email(m['sender'], owner_email, + email_regexp)) + return owner_email, approvers + + +def _GerritOwnerAndReviewers(input_api, email_regexp, approval_needed=False): + """Return the owner and reviewers of a change, if any. + + If approval_needed is True, only reviewers who have approved the change + will be returned. + """ + issue = input_api.change.issue + if not issue: + return None, (set() if approval_needed else + _ReviewersFromChange(input_api.change)) + + owner_email = input_api.gerrit.GetChangeOwner(issue) + reviewers = set( + r for r in input_api.gerrit.GetChangeReviewers(issue, approval_needed) + if _match_reviewer_email(r, owner_email, email_regexp)) + input_api.logging.debug('owner: %s; approvals given by: %s', + owner_email, ', '.join(sorted(reviewers))) + return owner_email, reviewers + def CheckSingletonInHeaders(input_api, output_api, source_file_filter=None): """Deprecated, must be removed.""" diff --git a/presubmit_support.py b/presubmit_support.py index 11b07589c..5b89a335d 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -41,6 +41,7 @@ import urlparse from warnings import warn # Local imports. +import auth import fix_encoding import gclient_utils import git_footers @@ -48,6 +49,7 @@ import gerrit_util import owners import owners_finder import presubmit_canned_checks +import rietveld import scm import subprocess2 as subprocess # Exposed through the API. @@ -388,13 +390,14 @@ class InputApi(object): ) def __init__(self, change, presubmit_path, is_committing, - verbose, gerrit_obj, dry_run=None): + rietveld_obj, verbose, gerrit_obj=None, dry_run=None): """Builds an InputApi object. Args: change: A presubmit.Change object. presubmit_path: The path to the presubmit script being processed. is_committing: True if the change is about to be committed. + rietveld_obj: rietveld.Rietveld client object gerrit_obj: provides basic Gerrit codereview functionality. dry_run: if true, some Checks will be skipped. """ @@ -402,8 +405,13 @@ class InputApi(object): self.version = [int(x) for x in __version__.split('.')] self.change = change self.is_committing = is_committing + self.rietveld = rietveld_obj self.gerrit = gerrit_obj self.dry_run = dry_run + # TBD + self.host_url = 'http://codereview.chromium.org' + if self.rietveld: + self.host_url = self.rietveld.url # We expose various modules and functions as attributes of the input_api # so that presubmit scripts don't have to import them. @@ -1264,17 +1272,19 @@ def DoPostUploadExecuter(change, class PresubmitExecuter(object): - def __init__(self, change, committing, verbose, - gerrit_obj, dry_run=None): + def __init__(self, change, committing, rietveld_obj, verbose, + gerrit_obj=None, dry_run=None): """ Args: change: The Change object. committing: True if 'git cl land' is running, False if 'git cl upload' is. + rietveld_obj: rietveld.Rietveld client object. gerrit_obj: provides basic Gerrit codereview functionality. dry_run: if true, some Checks will be skipped. """ self.change = change self.committing = committing + self.rietveld = rietveld_obj self.gerrit = gerrit_obj self.verbose = verbose self.dry_run = dry_run @@ -1298,7 +1308,7 @@ class PresubmitExecuter(object): # Load the presubmit script into context. input_api = InputApi(self.change, presubmit_path, self.committing, - self.verbose, + self.rietveld, self.verbose, gerrit_obj=self.gerrit, dry_run=self.dry_run) output_api = OutputApi(self.committing) context = {} @@ -1347,7 +1357,8 @@ def DoPresubmitChecks(change, input_stream, default_presubmit, may_prompt, - gerrit_obj, + rietveld_obj, + gerrit_obj=None, dry_run=None): """Runs all presubmit checks that apply to the files in the change. @@ -1367,6 +1378,7 @@ def DoPresubmitChecks(change, default_presubmit: A default presubmit script to execute in any case. may_prompt: Enable (y/n) questions on warning or error. If False, any questions are answered with yes by default. + rietveld_obj: rietveld.Rietveld object. gerrit_obj: provides basic Gerrit codereview functionality. dry_run: if true, some Checks will be skipped. @@ -1395,7 +1407,7 @@ def DoPresubmitChecks(change, if not presubmit_files and verbose: output.write("Warning, no PRESUBMIT.py found.\n") results = [] - executer = PresubmitExecuter(change, committing, verbose, + executer = PresubmitExecuter(change, committing, rietveld_obj, verbose, gerrit_obj, dry_run) if default_presubmit: if verbose: @@ -1590,8 +1602,17 @@ def main(argv=None): parser.add_option("--gerrit_url", help=optparse.SUPPRESS_HELP) parser.add_option("--gerrit_fetch", action='store_true', help=optparse.SUPPRESS_HELP) + parser.add_option("--rietveld_url", help=optparse.SUPPRESS_HELP) + parser.add_option("--rietveld_email", help=optparse.SUPPRESS_HELP) + parser.add_option("--rietveld_fetch", action='store_true', default=False, + help=optparse.SUPPRESS_HELP) + # These are for OAuth2 authentication for bots. See also apply_issue.py + parser.add_option("--rietveld_email_file", help=optparse.SUPPRESS_HELP) + parser.add_option("--rietveld_private_key_file", help=optparse.SUPPRESS_HELP) + auth.add_auth_options(parser) options, args = parser.parse_args(argv) + auth_config = auth.extract_auth_config_from_options(options) if options.verbose >= 2: logging.basicConfig(level=logging.DEBUG) @@ -1600,14 +1621,49 @@ def main(argv=None): else: logging.basicConfig(level=logging.ERROR) + if (any((options.rietveld_url, options.rietveld_email_file, + options.rietveld_fetch, options.rietveld_private_key_file)) + and any((options.gerrit_url, options.gerrit_fetch))): + parser.error('Options for only codereview --rietveld_* or --gerrit_* ' + 'allowed') + + if options.rietveld_email and options.rietveld_email_file: + parser.error("Only one of --rietveld_email or --rietveld_email_file " + "can be passed to this program.") + if options.rietveld_email_file: + with open(options.rietveld_email_file, "rb") as f: + options.rietveld_email = f.read().strip() + change_class, files = load_files(options, args) if not change_class: parser.error('For unversioned directory, is not optional.') logging.info('Found %d file(s).', len(files)) - gerrit_obj = None + rietveld_obj, gerrit_obj = None, None + + if options.rietveld_url: + # The empty password is permitted: '' is not None. + if options.rietveld_private_key_file: + rietveld_obj = rietveld.JwtOAuth2Rietveld( + options.rietveld_url, + options.rietveld_email, + options.rietveld_private_key_file) + else: + rietveld_obj = rietveld.CachingRietveld( + options.rietveld_url, + auth_config, + options.rietveld_email) + if options.rietveld_fetch: + assert options.issue + props = rietveld_obj.get_issue_properties(options.issue, False) + options.author = props['owner_email'] + options.description = props['description'] + logging.info('Got author: "%s"', options.author) + logging.info('Got description: """\n%s\n"""', options.description) + if options.gerrit_url and options.gerrit_fetch: assert options.issue and options.patchset + rietveld_obj = None gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc) options.author = gerrit_obj.GetChangeOwner(options.issue) options.description = gerrit_obj.GetChangeDescription(options.issue, @@ -1632,6 +1688,7 @@ def main(argv=None): sys.stdin, options.default_presubmit, options.may_prompt, + rietveld_obj, gerrit_obj, options.dry_run) return not results.should_continue() diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index e3894db7b..5cd68a9d1 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -27,6 +27,7 @@ import owners import owners_finder import subprocess2 as subprocess import presubmit_support as presubmit +import rietveld import auth import git_cl import git_common as git @@ -176,11 +177,11 @@ class PresubmitUnittest(PresubmitTestsBase): 'GitChange', 'InputApi', 'ListRelevantPresubmitFiles', 'main', 'NonexistantCannedCheckFilter', 'OutputApi', 'ParseFiles', 'PresubmitFailure', 'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs', - 'ast', 'cPickle', 'cpplint', 'cStringIO', 'contextlib', + 'ast', 'auth', 'cPickle', 'cpplint', 'cStringIO', 'contextlib', 'canned_check_filter', 'fix_encoding', 'fnmatch', 'gclient_utils', 'git_footers', 'glob', 'inspect', 'json', 'load_files', 'logging', 'marshal', 'normpath', 'optparse', 'os', 'owners', 'owners_finder', - 'pickle', 'presubmit_canned_checks', 'random', 're', 'scm', + 'pickle', 'presubmit_canned_checks', 'random', 're', 'rietveld', 'scm', 'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest', 'urllib2', 'warn', 'multiprocessing', 'DoGetTryMasters', 'GetTryMastersExecuter', 'itertools', 'urlparse', 'gerrit_util', @@ -620,7 +621,7 @@ class PresubmitUnittest(PresubmitTestsBase): output = presubmit.DoPresubmitChecks( change=change, committing=False, verbose=True, output_stream=None, input_stream=None, - default_presubmit=None, may_prompt=False, gerrit_obj=None) + default_presubmit=None, may_prompt=False, rietveld_obj=None) self.failUnless(output.should_continue()) self.assertEqual(output.getvalue().count('!!'), 0) self.assertEqual(output.getvalue().count('??'), 0) @@ -655,7 +656,7 @@ class PresubmitUnittest(PresubmitTestsBase): output = presubmit.DoPresubmitChecks( change=change, committing=False, verbose=True, output_stream=None, input_stream=input_buf, - default_presubmit=None, may_prompt=True, gerrit_obj=None) + default_presubmit=None, may_prompt=True, rietveld_obj=None) self.failIf(output.should_continue()) self.assertEqual(output.getvalue().count('??'), 2) @@ -663,7 +664,7 @@ class PresubmitUnittest(PresubmitTestsBase): output = presubmit.DoPresubmitChecks( change=change, committing=False, verbose=True, output_stream=None, input_stream=input_buf, - default_presubmit=None, may_prompt=True, gerrit_obj=None) + default_presubmit=None, may_prompt=True, rietveld_obj=None) self.failUnless(output.should_continue()) self.assertEquals(output.getvalue().count('??'), 2) self.assertEqual(output.getvalue().count( @@ -694,7 +695,7 @@ class PresubmitUnittest(PresubmitTestsBase): output = presubmit.DoPresubmitChecks( change=change, committing=False, verbose=True, output_stream=None, input_stream=None, - default_presubmit=None, may_prompt=False, gerrit_obj=None) + default_presubmit=None, may_prompt=False, rietveld_obj=None) # A warning is printed, and should_continue is True. self.failUnless(output.should_continue()) self.assertEquals(output.getvalue().count('??'), 2) @@ -725,7 +726,7 @@ class PresubmitUnittest(PresubmitTestsBase): output = presubmit.DoPresubmitChecks( change=change, committing=False, verbose=True, output_stream=None, input_stream=None, - default_presubmit=None, may_prompt=True, gerrit_obj=None) + default_presubmit=None, may_prompt=True, rietveld_obj=None) self.failIf(output.should_continue()) self.assertEqual(output.getvalue().count('??'), 0) self.assertEqual(output.getvalue().count('!!'), 2) @@ -759,7 +760,7 @@ def CheckChangeOnCommit(input_api, output_api): change=change, committing=False, verbose=True, output_stream=None, input_stream=input_buf, default_presubmit=always_fail_presubmit_script, - may_prompt=False, gerrit_obj=None) + may_prompt=False, rietveld_obj=None) self.failIf(output.should_continue()) text = ( 'Running presubmit upload checks ...\n' @@ -819,7 +820,7 @@ def CheckChangeOnCommit(input_api, output_api): change=change, committing=False, verbose=True, output_stream=output_buf, input_stream=input_buf, default_presubmit=tag_checker_presubmit_script, - may_prompt=False, gerrit_obj=None) + may_prompt=False, rietveld_obj=None) self.failUnless(presubmit_output) self.assertEquals(output_buf.getvalue(), @@ -961,7 +962,7 @@ def CheckChangeOnCommit(input_api, output_api): presubmit.DoPresubmitChecks(mox.IgnoreArg(), False, False, mox.IgnoreArg(), mox.IgnoreArg(), - None, False, None, None).AndReturn(output) + None, False, None, None, None).AndReturn(output) self.mox.ReplayAll() self.assertEquals( @@ -1022,6 +1023,7 @@ class InputApiUnittest(PresubmitTestsBase): 'environ', 'fnmatch', 'glob', + 'host_url', 'is_committing', 'is_windows', 'json', @@ -1037,6 +1039,7 @@ class InputApiUnittest(PresubmitTestsBase): 'platform', 'python_executable', 're', + 'rietveld', 'subprocess', 'tbr', 'tempfile', @@ -1059,9 +1062,10 @@ class InputApiUnittest(PresubmitTestsBase): api = presubmit.InputApi( self.fake_change, presubmit_path='foo/path/PRESUBMIT.py', - is_committing=False, gerrit_obj=None, verbose=False) + is_committing=False, rietveld_obj=None, verbose=False) self.assertEquals(api.PresubmitLocalPath(), 'foo/path') self.assertEquals(api.change, self.fake_change) + self.assertEquals(api.host_url, 'http://codereview.chromium.org') def testInputApiPresubmitScriptFiltering(self): description_lines = ('Hello there', @@ -1293,7 +1297,7 @@ class InputApiUnittest(PresubmitTestsBase): self.fake_root_dir, 'isdir', 'PRESUBMIT.py') api = presubmit.InputApi( change=change, presubmit_path=presubmit_path, - is_committing=True, gerrit_obj=None, verbose=False) + is_committing=True, rietveld_obj=None, verbose=False) paths_from_api = api.AbsoluteLocalPaths() self.assertEqual(len(paths_from_api), 2) for absolute_paths in [paths_from_change, paths_from_api]: @@ -1370,7 +1374,7 @@ class InputApiUnittest(PresubmitTestsBase): input_api = presubmit.InputApi( self.fake_change, presubmit_path='foo/path/PRESUBMIT.py', - is_committing=False, gerrit_obj=None, verbose=False) + is_committing=False, rietveld_obj=None, verbose=False) input_api.tempfile.NamedTemporaryFile = self.mox.CreateMock( input_api.tempfile.NamedTemporaryFile) input_api.tempfile.NamedTemporaryFile( @@ -1736,7 +1740,8 @@ class CannedChecksUnittest(PresubmitTestsBase): input_api.os_walk = self.mox.CreateMockAnything() input_api.os_path = presubmit.os.path input_api.re = presubmit.re - input_api.gerrit = self.mox.CreateMock(presubmit.GerritAccessor) + input_api.rietveld = self.mox.CreateMock(rietveld.Rietveld) + input_api.gerrit = None input_api.traceback = presubmit.traceback input_api.urllib2 = self.mox.CreateMock(presubmit.urllib2) input_api.unittest = unittest @@ -1750,6 +1755,7 @@ class CannedChecksUnittest(PresubmitTestsBase): input_api.is_windows = False input_api.change = change + input_api.host_url = 'http://localhost' input_api.is_committing = committing input_api.tbr = False input_api.dry_run = None @@ -1785,6 +1791,7 @@ class CannedChecksUnittest(PresubmitTestsBase): 'CheckOwners', 'CheckPatchFormatted', 'CheckGNFormatted', + 'CheckRietveldTryJobExecution', 'CheckSingletonInHeaders', 'CheckVPythonSpec', 'RunPythonUnitTests', 'RunPylint', @@ -2398,8 +2405,10 @@ class CannedChecksUnittest(PresubmitTestsBase): presubmit.OutputApi.PresubmitNotifyResult) def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None, - reviewers=None, is_committing=True, response=None, uncovered_files=None, - expected_output='', manually_specified_reviewers=None, dry_run=None, + reviewers=None, is_committing=True, + rietveld_response=None, gerrit_response=None, + uncovered_files=None, expected_output='', + manually_specified_reviewers=None, dry_run=None, modified_file='foo/xyz.cc'): if approvers is None: # The set of people who lgtm'ed a change. @@ -2422,7 +2431,10 @@ class CannedChecksUnittest(PresubmitTestsBase): change.RepositoryRoot = lambda: None affected_file = self.mox.CreateMock(presubmit.GitAffectedFile) input_api = self.MockInputApi(change, False) - input_api.gerrit = presubmit.GerritAccessor('host') + if gerrit_response: + assert not rietveld_response + input_api.rietveld = None + input_api.gerrit = presubmit.GerritAccessor('host') fake_db = self.mox.CreateMock(owners.Database) fake_db.email_regexp = input_api.re.compile(owners.BASIC_EMAIL_REGEXP) @@ -2442,22 +2454,14 @@ class CannedChecksUnittest(PresubmitTestsBase): change.AffectedFiles(file_filter=None).AndReturn([affected_file]) if not is_committing or (not tbr and issue) or ('OWNERS' in modified_file): change.OriginalOwnersFiles().AndReturn({}) - if issue and not response: - response = { - "owner": {"email": change.author_email}, - "labels": {"Code-Review": { - u'all': [ - { - u'email': a, - u'value': +1 - } for a in approvers - ], - u'default_value': 0, - u'values': {u' 0': u'No score', - u'+1': u'Looks good to me', - u'-1': u"I would prefer that you didn't submit this"} - }}, - "reviewers": {"REVIEWER": [{u'email': a}] for a in approvers}, + if issue and not rietveld_response and not gerrit_response: + rietveld_response = { + "owner_email": change.author_email, + "messages": [ + {"sender": a, "text": "I approve", "approval": True} + for a in approvers + ], + "reviewers": reviewers } if is_committing: @@ -2466,7 +2470,12 @@ class CannedChecksUnittest(PresubmitTestsBase): people = reviewers if issue: - input_api.gerrit._FetchChangeDetail = lambda _: response + if rietveld_response: + input_api.rietveld.get_issue_properties( + issue=int(input_api.change.issue), messages=True).AndReturn( + rietveld_response) + elif gerrit_response: + input_api.gerrit._FetchChangeDetail = lambda _: gerrit_response people.add(change.author_email) change.OriginalOwnersFiles().AndReturn({}) @@ -2484,25 +2493,12 @@ class CannedChecksUnittest(PresubmitTestsBase): def testCannedCheckOwners_DryRun(self): response = { - "owner": {"email": "john@example.com"}, - "labels": {"Code-Review": { - u'all': [ - { - u'email': u'ben@example.com', - u'value': 0 - }, - ], - u'approved': {u'email': u'ben@example.org'}, - u'default_value': 0, - u'values': {u' 0': u'No score', - u'+1': u'Looks good to me', - u'-1': u"I would prefer that you didn't submit this"} - }}, - "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]}, + "owner_email": "john@example.com", + "reviewers": ["ben@example.com"], } self.AssertOwnersWorks(approvers=set(), dry_run=True, - response=response, + rietveld_response=response, reviewers=set(["ben@example.com"]), expected_output='This is a dry run, but these failures would be ' + 'reported on commit:\nMissing LGTM from someone ' + @@ -2510,10 +2506,10 @@ class CannedChecksUnittest(PresubmitTestsBase): self.AssertOwnersWorks(approvers=set(['ben@example.com']), is_committing=False, - response=response, + rietveld_response=response, expected_output='') - def testCannedCheckOwners_Approved(self): + def testCannedCheckOwners_Approved_Gerrit(self): response = { "owner": {"email": "john@example.com"}, "labels": {"Code-Review": { @@ -2538,13 +2534,13 @@ class CannedChecksUnittest(PresubmitTestsBase): "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]}, } self.AssertOwnersWorks(approvers=set(['ben@example.com']), - response=response, + gerrit_response=response, is_committing=True, expected_output='') self.AssertOwnersWorks(approvers=set(['ben@example.com']), is_committing=False, - response=response, + gerrit_response=response, expected_output='') # Testing configuration with on -1..+1. @@ -2566,11 +2562,31 @@ class CannedChecksUnittest(PresubmitTestsBase): "reviewers": {"REVIEWER": [{u'email': u'ben@example.com'}]}, } self.AssertOwnersWorks(approvers=set(['ben@example.com']), - response=response, + gerrit_response=response, is_committing=True, expected_output='') - def testCannedCheckOwners_NotApproved(self): + + def testCannedCheckOwners_Approved(self): + response = { + "owner_email": "john@example.com", + "messages": [ + { + "sender": "ben@example.com", "text": "foo", "approval": True, + }, + ], + "reviewers": ["ben@example.com"], + } + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + rietveld_response=response, + expected_output='') + + self.AssertOwnersWorks(approvers=set(['ben@example.com']), + is_committing=False, + rietveld_response=response, + expected_output='') + + def testCannedCheckOwners_NotApproved_Gerrit(self): response = { "owner": {"email": "john@example.com"}, "labels": {"Code-Review": { @@ -2597,7 +2613,7 @@ class CannedChecksUnittest(PresubmitTestsBase): self.AssertOwnersWorks( approvers=set(), reviewers=set(["ben@example.com"]), - response=response, + gerrit_response=response, is_committing=True, expected_output= 'Missing LGTM from someone other than john@example.com\n') @@ -2606,7 +2622,7 @@ class CannedChecksUnittest(PresubmitTestsBase): approvers=set(), reviewers=set(["ben@example.com"]), is_committing=False, - response=response, + gerrit_response=response, expected_output='') # Testing configuration with on -1..+1. @@ -2630,26 +2646,49 @@ class CannedChecksUnittest(PresubmitTestsBase): self.AssertOwnersWorks( approvers=set(), reviewers=set(["ben@example.com"]), - response=response, + gerrit_response=response, is_committing=True, expected_output= 'Missing LGTM from someone other than john@example.com\n') + def testCannedCheckOwners_NotApproved(self): + response = { + "owner_email": "john@example.com", + "messages": [ + { + "sender": "ben@example.com", "text": "foo", "approval": False, + }, + ], + "reviewers": ["ben@example.com"], + } + self.AssertOwnersWorks( + approvers=set(), + reviewers=set(["ben@example.com"]), + rietveld_response=response, + expected_output= + 'Missing LGTM from someone other than john@example.com\n') + + self.AssertOwnersWorks( + approvers=set(), + reviewers=set(["ben@example.com"]), + is_committing=False, + rietveld_response=response, + expected_output='') + def testCannedCheckOwners_NoReviewers(self): response = { - "owner": {"email": "john@example.com"}, - "labels": {"Code-Review": { - u'default_value': 0, - u'values': {u' 0': u'No score', - u'+1': u'Looks good to me', - u'-1': u"I would prefer that you didn't submit this"} - }}, - "reviewers": {}, + "owner_email": "john@example.com", + "messages": [ + { + "sender": "ben@example.com", "text": "foo", "approval": False, + }, + ], + "reviewers": [], } self.AssertOwnersWorks( approvers=set(), reviewers=set(), - response=response, + rietveld_response=response, expected_output= 'Missing LGTM from someone other than john@example.com\n') @@ -2657,7 +2696,7 @@ class CannedChecksUnittest(PresubmitTestsBase): approvers=set(), reviewers=set(), is_committing=False, - response=response, + rietveld_response=response, expected_output='') def testCannedCheckOwners_NoIssueNoFiles(self):