git-cl: Use buildbucket v2 to fetch tryjob results.

Bug: 976104
Change-Id: Icf761f1cd093f7600ad43b71af474e52780f1997
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1842335
Reviewed-by: Anthony Polito <apolito@google.com>
Reviewed-by: Andrii Shyshkalov <tandrii@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

git_cl.py

@@ -525,16 +525,19 @@ def fetch_try_jobs(auth_config, changelist, buildbucket_host,
                    patchset=None):
   """Fetches tryjobs from buildbucket.
 
-  Returns a map from build ID to build info as a dictionary.
+  Returns list of buildbucket.v2.Build with the try jobs for the changelist.
   """
-  assert buildbucket_host
+  fields = ['id', 'builder', 'status']
-  assert changelist.GetIssue(), 'CL must be uploaded first'
+  request = {
-  assert changelist.GetCodereviewServer(), 'CL must be uploaded first'
+      'predicate': {
-  patchset = patchset or changelist.GetMostRecentPatchset()
+          'gerritChanges': [changelist.GetGerritChange(patchset)],
-  assert patchset, 'CL must be uploaded first'
+      },
+      'fields': ','.join('builds.*.' + field for field in fields),
+  }
 
   codereview_url = changelist.GetCodereviewServer()
   codereview_host = urlparse.urlparse(codereview_url).hostname
+
   authenticator = auth.get_authenticator_for_host(codereview_host, auth_config)
   if authenticator.has_cached_credentials():
     http = authenticator.authorize(httplib2.Http())
@@ -543,29 +546,10 @@ def fetch_try_jobs(auth_config, changelist, buildbucket_host,
           # Get the message on how to login.
           (auth.LoginRequiredError().message,))
     http = httplib2.Http()
-
   http.force_exception_to_status_code = True
 
-  buildset = 'patch/gerrit/{hostname}/{issue}/{patch}'.format(
+  response = _call_buildbucket(http, buildbucket_host, 'SearchBuilds', request)
-      hostname=codereview_host,
+  return response.get('builds', [])
-      issue=changelist.GetIssue(),
-      patch=patchset)
-  params = {'tag': 'buildset:%s' % buildset}
-
-  builds = {}
-  while True:
-    url = 'https://{hostname}/_ah/api/buildbucket/v1/search?{params}'.format(
-        hostname=buildbucket_host,
-        params=urllib.urlencode(params))
-    content = _buildbucket_retry('fetching tryjobs', http, url, 'GET')
-    for build in content.get('builds', []):
-      builds[build['id']] = build
-    if 'next_cursor' in content:
-      params['start_cursor'] = content['next_cursor']
-    else:
-      break
-  return builds
-
 
 def _fetch_latest_builds(
     auth_config, changelist, buildbucket_host, latest_patchset=None):
@@ -579,9 +563,8 @@ def _fetch_latest_builds(
     lastest_patchset(int|NoneType): the patchset to start fetching builds from.
       If None (default), starts with the latest available patchset.
   Returns:
-    A tuple (builds, patchset) where builds is a dict mapping from build ID to
+    A tuple (builds, patchset) where builds is a list of buildbucket.v2.Build,
-    build info from Buildbucket, and patchset is the patchset number where
+    and patchset is the patchset number where those builds came from.
-    those builds came from.
   """
   assert buildbucket_host
   assert changelist.GetIssue(), 'CL must be uploaded first'
@@ -607,23 +590,20 @@ def _filter_failed(builds):
   """Returns a list of buckets/builders that had failed builds.
 
   Args:
-    builds (dict): Builds, in the format returned by fetch_try_jobs,
+    builds (list): Builds, in the format returned by fetch_try_jobs,
-      i.e. a dict mapping build ID to build info dict, which includes
+      i.e. a list of buildbucket.v2.Builds which includes status and builder
-      the keys status, result, bucket, and builder_name.
+      info.
 
   Returns:
     A dict of bucket to builder to tests (empty list). This is the same format
     accepted by _trigger_try_jobs and returned by _get_bucket_map.
   """
   buckets = collections.defaultdict(dict)
-  for build in builds.values():
+  for build in builds:
-    if build['status'] == 'COMPLETED' and build['result'] == 'FAILURE':
+    if build['status'] in ('FAILURE', 'INFRA_FAILURE'):
-      project = build['project']
+      project = build['builder']['project']
-      bucket = build['bucket']
+      bucket = build['builder']['bucket']
-      if bucket.startswith('luci.'):
+      builder = build['builder']['builder']
-        # Assume legacy bucket name luci.<project>.<bucket>.
-        bucket = bucket.split('.')[2]
-      builder = _get_builder_from_build(build)
       buckets[project + '/' + bucket][builder] = []
   return buckets
 
@@ -634,99 +614,55 @@ def print_try_jobs(options, builds):
     print('No tryjobs scheduled.')
     return
 
-  # Make a copy, because we'll be modifying builds dictionary.
+  longest_builder = max(len(b['builder']['builder']) for b in builds)
-  builds = builds.copy()
+  name_fmt = '{builder:<%d}' % longest_builder
-  builder_names_cache = {}
-
-  def get_builder(b):
-    try:
-      return builder_names_cache[b['id']]
-    except KeyError:
-      name = _get_builder_from_build(b)
-      builder_names_cache[b['id']] = name
-      return name
-
   if options.print_master:
-    name_fmt = '%%-%ds %%-%ds' % (
+    longest_bucket = max(len(b['builder']['bucket']) for b in builds)
-        max(len(str(b['bucket'])) for b in builds.itervalues()),
+    name_fmt = ('{bucket:>%d} ' % longest_bucket) + name_fmt
-        max(len(str(get_builder(b))) for b in builds.itervalues()))
+
-    def get_name(b):
+  builds_by_status = {}
-      return name_fmt % (b['bucket'], get_builder(b))
+  for b in builds:
-  else:
+    builds_by_status.setdefault(b['status'], []).append({
-    name_fmt = '%%-%ds' % (
+        'id': b['id'],
-        max(len(str(get_builder(b))) for b in builds.itervalues()))
+        'name': name_fmt.format(
-    def get_name(b):
+            builder=b['builder']['builder'], bucket=b['builder']['bucket']),
-      return name_fmt % get_builder(b)
+    })
 
-  def sort_key(b):
+  sort_key = lambda b: (b['name'], b['id'])
-    return b['status'], b.get('result'), get_name(b), b.get('url')
 
-  def pop(title, f, color=None, **kwargs):
+  def print_builds(title, builds, fmt=None, color=None):
     """Pop matching builds from `builds` dict and print them."""
+    if not builds:
+      return
 
+    fmt = fmt or '{name} https://ci.chromium.org/b/{id}'
     if not options.color or color is None:
-      colorize = str
+      colorize = lambda x: x
     else:
       colorize = lambda x: '%s%s%s' % (color, x, Fore.RESET)
 
-    result = []
+    print(colorize(title))
-    for b in builds.values():
+    for b in sorted(builds, key=sort_key):
-      if all(b.get(k) == v for k, v in kwargs.iteritems()):
+      print(' ', colorize(fmt.format(**b)))
-        builds.pop(b['id'])
-        result.append(b)
-    if result:
-      print(colorize(title))
-      for b in sorted(result, key=sort_key):
-        print(' ', colorize('\t'.join(map(str, f(b)))))
 
   total = len(builds)
-  pop(status='COMPLETED', result='SUCCESS',
+  print_builds(
-      title='Successes:', color=Fore.GREEN,
+      'Successes:', builds_by_status.pop('SUCCESS', []), color=Fore.GREEN)
-      f=lambda b: (get_name(b), b.get('url')))
+  print_builds(
-  pop(status='COMPLETED', result='FAILURE', failure_reason='INFRA_FAILURE',
+      'Infra Failures:', builds_by_status.pop('INFRA_FAILURE', []),
-      title='Infra Failures:', color=Fore.MAGENTA,
+      color=Fore.MAGENTA)
-      f=lambda b: (get_name(b), b.get('url')))
+  print_builds('Failures:', builds_by_status.pop('FAILURE', []), color=Fore.RED)
-  pop(status='COMPLETED', result='FAILURE', failure_reason='BUILD_FAILURE',
+  print_builds('Canceled:', builds_by_status.pop('CANCELED', []), fmt='{name}',
-      title='Failures:', color=Fore.RED,
+      color=Fore.MAGENTA)
-      f=lambda b: (get_name(b), b.get('url')))
+  print_builds('Started:', builds_by_status.pop('STARTED', []))
-  pop(status='COMPLETED', result='CANCELED',
+  print_builds(
-      title='Canceled:', color=Fore.MAGENTA,
+      'Scheduled:', builds_by_status.pop('SCHEDULED', []), fmt='{name} id={id}')
-      f=lambda b: (get_name(b),))
-  pop(status='COMPLETED', result='FAILURE',
-      failure_reason='INVALID_BUILD_DEFINITION',
-      title='Wrong master/builder name:', color=Fore.MAGENTA,
-      f=lambda b: (get_name(b),))
-  pop(status='COMPLETED', result='FAILURE',
-      title='Other failures:',
-      f=lambda b: (get_name(b), b.get('failure_reason'), b.get('url')))
-  pop(status='COMPLETED',
-      title='Other finished:',
-      f=lambda b: (get_name(b), b.get('result'), b.get('url')))
-  pop(status='STARTED',
-      title='Started:', color=Fore.YELLOW,
-      f=lambda b: (get_name(b), b.get('url')))
-  pop(status='SCHEDULED',
-      title='Scheduled:',
-      f=lambda b: (get_name(b), 'id=%s' % b['id']))
   # The last section is just in case buildbucket API changes OR there is a bug.
-  pop(title='Other:',
+  print_builds(
-      f=lambda b: (get_name(b), 'id=%s' % b['id']))
+      'Other:', sum(builds_by_status.values(), []), fmt='{name} id={id}')
-  assert len(builds) == 0
   print('Total: %d tryjobs' % total)
 
 
-def _get_builder_from_build(build):
-  """Returns a builder name from a BB v1 build info dict."""
-  try:
-    parameters = json.loads(build['parameters_json'])
-    name = parameters['builder_name']
-  except (ValueError, KeyError) as error:
-    print('WARNING: Failed to get builder name for build %s: %s' % (
-          build['id'], error))
-    name = None
-  return name
-
-
 def _ComputeDiffLineRanges(files, upstream_commit):
   """Gets the changed line ranges for each file since upstream_commit.
 
@@ -809,35 +745,6 @@ def _FindYapfConfigFile(fpath, yapf_config_cache, top_dir=None):
   return ret
 
 
-def write_try_results_json(output_file, builds):
-  """Writes a subset of the data from fetch_try_jobs to a file as JSON.
-
-  The input |builds| dict is assumed to be generated by Buildbucket.
-  Buildbucket documentation: http://goo.gl/G0s101
-  """
-
-  def convert_build_dict(build):
-    """Extracts some of the information from one build dict."""
-    parameters = json.loads(build.get('parameters_json', '{}')) or {}
-    return {
-        'buildbucket_id': build.get('id'),
-        'bucket': build.get('bucket'),
-        'builder_name': parameters.get('builder_name'),
-        'created_ts': build.get('created_ts'),
-        'experimental': build.get('experimental'),
-        'failure_reason': build.get('failure_reason'),
-        'result': build.get('result'),
-        'status': build.get('status'),
-        'tags': build.get('tags'),
-        'url': build.get('url'),
-    }
-
-  converted = []
-  for _, build in sorted(builds.items()):
-    converted.append(convert_build_dict(build))
-  write_json(output_file, converted)
-
-
 def print_stats(args):
   """Prints statistics about the change to the user."""
   # --no-ext-diff is broken in some versions of Git, so try to work around
@@ -4873,7 +4780,7 @@ def CMDtry_results(parser, args):
     print('Buildbucket error: %s' % ex)
     return 1
   if options.json:
-    write_try_results_json(options.json, jobs)
+    write_json(options.json, jobs)
   else:
     print_try_jobs(options, jobs)
   return 0

tests/git_cl_test.py

@@ -2426,167 +2426,6 @@ class TestGitCl(TestCase):
     self.assertRegexpMatches(out.getvalue(), 'Issue.*123 has been submitted')
     self.assertRegexpMatches(out.getvalue(), 'Landed as: .*deadbeef')
 
-  BUILDBUCKET_BUILDS_MAP = {
-    '9000': {
-      'id': '9000',
-      'bucket': 'master.x.y',
-      'created_by': 'user:someone@chromium.org',
-      'created_ts': '147200002222000',
-      'experimental': False,
-      'parameters_json': json.dumps({
-        'builder_name': 'my-bot',
-        'properties': {'category': 'cq'},
-      }),
-      'status': 'STARTED',
-      'tags': [
-        'build_address:x.y/my-bot/2',
-        'builder:my-bot',
-        'experimental:false',
-        'user_agent:cq',
-      ],
-      'url': 'http://build.cr.org/p/x.y/builders/my-bot/builds/2',
-    },
-    '8000': {
-      'id': '8000',
-      'bucket': 'master.x.y',
-      'created_by': 'user:someone@chromium.org',
-      'created_ts': '147200001111000',
-      'experimental': False,
-      'failure_reason': 'BUILD_FAILURE',
-      'parameters_json': json.dumps({
-        'builder_name': 'my-bot',
-        'properties': {'category': 'cq'},
-      }),
-      'result_details_json': json.dumps({
-        'properties': {'buildnumber': 1},
-      }),
-      'result': 'FAILURE',
-      'status': 'COMPLETED',
-      'tags': [
-        'build_address:x.y/my-bot/1',
-        'builder:my-bot',
-        'experimental:false',
-        'user_agent:cq',
-      ],
-      'url': 'http://build.cr.org/p/x.y/builders/my-bot/builds/1',
-    },
-  }
-
-  def test_write_try_results_json(self):
-    expected_output = [
-      {
-        'bucket': 'master.x.y',
-        'buildbucket_id': '8000',
-        'builder_name': 'my-bot',
-        'created_ts': '147200001111000',
-        'experimental': False,
-        'failure_reason': 'BUILD_FAILURE',
-        'result': 'FAILURE',
-        'status': 'COMPLETED',
-        'tags': [
-          'build_address:x.y/my-bot/1',
-          'builder:my-bot',
-          'experimental:false',
-          'user_agent:cq',
-        ],
-        'url': 'http://build.cr.org/p/x.y/builders/my-bot/builds/1',
-      },
-      {
-        'bucket': 'master.x.y',
-        'buildbucket_id': '9000',
-        'builder_name': 'my-bot',
-        'created_ts': '147200002222000',
-        'experimental': False,
-        'failure_reason': None,
-        'result': None,
-        'status': 'STARTED',
-        'tags': [
-          'build_address:x.y/my-bot/2',
-          'builder:my-bot',
-          'experimental:false',
-          'user_agent:cq',
-        ],
-        'url': 'http://build.cr.org/p/x.y/builders/my-bot/builds/2',
-      },
-    ]
-    self.calls = [(('write_json', 'output.json', expected_output), '')]
-    git_cl.write_try_results_json('output.json', self.BUILDBUCKET_BUILDS_MAP)
-
-  def _setup_fetch_try_jobs(self, most_recent_patchset=20001):
-    out = StringIO.StringIO()
-    self.mock(sys, 'stdout', out)
-    self.mock(git_cl.Changelist, 'GetMostRecentPatchset',
-              lambda *args: most_recent_patchset)
-    self.mock(git_cl.auth, 'get_authenticator_for_host', lambda host, _cfg:
-              self._mocked_call(['get_authenticator_for_host', host]))
-    self.mock(git_cl, '_buildbucket_retry', lambda *_, **__:
-              self._mocked_call(['_buildbucket_retry']))
-
-  def _setup_fetch_try_jobs_gerrit(self, *request_results):
-    self._setup_fetch_try_jobs(most_recent_patchset=13)
-    self.calls += [
-      ((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
-      ((['git', 'config', 'branch.feature.gerritissue'],), '1'),
-      # TODO(tandrii): Uncomment the below if we decide to support checking
-      # patchsets for Gerrit.
-      # Simulate that Gerrit has more patchsets than local.
-      # ((['git', 'config', 'branch.feature.gerritpatchset'],), '12'),
-      ((['git', 'config', 'branch.feature.gerritserver'],),
-       'https://x-review.googlesource.com'),
-      ((['get_authenticator_for_host', 'x-review.googlesource.com'],),
-       AuthenticatorMock()),
-    ] + [((['_buildbucket_retry'],), r) for r in request_results]
-
-  def test_fetch_try_jobs_none_gerrit(self):
-    self._setup_fetch_try_jobs_gerrit({})
-    self.assertEqual(0, git_cl.main(['try-results']))
-    # TODO(tandrii): Uncomment the below if we decide to support checking
-    # patchsets for Gerrit.
-    # self.assertRegexpMatches(
-    #     sys.stdout.getvalue(),
-    #     r'Warning: Codereview server has newer patchsets \(13\)')
-    self.assertRegexpMatches(sys.stdout.getvalue(), 'No tryjobs')
-
-  def test_fetch_try_jobs_some_gerrit(self):
-    self._setup_fetch_try_jobs_gerrit({
-      'builds': self.BUILDBUCKET_BUILDS_MAP.values(),
-    })
-    # TODO(tandrii): Uncomment the below if we decide to support checking
-    # patchsets for Gerrit.
-    # self.calls.remove(
-    #     ((['git', 'config', 'branch.feature.gerritpatchset'],), '12'))
-    self.assertEqual(0, git_cl.main(['try-results', '--patchset', '5']))
-
-    # ... and doesn't result in warning.
-    self.assertNotRegexpMatches(sys.stdout.getvalue(), 'Warning')
-    self.assertRegexpMatches(sys.stdout.getvalue(), '^Failures:')
-    self.assertRegexpMatches(sys.stdout.getvalue(), 'Started:')
-    self.assertRegexpMatches(sys.stdout.getvalue(), '2 tryjobs')
-
-  def test_filter_failed_none(self):
-    self.assertEqual(git_cl._filter_failed({}), {})
-
-  def test_filter_failed_some(self):
-    builds = {
-      '9000': {
-        'id': '9000',
-        'bucket': 'luci.chromium.try',
-        'project': 'chromium',
-        'created_by': 'user:someone@chromium.org',
-        'created_ts': '147200002222000',
-        'experimental': False,
-        'parameters_json': json.dumps({
-          'builder_name': 'my-bot',
-          'properties': {'category': 'cq'},
-        }),
-        'status': 'COMPLETED',
-        'result': 'FAILURE',
-      }
-    }
-    self.assertEqual(
-      git_cl._filter_failed(builds),
-      {'chromium/try': {'my-bot': []}})
-
   def _mock_gerrit_changes_for_detail_cache(self):
     self.mock(git_cl.Changelist, '_GetGerritHost', lambda _: 'host')
 
@@ -3137,43 +2976,150 @@ class TestGitCl(TestCase):
     self.assertEqual(cl._GerritChangeIdentifier(), '123456')
 
 
-class CMDTryTestCase(unittest.TestCase):
+class CMDTestCaseBase(unittest.TestCase):
+  _STATUSES = [
+      'STATUS_UNSPECIFIED', 'SCHEDULED', 'STARTED', 'SUCCESS', 'FAILURE',
+      'INFRA_FAILURE', 'CANCELED',
+  ]
+  _CHANGE_DETAIL = {
+      'project': 'depot_tools',
+      'status': 'OPEN',
+      'owner': {'email': 'owner@e.mail'},
+      'current_revision': 'beeeeeef',
+      'revisions': {
+          'deadbeaf': {'_number': 6},
+          'beeeeeef': {
+              '_number': 7,
+              'fetch': {'http': {
+                  'url': 'https://chromium.googlesource.com/depot_tools',
+                  'ref': 'refs/changes/56/123456/7'
+              }},
+          },
+      },
+  }
+  _DEFAULT_RESPONSE = {
+      'builds': [
+          {
+              'id': str(100 + idx),
+              'builder': {
+                  'project': 'chromium',
+                  'bucket': 'try',
+                  'builder': 'bot_' + status.lower(),
+              },
+              'status': status,
+          }
+          for idx, status in enumerate(_STATUSES)
+      ]
+  }
+
   def setUp(self):
-    super(CMDTryTestCase, self).setUp()
+    super(CMDTestCaseBase, self).setUp()
     mock.patch('git_cl.sys.stdout', StringIO.StringIO()).start()
-    mock.patch('git_cl.uuid.uuid4', _constantFn('uuid4')).start()
+    mock.patch('git_cl.uuid.uuid4', return_value='uuid4').start()
-    mock.patch('git_cl.Changelist.GetIssue', _constantFn(123456)).start()
+    mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start()
     mock.patch('git_cl.Changelist.GetCodereviewServer',
-               _constantFn('https://chromium-review.googlesource.com')).start()
+               return_value='https://chromium-review.googlesource.com').start()
-    mock.patch('git_cl.Changelist.SetPatchset').start()
+    mock.patch('git_cl.Changelist.GetMostRecentPatchset',
-    mock.patch('git_cl.Changelist.GetPatchset', _constantFn(7)).start()
+               return_value=7).start()
     mock.patch('git_cl.auth.get_authenticator_for_host', AuthenticatorMock())
+    mock.patch('git_cl.Changelist._GetChangeDetail',
+               return_value=self._CHANGE_DETAIL).start()
+    mock.patch('git_cl._call_buildbucket',
+               return_value = self._DEFAULT_RESPONSE).start()
+    mock.patch('git_common.is_dirty_git_tree', return_value=False).start()
     self.addCleanup(mock.patch.stopall)
 
-  @mock.patch('git_cl.Changelist._GetChangeDetail')
+
+class CMDTryResultsTestCase(CMDTestCaseBase):
+  _DEFAULT_REQUEST = {
+      'predicate': {
+            "gerritChanges": [{
+                "project": "depot_tools",
+                "host": "chromium-review.googlesource.com",
+                "patchset": 7,
+                "change": 123456,
+            }],
+      },
+      'fields': 'builds.*.id,builds.*.builder,builds.*.status',
+  }
+
+  def testNoJobs(self):
+    git_cl._call_buildbucket.return_value = {}
+
+    self.assertEqual(0, git_cl.main(['try-results']))
+    self.assertEqual('No tryjobs scheduled.\n', sys.stdout.getvalue())
+    git_cl._call_buildbucket.assert_called_once_with(
+        mock.ANY, 'cr-buildbucket.appspot.com', 'SearchBuilds',
+        self._DEFAULT_REQUEST)
+
+  def testPrintToStdout(self):
+    self.assertEqual(0, git_cl.main(['try-results']))
+    self.assertEqual([
+        'Successes:',
+        '  bot_success            https://ci.chromium.org/b/103',
+        'Infra Failures:',
+        '  bot_infra_failure      https://ci.chromium.org/b/105',
+        'Failures:',
+        '  bot_failure            https://ci.chromium.org/b/104',
+        'Canceled:',
+        '  bot_canceled          ',
+        'Started:',
+        '  bot_started            https://ci.chromium.org/b/102',
+        'Scheduled:',
+        '  bot_scheduled          id=101',
+        'Other:',
+        '  bot_status_unspecified id=100',
+        'Total: 7 tryjobs',
+    ], sys.stdout.getvalue().splitlines())
+    git_cl._call_buildbucket.assert_called_once_with(
+        mock.ANY, 'cr-buildbucket.appspot.com', 'SearchBuilds',
+        self._DEFAULT_REQUEST)
+
+  def testPrintToStdoutWithMasters(self):
+    self.assertEqual(0, git_cl.main(['try-results', '--print-master']))
+    self.assertEqual([
+        'Successes:',
+        '  try bot_success            https://ci.chromium.org/b/103',
+        'Infra Failures:',
+        '  try bot_infra_failure      https://ci.chromium.org/b/105',
+        'Failures:',
+        '  try bot_failure            https://ci.chromium.org/b/104',
+        'Canceled:',
+        '  try bot_canceled          ',
+        'Started:',
+        '  try bot_started            https://ci.chromium.org/b/102',
+        'Scheduled:',
+        '  try bot_scheduled          id=101',
+        'Other:',
+        '  try bot_status_unspecified id=100',
+        'Total: 7 tryjobs',
+    ], sys.stdout.getvalue().splitlines())
+    git_cl._call_buildbucket.assert_called_once_with(
+        mock.ANY, 'cr-buildbucket.appspot.com', 'SearchBuilds',
+        self._DEFAULT_REQUEST)
+
+  @mock.patch('git_cl.write_json')
+  def testWriteToJson(self, mockJsonDump):
+    self.assertEqual(0, git_cl.main(['try-results', '--json', 'file.json']))
+    git_cl._call_buildbucket.assert_called_once_with(
+        mock.ANY, 'cr-buildbucket.appspot.com', 'SearchBuilds',
+        self._DEFAULT_REQUEST)
+    mockJsonDump.assert_called_once_with(
+        'file.json', self._DEFAULT_RESPONSE['builds'])
+
+  def test_filter_failed(self):
+    self.assertEqual({}, git_cl._filter_failed([]))
+    self.assertEqual({
+        'chromium/try': {'bot_failure': [], 'bot_infra_failure': []},
+    }, git_cl._filter_failed(self._DEFAULT_RESPONSE['builds']))
+
+
+class CMDTryTestCase(CMDTestCaseBase):
+
   @mock.patch('git_cl.Changelist.SetCQState')
-  @mock.patch('git_cl._get_bucket_map', _constantFn({}))
+  @mock.patch('git_cl._get_bucket_map', return_value={})
-  def testSetCQDryRunByDefault(self, mockSetCQState, mockGetChangeDetail):
+  def testSetCQDryRunByDefault(self, _mockGetBucketMap, mockSetCQState):
     mockSetCQState.return_value = 0
-    mockGetChangeDetail.return_value = {
-        'project': 'depot_tools',
-        'status': 'OPEN',
-        'owner': {'email': 'owner@e.mail'},
-        'current_revision': 'beeeeeef',
-        'revisions': {
-            'deadbeaf': {
-                '_number': 6,
-            },
-            'beeeeeef': {
-                '_number': 7,
-                'fetch': {'http': {
-                    'url': 'https://chromium.googlesource.com/depot_tools',
-                    'ref': 'refs/changes/56/123456/7'
-                }},
-            },
-        },
-    }
-
     self.assertEqual(0, git_cl.main(['try']))
     git_cl.Changelist.SetCQState.assert_called_with(git_cl._CQState.DRY_RUN)
     self.assertEqual(
@@ -3181,28 +3127,9 @@ class CMDTryTestCase(unittest.TestCase):
         'Scheduling CQ dry run on: '
         'https://chromium-review.googlesource.com/123456\n')
 
-  @mock.patch('git_cl.Changelist._GetChangeDetail')
   @mock.patch('git_cl._call_buildbucket')
-  def testScheduleOnBuildbucket(self, mockCallBuildbucket, mockGetChangeDetail):
+  def testScheduleOnBuildbucket(self, mockCallBuildbucket):
     mockCallBuildbucket.return_value = {}
-    mockGetChangeDetail.return_value = {
-        'project': 'depot_tools',
-        'status': 'OPEN',
-        'owner': {'email': 'owner@e.mail'},
-        'current_revision': 'beeeeeef',
-        'revisions': {
-            'deadbeaf': {
-                '_number': 6,
-            },
-            'beeeeeef': {
-                '_number': 7,
-                'fetch': {'http': {
-                    'url': 'https://chromium.googlesource.com/depot_tools',
-                    'ref': 'refs/changes/56/123456/7'
-                }},
-            },
-        },
-    }
 
     self.assertEqual(0, git_cl.main([
         'try', '-B', 'luci.chromium.try', '-b', 'win',
@@ -3241,27 +3168,7 @@ class CMDTryTestCase(unittest.TestCase):
     mockCallBuildbucket.assert_called_with(
         mock.ANY, 'cr-buildbucket.appspot.com', 'Batch', expected_request)
 
-  @mock.patch('git_cl.Changelist._GetChangeDetail')
+  def testScheduleOnBuildbucket_WrongBucket(self):
-  def testScheduleOnBuildbucket_WrongBucket(self, mockGetChangeDetail):
-    mockGetChangeDetail.return_value = {
-        'project': 'depot_tools',
-        'status': 'OPEN',
-        'owner': {'email': 'owner@e.mail'},
-        'current_revision': 'beeeeeef',
-        'revisions': {
-            'deadbeaf': {
-                '_number': 6,
-            },
-            'beeeeeef': {
-                '_number': 7,
-                'fetch': {'http': {
-                    'url': 'https://chromium.googlesource.com/depot_tools',
-                    'ref': 'refs/changes/56/123456/7'
-                }},
-            },
-        },
-    }
-
     self.assertEqual(0, git_cl.main([
         'try', '-B', 'not-a-bucket', '-b', 'win',
         '-p', 'key=val', '-p', 'json=[{"a":1}, null]']))
@@ -3301,95 +3208,47 @@ class CMDTryTestCase(unittest.TestCase):
         self.assertIn(expected_warning, git_cl.sys.stdout.getvalue())
 
 
-class CMDUploadTestCase(unittest.TestCase):
+class CMDUploadTestCase(CMDTestCaseBase):
-
   def setUp(self):
     super(CMDUploadTestCase, self).setUp()
-    mock.patch('git_cl.sys.stdout', StringIO.StringIO()).start()
+    mock.patch('git_cl.fetch_try_jobs').start()
-    mock.patch('git_cl.uuid.uuid4', _constantFn('uuid4')).start()
+    mock.patch('git_cl._trigger_try_jobs', return_value={}).start()
-    mock.patch('git_cl.Changelist.GetIssue', _constantFn(123456)).start()
+    mock.patch('git_cl.Changelist.CMDUpload', return_value=0).start()
-    mock.patch('git_cl.Changelist.GetCodereviewServer',
-               _constantFn('https://chromium-review.googlesource.com')).start()
-    mock.patch('git_cl.Changelist.GetMostRecentPatchset',
-               _constantFn(7)).start()
-    mock.patch('git_cl.auth.get_authenticator_for_host',
-               AuthenticatorMock()).start()
-    mock.patch('git_common.is_dirty_git_tree', return_value=False).start()
     self.addCleanup(mock.patch.stopall)
 
-  @mock.patch('git_cl.fetch_try_jobs')
+  def testUploadRetryFailed(self):
-  @mock.patch('git_cl._trigger_try_jobs')
-  @mock.patch('git_cl.Changelist._GetChangeDetail')
-  @mock.patch('git_cl.Changelist.CMDUpload', _constantFn(0))
-  def testUploadRetryFailed(self, mockGetChangeDetail, mockTriggerTryJobs,
-                            mockFetchTryJobs):
     # This test mocks out the actual upload part, and just asserts that after
     # upload, if --retry-failed is added, then the tool will fetch try jobs
     # from the previous patchset and trigger the right builders on the latest
     # patchset.
-    mockGetChangeDetail.return_value = {
+    git_cl.fetch_try_jobs.side_effect = [
-        'project': 'depot_tools',
+        # Latest patchset: No builds.
-        'status': 'OPEN',
+        [],
-        'owner': {'email': 'owner@e.mail'},
+        # Patchset before latest: Some builds.
-        'current_revision': 'beeeeeef',
+        [
-        'revisions': {
+            {
-            'deadbeaf': {
+                'id': str(100 + idx),
-                '_number': 6,
+                'builder': {
-            },
+                    'project': 'chromium',
-            'beeeeeef': {
+                    'bucket': 'try',
-                '_number': 7,
+                    'builder': 'bot_' + status.lower(),
-                'fetch': {'http': {
+                },
-                    'url': 'https://chromium.googlesource.com/depot_tools',
+                'status': status,
-                    'ref': 'refs/changes/56/123456/7'
+            }
-                }},
+            for idx, status in enumerate(self._STATUSES)
-            },
+        ],
-        },
-    }
-    mockFetchTryJobs.side_effect = [
-        # Prior patchset - no builds.
-        {},
-        # Prior to prior patchset -- some builds.
-        {
-          '9000': {
-            'id': '9000',
-            'project': 'infra',
-            'bucket': 'luci.infra.try',
-            'created_by': 'user:someone@chromium.org',
-            'created_ts': '147200002222000',
-            'experimental': False,
-            'parameters_json': json.dumps({
-              'builder_name': 'red-bot',
-              'properties': {'category': 'cq'},
-            }),
-            'status': 'COMPLETED',
-            'result': 'FAILURE',
-            'tags': ['user_agent:cq'],
-          },
-          8000: {
-            'id': '8000',
-            'project': 'infra',
-            'bucket': 'luci.infra.try',
-            'created_by': 'user:someone@chromium.org',
-            'created_ts': '147200002222020',
-            'experimental': False,
-            'parameters_json': json.dumps({
-              'builder_name': 'green-bot',
-              'properties': {'category': 'cq'},
-            }),
-            'status': 'COMPLETED',
-            'result': 'SUCCESS',
-            'tags': ['user_agent:cq'],
-          },
-        },
     ]
+
     self.assertEqual(0, git_cl.main(['upload', '--retry-failed']))
-    mockFetchTryJobs.assert_has_calls([
+    self.assertEqual([
         mock.call(mock.ANY, mock.ANY, 'cr-buildbucket.appspot.com', patchset=7),
         mock.call(mock.ANY, mock.ANY, 'cr-buildbucket.appspot.com', patchset=6),
-    ])
+    ], git_cl.fetch_try_jobs.mock_calls)
-    buckets = {'infra/try': {'red-bot': []}}
+    expected_buckets = {
-    mockTriggerTryJobs.assert_called_once_with(
+        'chromium/try': {'bot_failure': [], 'bot_infra_failure': []},
-        mock.ANY, mock.ANY, buckets, mock.ANY, 8)
+    }
+    git_cl._trigger_try_jobs.assert_called_once_with(
+        mock.ANY, mock.ANY, expected_buckets, mock.ANY, 8)
 
 if __name__ == '__main__':
   logging.basicConfig(