From fc03e67e9ae41efc4be498ae91f6d6eddc96f1c6 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Mon, 15 May 2017 14:09:42 -0700 Subject: [PATCH] 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 Reviewed-by: Andrii Shyshkalov --- presubmit_canned_checks.py | 38 ++++--------------------- presubmit_support.py | 24 ++++++++++++++++ tests/presubmit_unittest.py | 56 ++++++------------------------------- 3 files changed, 38 insertions(+), 80 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 0f6b72755..9535c6c10 100644 --- a/presubmit_canned_checks.py +++ b/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.""" - if input_api.change.BUG: + """Requires that the changelist have a Bug: field.""" + if input_api.change.BugsFromDescription(): return [] else: return [output_api.PresubmitNotifyResult( - '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.')] + 'If this change has an associated bug, add Bug: [bug number].')] 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(set([r.strip() for r in change.R.split(',')])) - if change.TBR: - reviewers.update(set([r.strip() for r in change.TBR.split(',')])) + reviewers.update(change.ReviewersFromDescription()) + reviewers.update(change.TBRsFromDescription()) # Drop reviewers that aren't specified in email address format. return set(reviewer for reviewer in reviewers if '@' in reviewer) diff --git a/presubmit_support.py b/presubmit_support.py index b089c0d0a..81634c592 100755 --- a/presubmit_support.py +++ b/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() diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 272e0b670..3f41753b0 100755 --- a/tests/presubmit_unittest.py +++ b/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.STORY == 'http://foo/') - self.failUnless(change.BLEH == None) + self.failUnless(change.BugsFromDescription() == ['123']) 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.TBR = '' + change.ReviewersFromDescription = lambda: manually_specified_reviewers + change.TBRsFromDescription = lambda: [] change.RepositoryRoot = lambda: None affected_file = self.mox.CreateMock(presubmit.GitAffectedFile) input_api = self.MockInputApi(change, False)