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

@@ -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)

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'),
     ]