Refactor PRESUBMIT support for tags

We want PRESUBMIT to be able to equally support
BUG= tags (old style) and Git-Footer: footers
(new style). This change refactors the way that
the presubmit api gives access to those properties
so that it is easier to add support for equivalent
footers.

It also limits the scope of tags/footers that it
exposes, as code search shows no PRESUBMIT files
that take advantage of any of the more esoteric
ones.

Bug: 710327, 710803
Change-Id: I86f1d6cb2e1f0aff9653ef3fb455e0a6f47acf5d
Reviewed-on: https://chromium-review.googlesource.com/506450
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

presubmit_canned_checks.py

@@ -39,39 +39,13 @@ BLACKLIST_LINT_FILTERS = [
 
 ### Description checks
 
-def CheckChangeHasTestField(input_api, output_api):
-  """Requires that the changelist have a TEST= field."""
-  if input_api.change.TEST:
-    return []
-  else:
-    return [output_api.PresubmitNotifyResult(
-        'If this change requires manual test instructions to QA team, add '
-        'TEST=[instructions].')]
-
-
 def CheckChangeHasBugField(input_api, output_api):
-  """Requires that the changelist have a BUG= field."""
+  """Requires that the changelist have a Bug: field."""
-  if input_api.change.BUG:
+  if input_api.change.BugsFromDescription():
     return []
   else:
     return [output_api.PresubmitNotifyResult(
-        'If this change has an associated bug, add BUG=[bug number].')]
+        'If this change has an associated bug, add Bug: [bug number].')]
-
-
-def CheckChangeHasTestedField(input_api, output_api):
-  """Requires that the changelist have a TESTED= field."""
-  if input_api.change.TESTED:
-    return []
-  else:
-    return [output_api.PresubmitError('Changelist must have a TESTED= field.')]
-
-
-def CheckChangeHasQaField(input_api, output_api):
-  """Requires that the changelist have a QA= field."""
-  if input_api.change.QA:
-    return []
-  else:
-    return [output_api.PresubmitError('Changelist must have a QA= field.')]
 
 
 def CheckDoNotSubmitInDescription(input_api, output_api):
@@ -948,10 +922,8 @@ def _GetRietveldIssueProps(input_api, messages):
 def _ReviewersFromChange(change):
   """Return the reviewers specified in the |change|, if any."""
   reviewers = set()
-  if change.R:
+  reviewers.update(change.ReviewersFromDescription())
-    reviewers.update(set([r.strip() for r in change.R.split(',')]))
+  reviewers.update(change.TBRsFromDescription())
-  if change.TBR:
-    reviewers.update(set([r.strip() for r in change.TBR.split(',')]))
 
   # Drop reviewers that aren't specified in email address format.
   return set(reviewer for reviewer in reviewers if '@' in reviewer)

presubmit_support.py

@@ -873,6 +873,30 @@ class Change(object):
       raise AttributeError(self, attr)
     return self.tags.get(attr)
 
+  # TODO(agable): Update these to also get "Git-Footer: Foo"-style footers.
+  def BugsFromDescription(self):
+    """Returns all bugs referenced in the commit description."""
+    return [b.strip() for b in self.tags.get('BUG', '').split(',') if b.strip()]
+
+  def ReviewersFromDescription(self):
+    """Returns all reviewers listed in the commit description."""
+    return [r.strip() for r in self.tags.get('R', '').split(',') if r.strip()]
+
+  def TBRsFromDescription(self):
+    """Returns all TBR reviewers listed in the commit description."""
+    return [r.strip() for r in self.tags.get('TBR', '').split(',') if r.strip()]
+
+  # TODO(agable): Delete these once we're sure they're unused.
+  @property
+  def BUG(self):
+    return ','.join(self.BugsFromDescription())
+  @property
+  def R(self):
+    return ','.join(self.ReviewersFromDescription())
+  @property
+  def TBR(self):
+    return ','.join(self.TBRsFromDescription())
+
   def AllFiles(self, root=None):
     """List all files under source control in the repo."""
     raise NotImplementedError()

tests/presubmit_unittest.py

@@ -40,9 +40,9 @@ class PresubmitTestsBase(SuperMoxTestBase):
   """Sets up and tears down the mocks but doesn't test anything as-is."""
   presubmit_text = """
 def CheckChangeOnUpload(input_api, output_api):
-  if input_api.change.ERROR:
+  if input_api.change.tags.get('ERROR'):
     return [output_api.PresubmitError("!!")]
-  if input_api.change.PROMPT_WARNING:
+  if input_api.change.tags.get('PROMPT_WARNING'):
     return [output_api.PresubmitPromptWarning("??")]
   else:
     return ()
@@ -304,7 +304,6 @@ class PresubmitUnittest(PresubmitTestsBase):
     description_lines = ('Hello there',
                          'this is a change',
                          'BUG=123',
-                         ' STORY =http://foo/  \t',
                          'and some more regular text  \t')
     unified_diff = [
         'diff --git binary_a.png binary_a.png',
@@ -430,9 +429,7 @@ class PresubmitUnittest(PresubmitTestsBase):
     self.failUnless(change.FullDescriptionText() ==
                     '\n'.join(description_lines))
 
-    self.failUnless(change.BUG == '123')
+    self.failUnless(change.BugsFromDescription() == ['123'])
-    self.failUnless(change.STORY == 'http://foo/')
-    self.failUnless(change.BLEH == None)
 
     self.failUnless(len(change.AffectedFiles()) == 7)
     self.failUnless(len(change.AffectedFiles()) == 7)
@@ -496,7 +493,7 @@ class PresubmitUnittest(PresubmitTestsBase):
   def testExecPresubmitScript(self):
     description_lines = ('Hello there',
                          'this is a change',
-                         'STORY=http://tracker/123')
+                         'BUG=123')
     files = [
       ['A', 'foo\\blat.cc'],
     ]
@@ -530,22 +527,13 @@ class PresubmitUnittest(PresubmitTestsBase):
 
     self.failIf(executer.ExecPresubmitScript(
       ('def CheckChangeOnUpload(input_api, output_api):\n'
-       '  if not input_api.change.STORY:\n'
+       '  if not input_api.change.BugsFromDescription():\n'
        '    return (output_api.PresubmitError("!!"))\n'
        '  else:\n'
        '    return ()'),
       fake_presubmit
     ))
 
-    self.failUnless(executer.ExecPresubmitScript(
-      ('def CheckChangeOnCommit(input_api, output_api):\n'
-       '  if not input_api.change.ERROR:\n'
-       '    return [output_api.PresubmitError("!!")]\n'
-       '  else:\n'
-       '    return ()'),
-      fake_presubmit
-    ))
-
     self.assertRaises(presubmit.PresubmitFailure,
       executer.ExecPresubmitScript,
       'def CheckChangeOnCommit(input_api, output_api):\n'
@@ -741,10 +729,6 @@ def CheckChangeOnUpload(input_api, output_api):
     return [output_api.PresubmitError('Tag parsing failed. 1')]
   if input_api.change.tags['STORY'] != 'http://tracker.com/42':
     return [output_api.PresubmitError('Tag parsing failed. 2')]
-  if input_api.change.BUG != 'boo':
-    return [output_api.PresubmitError('Tag parsing failed. 6')]
-  if input_api.change.STORY != 'http://tracker.com/42':
-    return [output_api.PresubmitError('Tag parsing failed. 7')]
   try:
     y = False
     x = input_api.change.invalid
@@ -1588,6 +1572,8 @@ class ChangeUnittest(PresubmitTestsBase):
         'AllFiles', 'DescriptionText', 'FullDescriptionText',
         'LocalPaths', 'Name', 'OriginalOwnersFiles', 'RepositoryRoot',
         'RightHandSideLines', 'SetDescriptionText', 'TAG_LINE_RE',
+        'BUG', 'R', 'TBR', 'BugsFromDescription',
+        'ReviewersFromDescription', 'TBRsFromDescription',
         'author_email', 'issue', 'patchset', 'scm', 'tags',
     ]
     # If this test fails, you should add the relevant test.
@@ -1603,7 +1589,6 @@ class ChangeUnittest(PresubmitTestsBase):
     self.assertEquals('foo1', change.Name())
     self.assertEquals('foo2', change.DescriptionText())
     self.assertEquals('foo3', change.author_email)
-    self.assertEquals('ro', change.DRU)
     self.assertEquals(3, change.issue)
     self.assertEquals(5, change.patchset)
     self.assertEquals(self.fake_root_dir, change.RepositoryRoot())
@@ -1615,13 +1600,10 @@ class ChangeUnittest(PresubmitTestsBase):
         '', 'foo\nDRU=ro', self.fake_root_dir, [], 3, 5, '')
     self.assertEquals('foo', change.DescriptionText())
     self.assertEquals('foo\nDRU=ro', change.FullDescriptionText())
-    self.assertEquals('ro', change.DRU)
 
     change.SetDescriptionText('bar\nWHIZ=bang')
     self.assertEquals('bar', change.DescriptionText())
     self.assertEquals('bar\nWHIZ=bang', change.FullDescriptionText())
-    self.assertEquals('bang', change.WHIZ)
-    self.assertFalse(change.DRU)
 
 
 def CommHelper(input_api, cmd, ret=None, **kwargs):
@@ -1684,8 +1666,6 @@ class CannedChecksUnittest(PresubmitTestsBase):
       'CheckChangeHasOnlyOneEol', 'CheckChangeHasNoCR',
       'CheckChangeHasNoCrAndHasOnlyOneEol', 'CheckChangeHasNoTabs',
       'CheckChangeTodoHasOwner',
-      'CheckChangeHasQaField', 'CheckChangeHasTestedField',
-      'CheckChangeHasTestField',
       'CheckChangeLintsClean',
       'CheckChangeWasUploaded',
       'CheckDoNotSubmit',
@@ -1820,24 +1800,6 @@ class CannedChecksUnittest(PresubmitTestsBase):
                          presubmit.OutputApi.PresubmitError,
                          True)
 
-  def testCannedCheckChangeHasTestField(self):
-    self.DescriptionTest(presubmit_canned_checks.CheckChangeHasTestField,
-                         'Foo\nTEST=did some stuff', 'Foo\n',
-                         presubmit.OutputApi.PresubmitNotifyResult,
-                         False)
-
-  def testCannedCheckChangeHasTestedField(self):
-    self.DescriptionTest(presubmit_canned_checks.CheckChangeHasTestedField,
-                         'Foo\nTESTED=did some stuff', 'Foo\n',
-                         presubmit.OutputApi.PresubmitError,
-                         False)
-
-  def testCannedCheckChangeHasQAField(self):
-    self.DescriptionTest(presubmit_canned_checks.CheckChangeHasQaField,
-                         'Foo\nQA=BSOD your machine', 'Foo\n',
-                         presubmit.OutputApi.PresubmitError,
-                         False)
-
   def testCannedCheckDoNotSubmitInDescription(self):
     self.DescriptionTest(presubmit_canned_checks.CheckDoNotSubmitInDescription,
                          'Foo\nDO NOTSUBMIT', 'Foo\nDO NOT ' + 'SUBMIT',
@@ -2298,8 +2260,8 @@ class CannedChecksUnittest(PresubmitTestsBase):
     change = self.mox.CreateMock(presubmit.Change)
     change.issue = issue
     change.author_email = 'john@example.com'
-    change.R = ','.join(manually_specified_reviewers)
+    change.ReviewersFromDescription = lambda: manually_specified_reviewers
-    change.TBR = ''
+    change.TBRsFromDescription = lambda: []
     change.RepositoryRoot = lambda: None
     affected_file = self.mox.CreateMock(presubmit.GitAffectedFile)
     input_api = self.MockInputApi(change, False)