From bfffd45e89bb9dff665470a89ceb15c41491b69d Mon Sep 17 00:00:00 2001 From: "bulach@chromium.org" Date: Wed, 22 Feb 2012 01:13:29 +0000 Subject: [PATCH] Presubmit tests: allow 100columns limit for .java files. Chrome for Android introduced the requirement for java files. Android's style guide: http://source.android.com/source/code-style.html#limit-line-length defines 100 columns for .java files. This patch changes the presubmit tests which are also used by chromium's CQ. TEST=testCannedCheckJavaLongLines Review URL: http://codereview.chromium.org/9417023 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@122938 0039d316-1c4b-4281-b951-d872f2087c98 --- presubmit_canned_checks.py | 38 +++++++++++++++---------- tests/presubmit_unittest.py | 55 +++++++++++++++++++++++++------------ 2 files changed, 60 insertions(+), 33 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 0a92e3ae0..8a9bb0382 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -79,7 +79,7 @@ def CheckDoNotSubmitInFiles(input_api, output_api): # We want to check every text file, not just source files. file_filter = lambda x : x keyword = 'DO NOT ' + 'SUBMIT' - errors = _FindNewViolationsOfRule(lambda line : keyword not in line, + errors = _FindNewViolationsOfRule(lambda _, line : keyword not in line, input_api, file_filter) text = '\n'.join('Found %s in %s' % (keyword, loc) for loc in errors) if text: @@ -234,8 +234,8 @@ def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None, """Find all newly introduced violations of a per-line rule (a callable). Arguments: - callable_rule: a callable taking a line of input and returning True - if the rule is satisfied and False if there was a problem. + callable_rule: a callable taking a file extension and line of input and + returning True if the rule is satisfied and False if there was a problem. input_api: object to enumerate the affected files. source_file_filter: a filter to be passed to the input api. error_formatter: a callable taking (filename, line_number, line) and @@ -251,11 +251,12 @@ def _FindNewViolationsOfRule(callable_rule, input_api, source_file_filter=None, # to the SCM to determine the changed region can be quite expensive on # Win32. Assuming that most files will be kept problem-free, we can # skip the SCM operations most of the time. - if all(callable_rule(line) for line in f.NewContents()): + extension = str(f.LocalPath()).rsplit('.', 1)[-1] + if all(callable_rule(extension, line) for line in f.NewContents()): continue # No violation found in full text: can skip considering diff. for line_num, line in f.ChangedContents(): - if not callable_rule(line): + if not callable_rule(extension, line): errors.append(error_formatter(f.LocalPath(), line_num, line)) return errors @@ -274,7 +275,7 @@ def CheckChangeHasNoTabs(input_api, output_api, source_file_filter=None): ('Makefile', 'makefile') and source_file_filter(affected_file)) - tabs = _FindNewViolationsOfRule(lambda line : '\t' not in line, + tabs = _FindNewViolationsOfRule(lambda _, line : '\t' not in line, input_api, filter_more) if tabs: @@ -287,7 +288,7 @@ def CheckChangeTodoHasOwner(input_api, output_api, source_file_filter=None): """Checks that the user didn't add TODO(name) without an owner.""" unowned_todo = input_api.re.compile('TO' + 'DO[^(]') - errors = _FindNewViolationsOfRule(lambda x : not unowned_todo.search(x), + errors = _FindNewViolationsOfRule(lambda _, x : not unowned_todo.search(x), input_api, source_file_filter) errors = ['Found TO' + 'DO with no owner in ' + x for x in errors] if errors: @@ -298,7 +299,7 @@ def CheckChangeTodoHasOwner(input_api, output_api, source_file_filter=None): def CheckChangeHasNoStrayWhitespace(input_api, output_api, source_file_filter=None): """Checks that there is no stray whitespace at source lines end.""" - errors = _FindNewViolationsOfRule(lambda line : line.rstrip() == line, + errors = _FindNewViolationsOfRule(lambda _, line : line.rstrip() == line, input_api, source_file_filter) if errors: return [output_api.PresubmitPromptWarning( @@ -311,18 +312,25 @@ def CheckLongLines(input_api, output_api, maxlen=80, source_file_filter=None): """Checks that there aren't any lines longer than maxlen characters in any of the text files to be submitted. """ - # Stupidly long symbols that needs to be worked around if takes 66% of line. - long_symbol = maxlen * 2 / 3 - # Hard line length limit at 50% more. - extra_maxlen = maxlen * 3 / 2 + maxlens = { + 'java': 100, + '': maxlen, + } # Note: these are C++ specific but processed on all languages. :( MACROS = ('#define', '#include', '#import', '#pragma', '#if', '#endif') - def no_long_lines(line): - if len(line) <= maxlen: + def no_long_lines(file_extension, line): + file_maxlen = maxlens.get(file_extension, maxlens['']) + # Stupidly long symbols that needs to be worked around if takes 66% of line. + long_symbol = file_maxlen * 2 / 3 + # Hard line length limit at 50% more. + extra_maxlen = file_maxlen * 3 / 2 + + line_len = len(line) + if line_len <= file_maxlen: return True - if len(line) > extra_maxlen: + if line_len > extra_maxlen: return False return ( diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 8753dc724..1663c3263 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1498,13 +1498,16 @@ class CannedChecksUnittest(PresubmitTestsBase): self.assertEquals(len(results2), 1) self.assertEquals(results2[0].__class__, error_type) - def ContentTest(self, check, content1, content2, error_type): + def ContentTest(self, check, content1, content1_path, content2, + content2_path, error_type): """Runs a test of a content-checking rule. Args: check: the check to run. content1: content which is expected to pass the check. + content1_path: file path for content1. content2: content which is expected to fail the check. + content2_path: file path for content2. error_type: the type of the error expected for content2. """ change1 = presubmit.Change( @@ -1514,12 +1517,13 @@ class CannedChecksUnittest(PresubmitTestsBase): input_api1.AffectedFiles( include_deletes=False, file_filter=mox.IgnoreArg()).AndReturn([affected_file]) + affected_file.LocalPath().AndReturn(content1_path) affected_file.NewContents().AndReturn([ - 'ahoy', + 'afoo', content1, - 'hay', - 'yer', - 'ya']) + 'bfoo', + 'cfoo', + 'dfoo']) change2 = presubmit.Change( 'foo2', 'foo2\n', self.fake_root_dir, None, 0, 0, None) @@ -1528,19 +1532,20 @@ class CannedChecksUnittest(PresubmitTestsBase): input_api2.AffectedFiles( include_deletes=False, file_filter=mox.IgnoreArg()).AndReturn([affected_file]) + affected_file.LocalPath().AndReturn(content2_path) affected_file.NewContents().AndReturn([ - 'ahoy', + 'dfoo', content2, - 'hay', - 'yer', - 'ya']) + 'efoo', + 'ffoo', + 'gfoo']) # It falls back to ChangedContents when there is a failure. This is an # optimization since NewContents() is much faster to execute than # ChangedContents(). affected_file.ChangedContents().AndReturn([ (42, content2), - (43, 'yer'), - (23, 'ya')]) + (43, 'hfoo'), + (23, 'ifoo')]) affected_file.LocalPath().AndReturn('foo.cc') self.mox.ReplayAll() @@ -1663,14 +1668,14 @@ class CannedChecksUnittest(PresubmitTestsBase): def testCannedCheckDoNotSubmitInFiles(self): self.ContentTest( lambda x,y,z: presubmit_canned_checks.CheckDoNotSubmitInFiles(x, y), - 'DO NOTSUBMIT', 'DO NOT ' + 'SUBMIT', + 'DO NOTSUBMIT', None, 'DO NOT ' + 'SUBMIT', None, presubmit.OutputApi.PresubmitError) def testCheckChangeHasNoStrayWhitespace(self): self.ContentTest( lambda x,y,z: presubmit_canned_checks.CheckChangeHasNoStrayWhitespace(x, y), - 'Foo', 'Foo ', + 'Foo', None, 'Foo ', None, presubmit.OutputApi.PresubmitPromptWarning) def testCheckChangeHasOnlyOneEol(self): @@ -1696,12 +1701,12 @@ class CannedChecksUnittest(PresubmitTestsBase): def testCheckChangeTodoHasOwner(self): self.ContentTest(presubmit_canned_checks.CheckChangeTodoHasOwner, - "TODO(foo): bar", "TODO: bar", + "TODO(foo): bar", None, "TODO: bar", None, presubmit.OutputApi.PresubmitPromptWarning) def testCannedCheckChangeHasNoTabs(self): self.ContentTest(presubmit_canned_checks.CheckChangeHasNoTabs, - 'blah blah', 'blah\tblah', + 'blah blah', None, 'blah\tblah', None, presubmit.OutputApi.PresubmitPromptWarning) # Make sure makefiles are ignored. @@ -1716,8 +1721,10 @@ class CannedChecksUnittest(PresubmitTestsBase): affected_file3.LocalPath().AndReturn('makefile') # Only this one will trigger. affected_file4 = self.mox.CreateMock(presubmit.SvnAffectedFile) - affected_file4.LocalPath().AndReturn('makefile.foo') + affected_file1.LocalPath().AndReturn('foo.cc') affected_file1.NewContents().AndReturn(['yo, ']) + affected_file4.LocalPath().AndReturn('makefile.foo') + affected_file4.LocalPath().AndReturn('makefile.foo') affected_file4.NewContents().AndReturn(['ye\t']) affected_file4.ChangedContents().AndReturn([(46, 'ye\t')]) affected_file4.LocalPath().AndReturn('makefile.foo') @@ -1744,12 +1751,17 @@ class CannedChecksUnittest(PresubmitTestsBase): def testCannedCheckLongLines(self): check = lambda x, y, z: presubmit_canned_checks.CheckLongLines(x, y, 10, z) - self.ContentTest(check, '0123456789', '01234567890', + self.ContentTest(check, '0123456789', None, '01234567890', None, + presubmit.OutputApi.PresubmitPromptWarning) + + def testCannedCheckJavaLongLines(self): + check = lambda x, y, _: presubmit_canned_checks.CheckLongLines(x, y) + self.ContentTest(check, 'A ' * 50, 'foo.java', 'A ' * 50 + 'B', 'foo.java', presubmit.OutputApi.PresubmitPromptWarning) def testCannedCheckLongLinesLF(self): check = lambda x, y, z: presubmit_canned_checks.CheckLongLines(x, y, 10, z) - self.ContentTest(check, '012345678\n', '0123456789\n', + self.ContentTest(check, '012345678\n', None, '0123456789\n', None, presubmit.OutputApi.PresubmitPromptWarning) def testCannedCheckLongLinesMacro(self): @@ -1758,7 +1770,9 @@ class CannedChecksUnittest(PresubmitTestsBase): check, # Put a space in so it doesn't trigger long symbols. Allow 1/3 more. '#if 56 89 12 45', + None, '#if 56 89 12 456', + None, presubmit.OutputApi.PresubmitPromptWarning) def testCannedCheckLongLinesHttp(self): @@ -1766,7 +1780,9 @@ class CannedChecksUnittest(PresubmitTestsBase): self.ContentTest( check, ' http:// 0 23 5', + None, ' http:// 0 23 56', + None, presubmit.OutputApi.PresubmitPromptWarning) def testCannedCheckLongLinesLongSymbol(self): @@ -1774,7 +1790,9 @@ class CannedChecksUnittest(PresubmitTestsBase): self.ContentTest( check, ' TUP5D_LoNG_SY ', + None, ' TUP5D_LoNG_SY5 ', + None, presubmit.OutputApi.PresubmitPromptWarning) def testCheckChangeSvnEolStyleCommit(self): @@ -2257,6 +2275,7 @@ class CannedChecksUnittest(PresubmitTestsBase): for _ in range(3): input_api.AffectedFiles(file_filter=mox.IgnoreArg(), include_deletes=False ).AndReturn([affected_file]) + affected_file.LocalPath() affected_file.NewContents().AndReturn('Hey!\nHo!\nHey!\nHo!\n\n') affected_file.ChangedContents().AndReturn([ (0, 'Hey!\n'),