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)