From ab05d582faad5164a1c1bb1abeb387dae328f889 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Wed, 9 Feb 2011 23:41:22 +0000 Subject: [PATCH] Modify presubmit checks to work on changed lines only. Unittest is being also updated. TEST=manual . run ./presubmit_unittest.py, observe success .create a CL with code style violations (long lines, traling spaces) and observe the violations reported by 'git cl presubmit' Patch contributed by Vadim Review URL: http://codereview.chromium.org/6461011 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@74378 0039d316-1c4b-4281-b951-d872f2087c98 --- presubmit_support.py | 43 +++++++++++--- tests/presubmit_unittest.py | 115 ++++++++++++++++++++++++++++-------- 2 files changed, 127 insertions(+), 31 deletions(-) diff --git a/presubmit_support.py b/presubmit_support.py index 049438d75..6be415dce 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -86,11 +86,9 @@ def PromptYesNo(input_stream, output_stream, prompt): def _RightHandSideLinesImpl(affected_files): """Implements RightHandSideLines for InputApi and GclChange.""" for af in affected_files: - lines = af.NewContents() - line_number = 0 + lines = af.ChangedContents() for line in lines: - line_number += 1 - yield (af, line_number, line) + yield (af, line[0], line[1]) class OutputApi(object): @@ -188,7 +186,7 @@ class InputApi(object): DEFAULT_WHITE_LIST = ( # C++ and friends r".*\.c$", r".*\.cc$", r".*\.cpp$", r".*\.h$", r".*\.m$", r".*\.mm$", - r".*\.inl$", r".*\.asm$", r".*\.hxx$", r".*\.hpp$", + r".*\.inl$", r".*\.asm$", r".*\.hxx$", r".*\.hpp$", r".*\.s$", r".*\.S$", # Scripts r".*\.js$", r".*\.py$", r".*\.sh$", r".*\.rb$", r".*\.pl$", r".*\.pm$", # No extension at all, note that ALL CAPS files are black listed in @@ -209,7 +207,7 @@ class InputApi(object): r".*\bxcodebuild[\\\/].*", r".*\bsconsbuild[\\\/].*", # All caps files like README and LICENCE. - r".*\b[A-Z0-9_]+$", + r".*\b[A-Z0-9_]{2,}$", # SCM (can happen in dual SCM configuration). (Slightly over aggressive) r"(|.*[\\\/])\.git[\\\/].*", r"(|.*[\\\/])\.svn[\\\/].*", @@ -345,8 +343,8 @@ class InputApi(object): Note: Copy-paste this function to suit your needs or use a lambda function. """ def Find(affected_file, items): + local_path = affected_file.LocalPath() for item in items: - local_path = affected_file.LocalPath() if self.re.match(item, local_path): logging.debug("%s matched %s" % (item, local_path)) return True @@ -481,9 +479,36 @@ class AffectedFile(object): """ raise NotImplementedError() # Implement if/when needed. + def ChangedContents(self): + """Returns a list of tuples (line number, line text) of all new lines. + + This relies on the scm diff output describing each changed code section + with a line of the form + + ^@@ , , @@$ + """ + new_lines = [] + line_num = 0 + + if self.IsDirectory(): + return [] + + for line in self.GenerateScmDiff().splitlines(): + m = re.match(r'^@@ [0-9\,\+\-]+ \+([0-9]+)\,[0-9]+ @@', line) + if m: + line_num = int(m.groups(1)[0]) + continue + if line.startswith('+') and not line.startswith('++'): + new_lines.append((line_num, line[1:])) + if not line.startswith('-'): + line_num += 1 + return new_lines + def __str__(self): return self.LocalPath() + def GenerateScmDiff(self): + raise NotImplementedError() # Implemented in derived classes. class SvnAffectedFile(AffectedFile): """Representation of a file in a change out of a Subversion checkout.""" @@ -532,6 +557,8 @@ class SvnAffectedFile(AffectedFile): self._is_text_file = (not mime_type or mime_type.startswith('text/')) return self._is_text_file + def GenerateScmDiff(self): + return scm.SVN.GenerateDiff(self.AbsoluteLocalPath()) class GitAffectedFile(AffectedFile): """Representation of a file in a change out of a git checkout.""" @@ -577,6 +604,8 @@ class GitAffectedFile(AffectedFile): self._is_text_file = os.path.isfile(self.AbsoluteLocalPath()) return self._is_text_file + def GenerateScmDiff(self): + return scm.GIT.GenerateDiff(self._local_root, files=[self.LocalPath(),]) class Change(object): """Describe a change. diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index d8f186b5c..679a574e3 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -37,6 +37,65 @@ def GetPreferredTrySlaves(): return %s """ + presubmit_diffs = """ +--- file1 2011-02-09 10:38:16.517224845 -0800 ++++ file2 2011-02-09 10:38:53.177226516 -0800 +@@ -1,6 +1,5 @@ + this is line number 0 + this is line number 1 +-this is line number 2 to be deleted + this is line number 3 + this is line number 4 + this is line number 5 +@@ -8,7 +7,7 @@ + this is line number 7 + this is line number 8 + this is line number 9 +-this is line number 10 to be modified ++this is line number 10 + this is line number 11 + this is line number 12 + this is line number 13 +@@ -21,9 +20,8 @@ + this is line number 20 + this is line number 21 + this is line number 22 +-this is line number 23 +-this is line number 24 +-this is line number 25 ++this is line number 23.1 ++this is line number 25.1 + this is line number 26 + this is line number 27 + this is line number 28 +@@ -31,6 +29,7 @@ + this is line number 30 + this is line number 31 + this is line number 32 ++this is line number 32.1 + this is line number 33 + this is line number 34 + this is line number 35 +@@ -38,14 +37,14 @@ + this is line number 37 + this is line number 38 + this is line number 39 +- + this is line number 40 +-this is line number 41 ++this is line number 41.1 + this is line number 42 + this is line number 43 + this is line number 44 + this is line number 45 ++ + this is line number 46 + this is line number 47 +-this is line number 48 ++this is line number 48.1 + this is line number 49 +""" + def setUp(self): SuperMoxTestBase.setUp(self) self.mox.StubOutWithMock(presubmit, 'random') @@ -56,6 +115,7 @@ def GetPreferredTrySlaves(): self.mox.StubOutWithMock(presubmit.scm.SVN, 'GetFileProperty') self.mox.StubOutWithMock(presubmit.gclient_utils, 'FileRead') self.mox.StubOutWithMock(presubmit.gclient_utils, 'FileWrite') + self.mox.StubOutWithMock(presubmit.scm.SVN, 'GenerateDiff') class PresubmitUnittest(PresubmitTestsBase): @@ -197,8 +257,9 @@ class PresubmitUnittest(PresubmitTestsBase): presubmit.scm.SVN.CaptureInfo(notfound).AndReturn({}) presubmit.scm.SVN.CaptureInfo(flap).AndReturn( {'URL': 'svn:/foo/boo/flap.h'}) - presubmit.gclient_utils.FileRead(blat, 'rU').AndReturn('boo!\nahh?') - presubmit.gclient_utils.FileRead(notfound, 'rU').AndReturn('look!\nthere?') + presubmit.scm.SVN.GenerateDiff(blat).AndReturn(self.presubmit_diffs) + presubmit.scm.SVN.GenerateDiff(notfound).AndReturn(self.presubmit_diffs) + self.mox.ReplayAll() change = presubmit.SvnChange('mychange', '\n'.join(description_lines), @@ -242,20 +303,24 @@ class PresubmitUnittest(PresubmitTestsBase): for line in change.RightHandSideLines(): rhs_lines.append(line) self.assertEquals(rhs_lines[0][0].LocalPath(), files[0][1]) - self.assertEquals(rhs_lines[0][1], 1) - self.assertEquals(rhs_lines[0][2],'boo!') + self.assertEquals(rhs_lines[0][1], 10) + self.assertEquals(rhs_lines[0][2],'this is line number 10') + + self.assertEquals(rhs_lines[3][0].LocalPath(), files[0][1]) + self.assertEquals(rhs_lines[3][1], 32) + self.assertEquals(rhs_lines[3][2], 'this is line number 32.1') - self.assertEquals(rhs_lines[1][0].LocalPath(), files[0][1]) - self.assertEquals(rhs_lines[1][1], 2) - self.assertEquals(rhs_lines[1][2], 'ahh?') + self.assertEquals(rhs_lines[8][0].LocalPath(), files[3][1]) + self.assertEquals(rhs_lines[8][1], 23) + self.assertEquals(rhs_lines[8][2], 'this is line number 23.1') - self.assertEquals(rhs_lines[2][0].LocalPath(), files[3][1]) - self.assertEquals(rhs_lines[2][1], 1) - self.assertEquals(rhs_lines[2][2], 'look!') + self.assertEquals(rhs_lines[12][0].LocalPath(), files[3][1]) + self.assertEquals(rhs_lines[12][1], 46) + self.assertEquals(rhs_lines[12][2], '') - self.assertEquals(rhs_lines[3][0].LocalPath(), files[3][1]) - self.assertEquals(rhs_lines[3][1], 2) - self.assertEquals(rhs_lines[3][2], 'there?') + self.assertEquals(rhs_lines[13][0].LocalPath(), files[3][1]) + self.assertEquals(rhs_lines[13][1], 49) + self.assertEquals(rhs_lines[13][2], 'this is line number 48.1') def testExecPresubmitScript(self): description_lines = ('Hello there', @@ -711,10 +776,9 @@ class InputApiUnittest(PresubmitTestsBase): presubmit.scm.SVN.GetFileProperty(another, 'svn:mime-type').AndReturn(None) presubmit.scm.SVN.GetFileProperty(third_party, 'svn:mime-type' ).AndReturn(None) - presubmit.gclient_utils.FileRead(blat, 'rU' - ).AndReturn('whatever\ncookie') - presubmit.gclient_utils.FileRead(another, 'rU' - ).AndReturn('whatever\ncookie2') + presubmit.scm.SVN.GenerateDiff(blat).AndReturn(self.presubmit_diffs) + presubmit.scm.SVN.GenerateDiff(another).AndReturn(self.presubmit_diffs) + self.mox.ReplayAll() change = presubmit.SvnChange('mychange', '\n'.join(description_lines), @@ -737,14 +801,14 @@ class InputApiUnittest(PresubmitTestsBase): # binary isn't a text file and beingdeleted doesn't exist. The rest is # outside foo/. rhs_lines = [x for x in input_api.RightHandSideLines(None)] - self.assertEquals(len(rhs_lines), 4) + self.assertEquals(len(rhs_lines), 14) self.assertEqual(rhs_lines[0][0].LocalPath(), presubmit.normpath(files[0][1])) - self.assertEqual(rhs_lines[1][0].LocalPath(), + self.assertEqual(rhs_lines[3][0].LocalPath(), presubmit.normpath(files[0][1])) - self.assertEqual(rhs_lines[2][0].LocalPath(), + self.assertEqual(rhs_lines[7][0].LocalPath(), presubmit.normpath(files[4][1])) - self.assertEqual(rhs_lines[3][0].LocalPath(), + self.assertEqual(rhs_lines[13][0].LocalPath(), presubmit.normpath(files[4][1])) def testDefaultWhiteListBlackListFilters(self): @@ -758,11 +822,13 @@ class InputApiUnittest(PresubmitTestsBase): f('experimental/b'), f('a/experimental'), f('a/experimental.cc'), + f('a/experimental.S'), ], [ # Expected. 'a/experimental', 'a/experimental.cc', + 'a/experimental.S', ], ), ( @@ -808,7 +874,7 @@ class InputApiUnittest(PresubmitTestsBase): input_api = presubmit.InputApi(None, './PRESUBMIT.py', False) self.mox.ReplayAll() - self.assertEqual(len(input_api.DEFAULT_WHITE_LIST), 20) + self.assertEqual(len(input_api.DEFAULT_WHITE_LIST), 22) self.assertEqual(len(input_api.DEFAULT_BLACK_LIST), 9) for item in files: results = filter(input_api.FilterSourceFile, item[0]) @@ -1018,8 +1084,9 @@ class AffectedFileUnittest(PresubmitTestsBase): def testMembersChanged(self): self.mox.ReplayAll() members = [ - 'AbsoluteLocalPath', 'Action', 'IsDirectory', 'IsTextFile', 'LocalPath', - 'NewContents', 'OldContents', 'OldFileTempPath', 'Property', 'ServerPath', + 'AbsoluteLocalPath', 'Action', 'ChangedContents', 'GenerateScmDiff', + 'IsDirectory', 'IsTextFile', 'LocalPath', 'NewContents', 'OldContents', + 'OldFileTempPath', 'Property', 'ServerPath', ] # If this test fails, you should add the relevant test. self.compareMembers(presubmit.AffectedFile('a', 'b'), members)