Make CQ_INCLUDE_TRYBOTS support review-specific

This CL wholly revamps the way presubmit_support adds
CQ_INCLUDE_TRYBOTS lines in commit descriptions. In
particular, when the CL is being uploaded to Gerrit,
it uses our pre-existing support for manipulating git
footers to make the whole process much simpler.

R=tandrii@chromium.org

Bug: 710547
Change-Id: I5858282a44c590f131021fa3820f1cb3f70ef620
Reviewed-on: https://chromium-review.googlesource.com/487831
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

git_footers.py

@@ -127,6 +127,17 @@ def add_footer(message, key, value, after_keys=None, before_keys=None):
   return '\n'.join(top_lines + footer_lines)
 
 
+def remove_footer(message, key):
+  """Returns a message with all instances of given footer removed."""
+  key = normalize_name(key)
+  top_lines, footer_lines, _ = split_footers(message)
+  if not footer_lines:
+    return message
+  new_footer_lines = [
+      l for l in footer_lines if normalize_name(parse_footer(l)[0]) != key]
+  return '\n'.join(top_lines + new_footer_lines)
+
+
 def get_unique(footers, key):
   key = normalize_name(key)
   values = footers[key]

presubmit_support.py

@@ -43,6 +43,7 @@ from warnings import warn
 import auth
 import fix_encoding
 import gclient_utils
+import git_footers
 import gerrit_util
 import owners
 import owners_finder
@@ -286,40 +287,38 @@ class OutputApi(object):
         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):
+    include_re = re.compile(r'^CQ_INCLUDE_TRYBOTS=(.*)$', re.M | re.I)
+
+    prior_bots = []
+    if cl.IsGerrit():
+      trybot_footers = git_footers.parse_footers(description).get(
+          git_footers.normalize_name('Cq-Include-Trybots'), [])
+      for f in trybot_footers:
+        prior_bots += [b.strip() for b in f.split(';') if b.strip()]
+    else:
+      trybot_tags = include_re.finditer(description)
+      for t in trybot_tags:
+        prior_bots += [b.strip() for b in t.group(1).split(';') if b.strip()]
+
+    if set(prior_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)
+    all_bots = ';'.join(sorted(set(prior_bots) | set(bots_to_include)))
+
+    if cl.IsGerrit():
+      description = git_footers.remove_footer(
+          description, 'Cq-Include-Trybots')
+      description = git_footers.add_footer(
+          description, 'Cq-Include-Trybots', all_bots,
+          before_keys=['Change-Id'])
     else:
-      # If we're adding a new CQ_INCLUDE_TRYBOTS line then make
-      # absolutely sure to add it before any Change-Id: line, to avoid
-      # breaking Gerrit.
-      #
-      # The use of \n outside the capture group causes the last
-      # newline before Change-Id and any extra newlines after it to be
-      # consumed. They are re-added during the join operation.
-      #
-      # The filter operation drops the trailing empty string after the
-      # original string is split.
-      split_desc = filter(
-        None, re.split('\n(Change-Id: \w*)\n*', description, 1, re.M))
-      # Make sure to insert this before the last entry. For backward
-      # compatibility, ensure the CL description ends in a newline.
-      if len(split_desc) == 1:
-        insert_idx = 1
+      new_include_trybots = 'CQ_INCLUDE_TRYBOTS=%s' % all_bots
+      m = include_re.search(description)
+      if m:
+        description = include_re.sub(new_include_trybots, description)
       else:
-        insert_idx = len(split_desc) - 1
-      split_desc.insert(insert_idx, new_include_trybots)
-      new_description = '\n'.join(split_desc) + '\n'
-    cl.UpdateDescription(new_description, force=True)
+        description = '%s\n%s\n' % (description, new_include_trybots)
+
+    cl.UpdateDescription(description, force=True)
     return [self.PresubmitNotifyResult(message)]
 
 

tests/git_footers_test.py

@@ -105,6 +105,7 @@ My commit message is my best friend. It is my life. I must master it.
     self.assertEqual(
         git_footers.add_footer('', 'Key', 'Value'),
         '\nKey: Value')
+
     self.assertEqual(
         git_footers.add_footer('Header with empty line.\n\n', 'Key', 'Value'),
         'Header with empty line.\n\nKey: Value')
@@ -145,6 +146,25 @@ My commit message is my best friend. It is my life. I must master it.
               'Key', 'value', after_keys=['Other'], before_keys=['Some']),
         'Top\n\nSome: footer\nOther: footer\nKey: value\nFinal: footer')
 
+  def testRemoveFooter(self):
+    self.assertEqual(
+        git_footers.remove_footer('message', 'Key'),
+        'message')
+
+    self.assertEqual(
+        git_footers.remove_footer('message\n\nSome: footer', 'Key'),
+        'message\n\nSome: footer')
+
+    self.assertEqual(
+        git_footers.remove_footer('message\n\nSome: footer\nKey: value', 'Key'),
+        'message\n\nSome: footer')
+
+    self.assertEqual(
+        git_footers.remove_footer(
+            'message\n\nKey: value\nSome: footer\nKey: value', 'Key'),
+        'message\n\nSome: footer')
+
+
   def testReadStdin(self):
     self.mock(git_footers.sys, 'stdin', StringIO.StringIO(
         'line\r\notherline\r\n\r\n\r\nFoo: baz'))

tests/presubmit_unittest.py

@@ -155,15 +155,14 @@ class PresubmitUnittest(PresubmitTestsBase):
   def testMembersChanged(self):
     self.mox.ReplayAll()
     members = [
-      'AffectedFile', 'Change',
-      'DoPostUploadExecuter', 'DoPresubmitChecks', 'GetPostUploadExecuter',
-      'GitAffectedFile', 'CallCommand', 'CommandData',
+      'AffectedFile', 'Change', 'DoPostUploadExecuter', 'DoPresubmitChecks',
+      'GetPostUploadExecuter', 'GitAffectedFile', 'CallCommand', 'CommandData',
       'GitChange', 'InputApi', 'ListRelevantPresubmitFiles', 'main',
       'NonexistantCannedCheckFilter', 'OutputApi', 'ParseFiles',
       'PresubmitFailure', 'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs',
-      'auth', 'cPickle', 'cpplint', 'cStringIO',
-      'contextlib', 'canned_check_filter', 'fix_encoding', 'fnmatch',
-      'gclient_utils', 'glob', 'inspect', 'json', 'load_files', 'logging',
+      'auth', 'cPickle', 'cpplint', 'cStringIO', 'contextlib',
+      'canned_check_filter', 'fix_encoding', 'fnmatch', 'gclient_utils',
+      'git_footers', 'glob', 'inspect', 'json', 'load_files', 'logging',
       'marshal', 'normpath', 'optparse', 'os', 'owners', 'owners_finder',
       'pickle', 'presubmit_canned_checks', 'random', 're', 'rietveld', 'scm',
       'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest',
@@ -1406,10 +1405,12 @@ class OutputApiUnittest(PresubmitTestsBase):
     self.failIf(output.should_continue())
     self.failUnless(output.getvalue().count('???'))
 
-  def _testIncludingCQTrybots(self, cl_text, new_trybots, updated_cl_text):
+  def _testIncludingCQTrybots(self, cl_text, new_trybots, updated_cl_text,
+                              is_gerrit=False):
     class FakeCL(object):
       def __init__(self, description):
         self._description = description
+        self._is_gerrit = is_gerrit
 
       def GetDescription(self, force=False):
         return self._description
@@ -1417,6 +1418,9 @@ class OutputApiUnittest(PresubmitTestsBase):
       def UpdateDescription(self, description, force=False):
         self._description = description
 
+      def IsGerrit(self):
+        return self._is_gerrit
+
     def FakePresubmitNotifyResult(message):
       return message
 
@@ -1435,11 +1439,11 @@ class OutputApiUnittest(PresubmitTestsBase):
     # We need long lines in this test.
     # pylint: disable=line-too-long
 
-    # Deliberately has a space at the end to exercise space-stripping code.
+    # Deliberately has a spaces 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 
+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',
@@ -1461,42 +1465,39 @@ CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserve
 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
 """)
 
-    # Starting without any CQ_INCLUDE_TRYBOTS line, but with a
-    # Change-Id: line (with no trailing newline), which must continue
-    # to be the last line.
+    # All pre-existing bots are already in output set.
     self._testIncludingCQTrybots(
       """A change to GPU-related code.
-Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""",
+
+CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_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'
       ],
       """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
-Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2
+
+CQ_INCLUDE_TRYBOTS=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, but with a
-    # Change-Id: line (with a trailing newline), which must continue
-    # to be the last line.
+    # Equivalent tests for Gerrit (pre-existing Change-Id line).
     self._testIncludingCQTrybots(
       """A change to GPU-related code.
-Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2
-""",
+
+Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""",
       [
+        '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',
       ],
       """A change to GPU-related code.
-CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
-Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2
-""")
 
-    # Starting with a CQ_INCLUDE_TRYBOTS line and a Change-Id: line,
-    # the latter of which must continue to be the last line.
+Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
+Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""", is_gerrit=True)
+
     self._testIncludingCQTrybots(
       """A change to GPU-related code.
-CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel
+
+Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_rel
 Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2
 """,
       [
@@ -1504,25 +1505,25 @@ Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2
         '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
-Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2
-""")
 
-    # All pre-existing bots are already in output set.
+Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
+Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""", is_gerrit=True)
+
     self._testIncludingCQTrybots(
       """A change to GPU-related code.
 
-CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel
+Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_rel
+Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_dbg
+Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2
 """,
       [
         'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel',
-        'master.tryserver.chromium.win:win_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
-""")
-
+Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_dbg;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
+Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""", is_gerrit=True)
 
 
 class AffectedFileUnittest(PresubmitTestsBase):