From 8c5a353e63ac55ace8f761478f085b3be5e65fc2 Mon Sep 17 00:00:00 2001 From: tandrii Date: Fri, 4 Nov 2016 07:52:02 -0700 Subject: [PATCH] Implement and test git cl try for Gerrit. R=borenet@chromium.org,qyearsley@chromium.org BUG=599931 Review-Url: https://codereview.chromium.org/2468263005 --- git_cl.py | 91 +++++++++++++++---------- tests/git_cl_test.py | 158 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 202 insertions(+), 47 deletions(-) diff --git a/git_cl.py b/git_cl.py index 51593f20e..66bb76831 100755 --- a/git_cl.py +++ b/git_cl.py @@ -427,7 +427,6 @@ def _trigger_try_jobs(auth_config, changelist, buckets, options, # _RietveldChangelistImpl does, then caching values in these two variables # won't be necessary. owner_email = changelist.GetIssueOwner() - project = changelist.GetIssueProject() buildbucket_put_url = ( 'https://{hostname}/_ah/api/buildbucket/v1/builds/batch'.format( @@ -437,7 +436,14 @@ def _trigger_try_jobs(auth_config, changelist, buckets, options, hostname=codereview_host, issue=changelist.GetIssue(), patch=patchset) + + shared_parameters_properties = changelist.GetTryjobProperties(patchset) + shared_parameters_properties['category'] = category + if options.clobber: + shared_parameters_properties['clobber'] = True extra_properties = _get_properties_from_options(options) + if extra_properties: + shared_parameters_properties.update(extra_properties) batch_req_body = {'builds': []} print_text = [] @@ -455,23 +461,12 @@ def _trigger_try_jobs(auth_config, changelist, buckets, options, 'author': {'email': owner_email}, 'revision': options.revision, }], - 'properties': { - 'category': category, - 'issue': changelist.GetIssue(), - 'patch_project': project, - 'patch_storage': 'rietveld', - 'patchset': patchset, - 'rietveld': codereview_url, - }, + 'properties': shared_parameters_properties.copy(), } if 'presubmit' in builder.lower(): parameters['properties']['dry_run'] = 'true' if tests: parameters['properties']['testfilter'] = tests - if extra_properties: - parameters['properties'].update(extra_properties) - if options.clobber: - parameters['properties']['clobber'] = True tags = [ 'builder:%s' % builder, @@ -1691,12 +1686,6 @@ class Changelist(object): """Get owner from codereview, which may differ from this checkout.""" return self._codereview_impl.GetIssueOwner() - def GetIssueProject(self): - """Get project from codereview, which may differ from what this - checkout's codereview.settings or gerrit project URL say. - """ - return self._codereview_impl.GetIssueProject() - def GetApprovingReviewers(self): return self._codereview_impl.GetApprovingReviewers() @@ -1707,6 +1696,10 @@ class Changelist(object): """Returns reason (str) if unable trigger tryjobs on this CL or None.""" return self._codereview_impl.CannotTriggerTryJobReason() + def GetTryjobProperties(self, patchset=None): + """Returns dictionary of properties to launch tryjob.""" + return self._codereview_impl.GetTryjobProperties(patchset=patchset) + def __getattr__(self, attr): # This is because lots of untested code accesses Rietveld-specific stuff # directly, and it's hard to fix for sure. So, just let it work, and fix @@ -1839,7 +1832,7 @@ class _ChangelistCodereviewBase(object): def GetIssueOwner(self): raise NotImplementedError() - def GetIssueProject(self): + def GetTryjobProperties(self, patchset=None): raise NotImplementedError() @@ -1920,15 +1913,23 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): return 'CL %s is private' % self.GetIssue() return None + def GetTryjobProperties(self, patchset=None): + """Returns dictionary of properties to launch tryjob.""" + project = (self.GetIssueProperties() or {}).get('project') + return { + 'issue': self.GetIssue(), + 'patch_project': project, + 'patch_storage': 'rietveld', + 'patchset': patchset or self.GetPatchset(), + 'rietveld': self.GetCodereviewServer(), + } + def GetApprovingReviewers(self): return get_approving_reviewers(self.GetIssueProperties()) def GetIssueOwner(self): return (self.GetIssueProperties() or {}).get('owner_email') - def GetIssueProject(self): - return (self.GetIssueProperties() or {}).get('project') - def AddComment(self, message): return self.RpcServer().add_comment(self.GetIssue(), message) @@ -2868,16 +2869,38 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): labels={'Commit-Queue': vote_map[new_state]}) def CannotTriggerTryJobReason(self): - # TODO(tandrii): implement for Gerrit. - raise NotImplementedError() + try: + data = self._GetChangeDetail() + except GerritIssueNotExists: + return 'Gerrit doesn\'t know about your issue %s' % self.GetIssue() - def GetIssueOwner(self): - # TODO(tandrii): implement for Gerrit. - raise NotImplementedError() + if data['status'] in ('ABANDONED', 'MERGED'): + return 'CL %s is closed' % self.GetIssue() - def GetIssueProject(self): - # TODO(tandrii): implement for Gerrit. - raise NotImplementedError() + def GetTryjobProperties(self, patchset=None): + """Returns dictionary of properties to launch tryjob.""" + data = self._GetChangeDetail(['ALL_REVISIONS']) + patchset = int(patchset or self.GetPatchset()) + assert patchset + revision_data = None # Pylint wants it to be defined. + for revision_data in data['revisions'].itervalues(): + if int(revision_data['_number']) == patchset: + break + else: + raise Exception('Patchset %d is not known in Gerrit issue %d' % + (patchset, self.GetIssue())) + return { + 'patch_issue': self.GetIssue(), + 'patch_set': patchset or self.GetPatchset(), + 'patch_project': data['project'], + 'patch_storage': 'gerrit', + 'patch_ref': revision_data['fetch']['http']['ref'], + 'patch_repository_url': revision_data['fetch']['http']['url'], + 'patch_gerrit_url': self.GetCodereviewServer(), + } + + def GetIssueOwner(self): + return self._GetChangeDetail(['DETAILED_ACCOUNTS'])['owner']['email'] _CODEREVIEW_IMPLEMENTATIONS = { @@ -4828,12 +4851,6 @@ def CMDtry(parser, args): if not cl.GetIssue(): parser.error('Need to upload first') - if cl.IsGerrit(): - parser.error( - 'Not yet supported for Gerrit (http://crbug.com/599931).\n' - 'If your project has Commit Queue, dry run is a workaround:\n' - ' git cl set-commit --dry-run') - error_message = cl.CannotTriggerTryJobReason() if error_message: parser.error('Can\'t trigger try jobs: %s' % error_message) diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 0d8b285d3..37c2fa13a 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1912,10 +1912,58 @@ class TestGitCl(TestCase): out.getvalue(), 'scheduled CQ Dry Run on https://codereview.chromium.org/123\n') - def test_git_cl_try_buildbucket_with_properties(self): - self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda _: 20001) - self.mock(git_cl.Changelist, 'GetIssueOwner', lambda _: 'owner@e.mail') - self.mock(git_cl.Changelist, 'GetIssueProject', lambda _: 'depot_tools') + def test_git_cl_try_default_cq_dry_run_gerrit(self): + self.mock(git_cl.Changelist, 'GetChange', + lambda _, *a: ( + self._mocked_call(['GetChange']+list(a)))) + self.mock(git_cl.presubmit_support, 'DoGetTryMasters', + lambda *_, **__: ( + self._mocked_call(['DoGetTryMasters']))) + self.mock(git_cl._GerritChangelistImpl, 'SetCQState', + lambda _, s: self._mocked_call(['SetCQState', s])) + + def _GetChangeDetail(gerrit_change_list_impl, opts=None): + # Get realistic expectations. + gerrit_change_list_impl._GetGerritHost() + return self._mocked_call(['_GetChangeDetail', opts or []]) + self.mock(git_cl._GerritChangelistImpl, '_GetChangeDetail', + _GetChangeDetail) + + self.calls = [ + ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), + ((['git', 'config', 'branch.feature.rietveldissue'],), CERR1), + ((['git', 'config', 'branch.feature.gerritissue'],), '123456'), + ((['git', 'config', 'branch.feature.gerritserver'],), + 'https://chromium-review.googlesource.com'), + ((['_GetChangeDetail', []],), {'status': 'OPEN'}), + ((['git', 'config', 'branch.feature.merge'],), 'feature'), + ((['git', 'config', 'branch.feature.remote'],), 'origin'), + ((['get_or_create_merge_base', 'feature', 'feature'],), + 'fake_ancestor_sha'), + ((['GetChange', 'fake_ancestor_sha', None], ), + git_cl.presubmit_support.GitChange( + '', '', '', '', '', '', '', '')), + ((['git', 'rev-parse', '--show-cdup'],), '../'), + ((['DoGetTryMasters'], ), None), + ((['SetCQState', git_cl._CQState.DRY_RUN], ), None), + ] + out = StringIO.StringIO() + self.mock(git_cl.sys, 'stdout', out) + self.assertEqual(0, git_cl.main(['try'])) + self.assertEqual( + out.getvalue(), + 'scheduled CQ Dry Run on ' + 'https://chromium-review.googlesource.com/123456\n') + + def test_git_cl_try_buildbucket_with_properties_rietveld(self): + self.mock(git_cl._RietveldChangelistImpl, 'GetIssueProperties', + lambda _: { + 'owner_email': 'owner@e.mail', + 'private': False, + 'closed': False, + 'project': 'depot_tools', + 'patchsets': [20001], + }) self.mock(git_cl.uuid, 'uuid4', lambda: 'uuid4') self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), @@ -1923,8 +1971,8 @@ class TestGitCl(TestCase): ((['git', 'config', 'rietveld.autoupdate'],), CERR1), ((['git', 'config', 'rietveld.server'],), 'https://codereview.chromium.org'), - ((['git', 'config', 'branch.feature.rietveldserver'],), CERR1), ((['git', 'config', 'branch.feature.rietveldpatchset'],), '20001'), + ((['git', 'config', 'branch.feature.rietveldserver'],), CERR1), ] def _buildbucket_retry(*_, **kw): @@ -1968,10 +2016,100 @@ class TestGitCl(TestCase): git_cl.sys.stdout.getvalue(), 'Tried jobs on:\nBucket: master.tryserver.chromium') + def test_git_cl_try_buildbucket_with_properties_gerrit(self): + self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda _: 7) + self.mock(git_cl.uuid, 'uuid4', lambda: 'uuid4') + + def _GetChangeDetail(gerrit_change_list_impl, opts=None): + # Get realistic expectations. + gerrit_change_list_impl._GetGerritHost() + return self._mocked_call(['_GetChangeDetail', opts or []]) + self.mock(git_cl._GerritChangelistImpl, '_GetChangeDetail', + _GetChangeDetail) + + self.calls = [ + ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), + ((['git', 'config', 'branch.feature.rietveldissue'],), CERR1), + ((['git', 'config', 'branch.feature.gerritissue'],), '123456'), + ((['git', 'config', 'branch.feature.gerritserver'],), + 'https://chromium-review.googlesource.com'), + ((['_GetChangeDetail', []],), {'status': 'OPEN'}), + ((['git', 'config', 'branch.feature.gerritpatchset'],), '7'), + ((['_GetChangeDetail', ['DETAILED_ACCOUNTS']],), + {'owner': {'email': 'owner@e.mail'}}), + ((['_GetChangeDetail', ['ALL_REVISIONS']],), { + 'project': 'depot_tools', + 'revisions': { + 'deadbeaf': { + '_number': 6, + }, + 'beeeeeef': { + '_number': 7, + 'fetch': {'http': { + 'url': 'https://chromium.googlesource.com/depot_tools', + 'ref': 'refs/changes/56/123456/7' + }}, + }, + }, + }), + ] + + def _buildbucket_retry(*_, **kw): + # self.maxDiff = 10000 + body = json.loads(kw['body']) + self.assertEqual(len(body['builds']), 1) + build = body['builds'][0] + params = json.loads(build.pop('parameters_json')) + self.assertEqual(params, { + u'builder_name': u'win', + u'changes': [{u'author': {u'email': u'owner@e.mail'}, + u'revision': None}], + u'properties': { + u'category': u'git_cl_try', + u'key': u'val', + u'json': [{u'a': 1}, None], + u'master': u'tryserver.chromium', + + u'patch_gerrit_url': + u'https://chromium-review.googlesource.com', + u'patch_issue': 123456, + u'patch_project': u'depot_tools', + u'patch_ref': u'refs/changes/56/123456/7', + u'patch_repository_url': + u'https://chromium.googlesource.com/depot_tools', + u'patch_set': 7, + u'patch_storage': u'gerrit', + } + }) + self.assertEqual(build, { + u'bucket': u'master.tryserver.chromium', + u'client_operation_id': u'uuid4', + u'tags': [ + u'builder:win', + u'buildset:patch/gerrit/chromium-review.googlesource.com/123456/7', + u'user_agent:git_cl_try', + u'master:tryserver.chromium'], + }) + + self.mock(git_cl, '_buildbucket_retry', _buildbucket_retry) + + self.mock(git_cl.sys, 'stdout', StringIO.StringIO()) + self.assertEqual(0, git_cl.main([ + 'try', '-m', 'tryserver.chromium', '-b', 'win', + '-p', 'key=val', '-p', 'json=[{"a":1}, null]'])) + self.assertRegexpMatches( + git_cl.sys.stdout.getvalue(), + 'Tried jobs on:\nBucket: master.tryserver.chromium') + def test_git_cl_try_buildbucket_bucket_flag(self): - self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda _: 20001) - self.mock(git_cl.Changelist, 'GetIssueOwner', lambda _: 'owner@e.mail') - self.mock(git_cl.Changelist, 'GetIssueProject', lambda _: 'depot_tools') + self.mock(git_cl._RietveldChangelistImpl, 'GetIssueProperties', + lambda _: { + 'owner_email': 'owner@e.mail', + 'private': False, + 'closed': False, + 'project': 'depot_tools', + 'patchsets': [20001], + }) self.mock(git_cl.uuid, 'uuid4', lambda: 'uuid4') self.calls = [ ((['git', 'symbolic-ref', 'HEAD'],), 'feature'), @@ -1979,8 +2117,8 @@ class TestGitCl(TestCase): ((['git', 'config', 'rietveld.autoupdate'],), CERR1), ((['git', 'config', 'rietveld.server'],), 'https://codereview.chromium.org'), - ((['git', 'config', 'branch.feature.rietveldserver'],), CERR1), ((['git', 'config', 'branch.feature.rietveldpatchset'],), '20001'), + ((['git', 'config', 'branch.feature.rietveldserver'],), CERR1), ] def _buildbucket_retry(*_, **kw): @@ -2025,7 +2163,7 @@ class TestGitCl(TestCase): ((['git', 'config', 'branch.feature.rietveldissue'],), '123'), ((['git', 'config', 'rietveld.autoupdate'],), CERR1), ((['git', 'config', 'rietveld.server'],), - 'https://codereview.chromium.org'), + 'https://codereview.chromium.org'), ((['git', 'config', 'branch.feature.rietveldserver'],), CERR1), ((['git', 'config', 'branch.feature.rietveldpatchset'],), '20001'), ]