diff --git a/git_cl.py b/git_cl.py index 6c06ef4032..a85226f86f 100755 --- a/git_cl.py +++ b/git_cl.py @@ -313,7 +313,8 @@ def _git_set_branch_config_value(key, value, branch=None, **kwargs): def _get_properties_from_options(options): - properties = dict(x.split('=', 1) for x in options.properties) + prop_list = getattr(options, 'properties', []) + properties = dict(x.split('=', 1) for x in prop_list) for key, val in properties.iteritems(): try: properties[key] = json.loads(val) @@ -456,9 +457,9 @@ def _trigger_try_jobs(auth_config, changelist, buckets, options, patchset): print('To see results in browser, run: git cl web') gerrit_changes = [changelist.GetGerritChange(patchset)] - shared_properties = {'category': options.category} + shared_properties = {'category': getattr(options, 'category', 'git_cl_try')} shared_properties.update(changelist.GetLegacyProperties(patchset)) - if options.clobber: + if getattr(options, 'clobber', False): shared_properties['clobber'] = True shared_properties.update(_get_properties_from_options(options) or {}) @@ -480,7 +481,7 @@ def _trigger_try_jobs(auth_config, changelist, buckets, options, patchset): 'scheduleBuild': { 'requestId': str(uuid.uuid4()), 'builder': { - 'project': options.project or project, + 'project': getattr(options, 'project', None) or project, 'bucket': bucket, 'builder': builder, }, @@ -4458,10 +4459,11 @@ def CMDupload(parser, args): parser.add_option('--r-owners', dest='add_owners_to', action='store_const', const='R', help='add a set of OWNERS to R') parser.add_option('-c', '--use-commit-queue', action='store_true', + default=False, help='tell the CQ to commit this patchset; ' - 'implies --send-mail') - parser.add_option('-d', '--cq-dry-run', dest='cq_dry_run', - action='store_true', + 'implies --send-mail') + parser.add_option('-d', '--cq-dry-run', + action='store_true', default=False, help='Send the patchset to do a CQ dry run right after ' 'upload.') parser.add_option('--preserve-tryjobs', action='store_true', @@ -4478,11 +4480,17 @@ def CMDupload(parser, args): parser.add_option('--parallel', action='store_true', help='Run all tests specified by input_api.RunTests in all ' 'PRESUBMIT files in parallel.') - parser.add_option('--no-autocc', action='store_true', help='Disables automatic addition of CC emails') parser.add_option('--private', action='store_true', help='Set the review private. This implies --no-autocc.') + parser.add_option('-R', '--retry-failed', action='store_true', + help='Retry failed tryjobs from old patchset immediately ' + 'after uploading new patchset. Cannot be used with ' + '--use-commit-queue or --cq-dry-run.') + parser.add_option('--buildbucket-host', default='cr-buildbucket.appspot.com', + help='Host of buildbucket. The default host is %default.') + auth.add_auth_options(parser) orig_args = args _add_codereview_select_options(parser) @@ -4502,8 +4510,11 @@ def CMDupload(parser, args): options.message = gclient_utils.FileRead(options.message_file) options.message_file = None - if options.cq_dry_run and options.use_commit_queue: - parser.error('Only one of --use-commit-queue and --cq-dry-run allowed.') + if ([options.cq_dry_run, + options.use_commit_queue, + options.retry_failed].count(True) > 1): + parser.error('Only one of --use-commit-queue, --cq-dry-run, or ' + '--retry-failed is allowed.') if options.use_commit_queue: options.send_mail = True @@ -4512,8 +4523,32 @@ def CMDupload(parser, args): settings.GetIsGerrit() cl = Changelist() + if options.retry_failed and not cl.GetIssue(): + print('No previous patchsets, so --retry-failed has no effect.') + options.retry_failed = False + # cl.GetMostRecentPatchset uses cached information, and can return the last + # patchset before upload. Calling it here makes it clear that it's the + # last patchset before upload. Note that GetMostRecentPatchset will fail + # if no CL has been uploaded yet. + if options.retry_failed: + patchset = cl.GetMostRecentPatchset() + + ret = cl.CMDUpload(options, args, orig_args) - return cl.CMDUpload(options, args, orig_args) + if options.retry_failed: + if ret != 0: + print('Upload failed, so --retry-failed has no effect.') + return ret + auth_config = auth.extract_auth_config_from_options(options) + builds = fetch_try_jobs( + auth_config, cl, options.buildbucket_host, patchset) + buckets = _filter_failed(builds) + if len(buckets) == 0: + print('No failed tryjobs, so --retry-failed has no effect.') + return ret + _trigger_try_jobs(auth_config, cl, buckets, options, patchset + 1) + + return ret @subcommand.usage('--description=') @@ -4808,7 +4843,7 @@ def CMDtry(parser, args): print('There are no failed jobs in the latest set of jobs ' '(patchset #%d), doing nothing.' % patchset) return 0 - num_builders = sum(len(builders) for builders in buckets.values()) + num_builders = sum(map(len, buckets.itervalues())) if num_builders > 10: confirm_or_exit('There are %d builders with failed builds.' % num_builders, action='continue') diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 2ae36b3743..9881bd29ce 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -3262,7 +3262,6 @@ 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, mockGetChangeDetail): mockGetChangeDetail.return_value = { @@ -3322,6 +3321,87 @@ class CMDTryTestCase(unittest.TestCase): 'WARNING Please specify buckets', git_cl.sys.stdout.getvalue()) +class CMDUploadTestCase(unittest.TestCase): + + def setUp(self): + super(CMDUploadTestCase, self).setUp() + mock.patch('git_cl.sys.stdout', StringIO.StringIO()).start() + mock.patch('git_cl.uuid.uuid4', _constantFn('uuid4')).start() + mock.patch('git_cl.Changelist.GetIssue', _constantFn(123456)).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()) + self.addCleanup(mock.patch.stopall) + + @mock.patch('git_cl.fetch_try_jobs') + @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 = { + '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' + }}, + }, + }, + } + mockFetchTryJobs.return_value = { + '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_called_with( + mock.ANY, mock.ANY, 'cr-buildbucket.appspot.com', 7) + buckets = {'infra/try': {'red-bot': []}} + mockTriggerTryJobs.assert_called_once_with( + mock.ANY, mock.ANY, buckets, mock.ANY, 8) + if __name__ == '__main__': logging.basicConfig( level=logging.DEBUG if '-v' in sys.argv else logging.ERROR)