diff --git a/git_cl.py b/git_cl.py index 8bf9ff2b9d..ccf92ebb79 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1358,7 +1358,7 @@ class Changelist(object): def GetDescription(self, pretty=False, force=False): if not self.has_description or force: if self.GetIssue(): - self.description = self._codereview_impl.FetchDescription() + self.description = self._codereview_impl.FetchDescription(force=force) self.has_description = True if pretty: # Set width to 72 columns + 2 space indent. @@ -1676,7 +1676,7 @@ class _ChangelistCodereviewBase(object): """Returns server URL without end slash, like "https://codereview.com".""" raise NotImplementedError() - def FetchDescription(self): + def FetchDescription(self, force=False): """Fetches and returns description from the codereview server.""" raise NotImplementedError() @@ -1812,11 +1812,11 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase): if refresh: authenticator.get_access_token() - def FetchDescription(self): + def FetchDescription(self, force=False): issue = self.GetIssue() assert issue try: - return self.RpcServer().get_description(issue).strip() + return self.RpcServer().get_description(issue, force=force).strip() except urllib2.HTTPError as e: if e.code == 404: DieWithError( @@ -2401,8 +2401,9 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase): data = self._GetChangeDetail(['CURRENT_REVISION']) return data['revisions'][data['current_revision']]['_number'] - def FetchDescription(self): - data = self._GetChangeDetail(['CURRENT_REVISION', 'CURRENT_COMMIT']) + def FetchDescription(self, force=False): + data = self._GetChangeDetail(['CURRENT_REVISION', 'CURRENT_COMMIT'], + no_cache=force) current_rev = data['current_revision'] return data['revisions'][current_rev]['commit']['message'] diff --git a/presubmit_support.py b/presubmit_support.py index d71ebd26e8..7e06021ea8 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -271,6 +271,38 @@ class OutputApi(object): return self.PresubmitNotifyResult(*args, **kwargs) return self.PresubmitPromptWarning(*args, **kwargs) + def EnsureCQIncludeTrybotsAreAdded(self, cl, bots_to_include, message): + """Helper for any PostUploadHook wishing to add CQ_INCLUDE_TRYBOTS. + + Merges the bots_to_include into the current CQ_INCLUDE_TRYBOTS list, + keeping it alphabetically sorted. Returns the results that should be + returned from the PostUploadHook. + + Args: + cl: The git_cl.Changelist object. + bots_to_include: A list of strings of bots to include, in the form + "master:slave". + message: A message to be printed in the case that + CQ_INCLUDE_TRYBOTS was updated. + """ + description = cl.GetDescription(force=True) + all_bots = [] + include_re = re.compile(r'^CQ_INCLUDE_TRYBOTS=(.*)', re.M | re.I) + m = include_re.search(description) + if m: + all_bots = [i.strip() for i in m.group(1).split(';') if i.strip()] + if set(all_bots) >= set(bots_to_include): + return [] + # Sort the bots to keep them in some consistent order -- not required. + all_bots = sorted(set(all_bots) | set(bots_to_include)) + new_include_trybots = 'CQ_INCLUDE_TRYBOTS=%s' % ';'.join(all_bots) + if m: + new_description = include_re.sub(new_include_trybots, description) + else: + new_description = description + '\n' + new_include_trybots + '\n' + cl.UpdateDescription(new_description, force=True) + return [self.PresubmitNotifyResult(message)] + class InputApi(object): """An instance of this object is passed to presubmit scripts so they can @@ -1096,42 +1128,6 @@ def DoPostUploadExecuter(change, return results -# This helper function should be used by any PostUploadHook wishing to -# add entries to the CQ_INCLUDE_TRYBOTS line. It returns the results -# that should be returned from the PostUploadHook. -def EnsureCQIncludeTrybotsAreAdded(cl, bots_to_include, message, output_api): - """Helper to be used by any PostUploadHook wishing to add CQ_INCLUDE_TRYBOTS. - - Merges the bots_to_include into the current CQ_INCLUDE_TRYBOTS list, - keeping it alphabetically sorted. Returns the results that should be - returned from the PostUploadHook. - - Args: - cl: The git_cl.Changelist object. - bots_to_include: A list of strings of bots to include, in the form - "master:slave". - message: A message to be printed in the case that - CQ_INCLUDE_TRYBOTS was updated. - output_api: An OutputApi instance used to display messages. - """ - rietveld_obj = cl.RpcServer() - issue = cl.issue - description = rietveld_obj.get_description(issue) - all_bots = [] - include_re = re.compile(r'^CQ_INCLUDE_TRYBOTS=(.*)', re.M | re.I) - m = include_re.search(description) - if m: - all_bots = [i.strip() for i in m.group(1).split(';') if i.strip()] - if not (set(all_bots) - set(bots_to_include)): - return [] - # Sort the bots to keep them in some consistent order -- not required. - all_bots = sorted(set(all_bots) | set(bots_to_include)) - new_include_trybots = 'CQ_INCLUDE_TRYBOTS=%s' % ';'.join(all_bots) - new_description = include_re.sub(new_include_trybots, description) - rietveld_obj.update_description(issue, new_description) - return [output_api.PresubmitNotifyResult(message)] - - class PresubmitExecuter(object): def __init__(self, change, committing, rietveld_obj, verbose, gerrit_obj=None, dry_run=None): diff --git a/rietveld.py b/rietveld.py index 5ef935e35d..9677f7ac9f 100644 --- a/rietveld.py +++ b/rietveld.py @@ -72,7 +72,7 @@ class Rietveld(object): logging.info('closing issue %d' % issue) self.post("/%d/close" % issue, [('xsrf_token', self.xsrf_token())]) - def get_description(self, issue): + def get_description(self, issue, force=False): """Returns the issue's description. Converts any CRLF into LF and strip extraneous whitespace. @@ -654,11 +654,14 @@ class CachingRietveld(Rietveld): function_cache[args] = update(*args) return copy.deepcopy(function_cache[args]) - def get_description(self, issue): - return self._lookup( - 'get_description', - (issue,), - super(CachingRietveld, self).get_description) + def get_description(self, issue, force=False): + if force: + return super(CachingRietveld, self).get_description(issue, force=force) + else: + return self._lookup( + 'get_description', + (issue,), + super(CachingRietveld, self).get_description) def get_issue_properties(self, issue, messages): """Returns the issue properties. diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index bf6ba23042..d4be5b9919 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -38,7 +38,7 @@ class ChangelistMock(object): pass def GetIssue(self): return 1 - def GetDescription(self): + def GetDescription(self, force=False): return ChangelistMock.desc def UpdateDescription(self, desc, force=False): ChangelistMock.desc = desc @@ -57,7 +57,7 @@ class RietveldMock(object): pass @staticmethod - def get_description(issue): + def get_description(issue, force=False): return 'Issue: %d' % issue @staticmethod @@ -178,7 +178,7 @@ class TestGitClBasic(unittest.TestCase): codereview_host='host') cl.description = 'x' cl.has_description = True - cl._codereview_impl.FetchDescription = lambda: 'y' + cl._codereview_impl.FetchDescription = lambda *a, **kw: 'y' self.assertEquals(cl.GetDescription(), 'x') self.assertEquals(cl.GetDescription(force=True), 'y') self.assertEquals(cl.GetDescription(), 'y') @@ -1695,7 +1695,7 @@ class TestGitCl(TestCase): self.mock(git_cl._RietveldChangelistImpl, 'GetMostRecentPatchset', lambda x: '60001') self.mock(git_cl._RietveldChangelistImpl, 'FetchDescription', - lambda *args: 'Description') + lambda *a, **kw: 'Description') self.mock(git_cl, 'IsGitVersionAtLeast', lambda *args: True) if new_branch: diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 0e7cc85255..831651a58d 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -168,7 +168,7 @@ class PresubmitUnittest(PresubmitTestsBase): 'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest', 'urllib2', 'warn', 'multiprocessing', 'DoGetTryMasters', 'GetTryMastersExecuter', 'itertools', 'urlparse', 'gerrit_util', - 'GerritAccessor', 'EnsureCQIncludeTrybotsAreAdded', + 'GerritAccessor', ] # If this test fails, you should add the relevant test. self.compareMembers(presubmit, members) @@ -955,53 +955,6 @@ def CheckChangeOnCommit(input_api, output_api): except SystemExit, e: self.assertEquals(2, e.code) - def testEnsureCQIncludeTrybotsAreAdded(self): - # Deliberately has a space at the end to exercise space-stripping code. - cl_text = """A change to GPU-related code. - -CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel -""" - - updated_cl_text = """A change to GPU-related code. - -CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel -""" - - class FakeIssue(object): - def __init__(self, description): - self.description = description - - class FakeRietveld(object): - def get_description(self, issue): - return issue.description - - def update_description(self, issue, new_description): - issue.description = new_description - - class FakeCL(object): - def __init__(self, description): - self.issue = FakeIssue(description) - def RpcServer(self): - return FakeRietveld() - - class FakeOutputAPI(object): - def PresubmitNotifyResult(self, message): - return message - - cl = FakeCL(cl_text) - message = 'Automatically added optional GPU tests to run on CQ.' - results = presubmit.EnsureCQIncludeTrybotsAreAdded( - cl, - [ - 'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel', - 'master.tryserver.chromium.mac:mac_optional_gpu_tests_rel', - 'master.tryserver.chromium.win:win_optional_gpu_tests_rel' - ], - message, - FakeOutputAPI()) - self.assertEqual(updated_cl_text, cl.issue.description) - self.assertEqual([message], results) - class InputApiUnittest(PresubmitTestsBase): """Tests presubmit.InputApi.""" @@ -1387,7 +1340,7 @@ class OutputApiUnittest(PresubmitTestsBase): members = [ 'MailTextResult', 'PresubmitError', 'PresubmitNotifyResult', 'PresubmitPromptWarning', 'PresubmitPromptOrNotify', 'PresubmitResult', - 'is_committing', + 'is_committing', 'EnsureCQIncludeTrybotsAreAdded', ] # If this test fails, you should add the relevant test. self.compareMembers(presubmit.OutputApi(False), members) @@ -1451,6 +1404,77 @@ class OutputApiUnittest(PresubmitTestsBase): self.failIf(output.should_continue()) self.failUnless(output.getvalue().count('???')) + def _testIncludingCQTrybots(self, cl_text, new_trybots, updated_cl_text): + class FakeCL(object): + def __init__(self, description): + self._description = description + + def GetDescription(self, force=False): + return self._description + + def UpdateDescription(self, description, force=False): + self._description = description + + def FakePresubmitNotifyResult(message): + return message + + cl = FakeCL(cl_text) + output_api = presubmit.OutputApi(False) + output_api.PresubmitNotifyResult = FakePresubmitNotifyResult + message = 'Automatically added optional bots to run on CQ.' + results = output_api.EnsureCQIncludeTrybotsAreAdded( + cl, + new_trybots, + message) + self.assertEqual(updated_cl_text, cl.GetDescription()) + self.assertEqual([message], results) + + def testEnsureCQIncludeTrybotsAreAdded(self): + # We need long lines in this test. + # pylint: disable=line-too-long + + # Deliberately has a space at the end to exercise space-stripping code. + self._testIncludingCQTrybots( + """A change to GPU-related code. + +CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel +""", + [ + 'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel', + 'master.tryserver.chromium.win:win_optional_gpu_tests_rel' + ], + """A change to GPU-related code. + +CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel +""") + + # Starting without any CQ_INCLUDE_TRYBOTS line. + self._testIncludingCQTrybots( + """A change to GPU-related code.""", + [ + 'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel', + 'master.tryserver.chromium.mac:mac_optional_gpu_tests_rel', + ], + """A change to GPU-related code. +CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel +""") + + # All pre-existing bots are already in output set. + self._testIncludingCQTrybots( + """A change to GPU-related code. + +CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel +""", + [ + 'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel', + 'master.tryserver.chromium.win:win_optional_gpu_tests_rel' + ], + """A change to GPU-related code. + +CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel +""") + + class AffectedFileUnittest(PresubmitTestsBase): def testMembersChanged(self):