git cl try: support multiple bots from different masters without specifying master.

It might be good to make this change after the refactoring CL http://crrev.com/2442153002.

BUG=640740

Review-Url: https://codereview.chromium.org/2439293002

git_cl.py

@@ -363,6 +363,7 @@ def _get_bucket_map(changelist, options, option_parser):
 
     # Fall back to deprecated method: get try slaves from PRESUBMIT.py
     # files.
+    # TODO(qyearsley): Remove this.
     options.bot = presubmit_support.DoGetTrySlaves(
         change=change,
         changed_files=change.LocalPaths(),
@@ -378,10 +379,22 @@ def _get_bucket_map(changelist, options, option_parser):
   if options.bucket:
     return {options.bucket: {b: [] for b in options.bot}}
 
+  if not options.master:
+    bucket_map, error_message = _get_bucket_map_for_builders(options.bot)
+    if error_message:
+      option_parser.error(
+          'Tryserver master cannot be found because: %s\n'
+          'Please manually specify the tryserver master, e.g. '
+          '"-m tryserver.chromium.linux".' % error_message)
+    return bucket_map
+
   builders_and_tests = {}
 
   # TODO(machenbach): The old style command-line options don't support
   # multiple try masters yet.
+  # TODO(qyearsley): If options.bot is always a list of strings, then
+  # "new_style" never applies, and so we should remove support for Specifying
+  # test filters completely.
   old_style = filter(lambda x: isinstance(x, basestring), options.bot)
   new_style = filter(lambda x: isinstance(x, tuple), options.bot)
 
@@ -396,58 +409,37 @@ def _get_bucket_map(changelist, options, option_parser):
   for bot, tests in new_style:
     builders_and_tests.setdefault(bot, []).extend(tests)
 
-  if not options.master:
-    # TODO(qyearsley): crbug.com/640740
-    options.master, error_message = _get_builder_master(options.bot)
-    if error_message:
-      option_parser.error(
-          'Tryserver master cannot be found because: %s\n'
-          'Please manually specify the tryserver master, e.g. '
-          '"-m tryserver.chromium.linux".' % error_message)
-
-  # Return a master map with one master to be backwards compatible. The
-  # master name defaults to an empty string, which will cause the master
-  # not to be set on rietveld (deprecated).
-  bucket = ''
-  if options.master:
-    # Add the "master." prefix to the master name to obtain the bucket name.
-    bucket = _prefix_master(options.master)
+  # Add the "master." prefix to the master name to obtain the bucket name.
+  bucket = _prefix_master(options.master)
   return {bucket: builders_and_tests}
 
 
-def _get_builder_master(bot_list):
-  """Fetches a master for the given list of builders.
-
-  Returns a pair (master, error_message), where either master or
-  error_message is None.
-  """
+def _get_bucket_map_for_builders(builders):
+  """Returns a map of buckets to builders for the given builders."""
   map_url = 'https://builders-map.appspot.com/'
   try:
-    master_map = json.load(urllib2.urlopen(map_url))
+    builders_map = json.load(urllib2.urlopen(map_url))
   except urllib2.URLError as e:
     return None, ('Failed to fetch builder-to-master map from %s. Error: %s.' %
                   (map_url, e))
   except ValueError as e:
     return None, ('Invalid json string from %s. Error: %s.' % (map_url, e))
-  if not master_map:
+  if not builders_map:
     return None, 'Failed to build master map.'
 
-  result_master = ''
-  for bot in bot_list:
-    builder = bot.split(':', 1)[0]
-    master_list = master_map.get(builder, [])
-    if not master_list:
+  bucket_map = {}
+  for builder in builders:
+    builder = builder.split(':', 1)[0]
+    masters = builders_map.get(builder, [])
+    if not masters:
       return None, ('No matching master for builder %s.' % builder)
-    elif len(master_list) > 1:
+    if len(masters) > 1:
       return None, ('The builder name %s exists in multiple masters %s.' %
-                    (builder, master_list))
-    else:
-      cur_master = master_list[0]
-      if not result_master:
-        result_master = cur_master
-      elif result_master != cur_master:
-        return None, 'The builders do not belong to the same master.'
-  return result_master, None
+                    (builder, masters))
+    bucket = _prefix_master(masters[0])
+    bucket_map.setdefault(bucket, {})[builder] = []
+
+  return bucket_map, None
 
 
 def _trigger_try_jobs(auth_config, changelist, buckets, options,

tests/git_cl_test.py

@@ -1988,7 +1988,6 @@ class TestGitCl(TestCase):
     ]
 
     def _buildbucket_retry(*_, **kw):
-      # self.maxDiff = 10000
       body = json.loads(kw['body'])
       self.assertEqual(len(body['builds']), 1)
       build = body['builds'][0]
@@ -2023,6 +2022,48 @@ class TestGitCl(TestCase):
         git_cl.sys.stdout.getvalue(),
         'Tried jobs on:\nBucket: test.bucket')
 
+  def test_git_cl_try_bots_on_multiple_masters(self):
+    self.mock(git_cl.Changelist, 'GetMostRecentPatchset', lambda _: 20001)
+    self.calls = [
+      ((['git', 'symbolic-ref', 'HEAD'],), 'feature'),
+      ((['git', 'config', 'branch.feature.rietveldissue'],), '123'),
+      ((['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'),
+    ]
+
+    def _buildbucket_retry(*_, **kw):
+      body = json.loads(kw['body'])
+      self.assertEqual(len(body['builds']), 2)
+
+      first_build_params = json.loads(body['builds'][0]['parameters_json'])
+      self.assertEqual(first_build_params['builder_name'], 'builder1')
+      self.assertEqual(first_build_params['properties']['master'], 'master1')
+
+      first_build_params = json.loads(body['builds'][1]['parameters_json'])
+      self.assertEqual(first_build_params['builder_name'], 'builder2')
+      self.assertEqual(first_build_params['properties']['master'], 'master2')
+
+    self.mock(git_cl, '_buildbucket_retry', _buildbucket_retry)
+
+    self.mock(git_cl.urllib2, 'urlopen', lambda _: StringIO.StringIO(
+      json.dumps({'builder1': ['master1'], 'builder2': ['master2']})))
+
+    self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
+    self.assertEqual(
+        0, git_cl.main(['try', '-b', 'builder1', '-b', 'builder2']))
+    self.assertEqual(
+        git_cl.sys.stdout.getvalue(),
+        'Tried jobs on:\n'
+        'Bucket: master.master1\n'
+        '  builder1: []\n'
+        'Bucket: master.master2\n'
+        '  builder2: []\n'
+        'To see results here, run:        git cl try-results\n'
+        'To see results in browser, run:  git cl web\n')
+
   def _common_GerritCommitMsgHookCheck(self):
     self.mock(git_cl.sys, 'stdout', StringIO.StringIO())
     self.mock(git_cl.os.path, 'abspath',