From 123a468d14c58e78685f4d2bf166b4c285870f2d Mon Sep 17 00:00:00 2001 From: qyearsley Date: Wed, 26 Oct 2016 09:12:17 -0700 Subject: [PATCH] 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 | 68 +++++++++++++++++++------------------------- tests/git_cl_test.py | 43 +++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 39 deletions(-) diff --git a/git_cl.py b/git_cl.py index 27dd8b4ed..6b0d30491 100755 --- a/git_cl.py +++ b/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, diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 4b8052178..4b4e295e2 100755 --- a/tests/git_cl_test.py +++ b/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',