Moved EnsureCQIncludeTrybotsAreAdded to OutputApi.

This makes them accessible to presubmit scripts' PostUploadHook.

Fixed bugs where caching needed to be bypassed in order for
sequential PostUploadHooks to see each others' results.

BUG=688765

Change-Id: I56c0c6b6419e2474f4b7f701be036fb2a524f8e3
Reviewed-on: https://chromium-review.googlesource.com/439877
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Paweł Hajdan Jr. <phajdan.jr@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Kenneth Russell <kbr@chromium.org>

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']
 

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):

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.

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:

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):