2011-03-29 David Levin <levin@chromium.org>
authorlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 Mar 2011 02:00:49 +0000 (02:00 +0000)
committerlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 30 Mar 2011 02:00:49 +0000 (02:00 +0000)
        Reviewed by Shinichiro Hamaji.

        check-webkit-style confused by two ChangeLog entries in a row from same user
        https://bugs.webkit.org/show_bug.cgi?id=57250

        * Scripts/webkitpy/style/checker.py: Add the line should be checked function to ChangeLogChecker.
        * Scripts/webkitpy/style/checker_unittest.py: Fix test due to that new function.
        * Scripts/webkitpy/style/checkers/changelog.py: Made this code aware of what lines were being checked.
          It basically assumes only one ChangeLog entry is being processed because that is the standard case and
          checking more than that would be very messey.
        * Scripts/webkitpy/style/checkers/changelog_unittest.py: Add testing to catch the broken case.
        * Scripts/webkitpy/style/error_handlers.py: Added should_line_be_checked.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@82382 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/style/checker.py
Tools/Scripts/webkitpy/style/checker_unittest.py
Tools/Scripts/webkitpy/style/checkers/changelog.py
Tools/Scripts/webkitpy/style/checkers/changelog_unittest.py
Tools/Scripts/webkitpy/style/error_handlers.py

index 096ab30..4276b82 100644 (file)
@@ -1,3 +1,18 @@
+2011-03-29  David Levin  <levin@chromium.org>
+
+        Reviewed by Shinichiro Hamaji.
+
+        check-webkit-style confused by two ChangeLog entries in a row from same user
+        https://bugs.webkit.org/show_bug.cgi?id=57250
+
+        * Scripts/webkitpy/style/checker.py: Add the line should be checked function to ChangeLogChecker.
+        * Scripts/webkitpy/style/checker_unittest.py: Fix test due to that new function.
+        * Scripts/webkitpy/style/checkers/changelog.py: Made this code aware of what lines were being checked.
+          It basically assumes only one ChangeLog entry is being processed because that is the standard case and
+          checking more than that would be very messey.
+        * Scripts/webkitpy/style/checkers/changelog_unittest.py: Add testing to catch the broken case.
+        * Scripts/webkitpy/style/error_handlers.py: Added should_line_be_checked.
+
 2011-03-29  Kent Tamura  <tkent@chromium.org>
 
         Reviewed by Dimitri Glazkov.
index 5a77710..48abcf9 100644 (file)
@@ -498,7 +498,10 @@ class CheckerDispatcher(object):
         if file_type == FileType.NONE:
             checker = None
         elif file_type == FileType.CHANGELOG:
-            checker = ChangeLogChecker(file_path, handle_style_error)
+            should_line_be_checked = None
+            if handle_style_error:
+                should_line_be_checked = handle_style_error.should_line_be_checked
+            checker = ChangeLogChecker(file_path, handle_style_error, should_line_be_checked)
         elif file_type == FileType.CPP:
             file_extension = self._file_extension(file_path)
             checker = CppChecker(file_path, file_extension,
index 8935c2d..144d40a 100755 (executable)
@@ -369,12 +369,10 @@ class CheckerDispatcherDispatchTest(unittest.TestCase):
 
     """Tests dispatch() method of CheckerDispatcher class."""
 
-    def mock_handle_style_error(self):
-        pass
-
     def dispatch(self, file_path):
         """Call dispatch() with the given file path."""
         dispatcher = CheckerDispatcher()
+        self.mock_handle_style_error = DefaultStyleErrorHandler('', None, None, [])
         checker = dispatcher.dispatch(file_path,
                                       self.mock_handle_style_error,
                                       min_confidence=3)
index be279a7..75754fa 100644 (file)
@@ -34,12 +34,15 @@ class ChangeLogChecker(object):
 
     """Processes text lines for checking style."""
 
-    def __init__(self, file_path, handle_style_error):
+    def __init__(self, file_path, handle_style_error, should_line_be_checked):
         self.file_path = file_path
         self.handle_style_error = handle_style_error
+        self.should_line_be_checked = should_line_be_checked
         self._tab_checker = TabChecker(file_path, handle_style_error)
 
-    def check_entry(self, entry_line_number, entry_lines):
+    def check_entry(self, first_line_checked, entry_lines):
+        if not entry_lines:
+            return
         for line in entry_lines:
             if parse_bug_id_from_changelog(line):
                 break
@@ -48,26 +51,24 @@ class ChangeLogChecker(object):
             if re.search("build", line, re.IGNORECASE) and re.search("fix", line, re.IGNORECASE):
                 break
         else:
-            self.handle_style_error(entry_line_number + 1,
+            self.handle_style_error(first_line_checked,
                                     "changelog/bugnumber", 5,
                                     "ChangeLog entry has no bug number")
 
     def check(self, lines):
         self._tab_checker.check(lines)
-        entry_line_number = 0
+        first_line_checked = 0
         entry_lines = []
-        started_at_first_line = False
-
-        for line_number, line in enumerate(lines):
-            if re.match("^\d{4}-\d{2}-\d{2}", line):
-                if line_number:
-                    self.check_entry(entry_line_number, entry_lines)
-                else:
-                    started_at_first_line = True
-                entry_line_number = line_number
-                entry_lines = []
 
+        for line_index, line in enumerate(lines):
+            if not self.should_line_be_checked(line_index + 1):
+                # If we transitioned from finding changed lines to
+                # unchanged lines, then we are done.
+                if first_line_checked:
+                    break
+                continue
+            if not first_line_checked:
+                first_line_checked = line_index + 1
             entry_lines.append(line)
 
-        if started_at_first_line:
-            self.check_entry(entry_line_number, entry_lines)
+        self.check_entry(first_line_checked, entry_lines)
index 414e4fe..02296d3 100644 (file)
@@ -32,59 +32,124 @@ import unittest
 class ChangeLogCheckerTest(unittest.TestCase):
     """Tests ChangeLogChecker class."""
 
-    def assert_no_error(self, changelog_data):
+    def assert_no_error(self, lines_to_check, changelog_data):
         def handle_style_error(line_number, category, confidence, message):
-            self.fail('Unexpected error: %d %s %d %s' % (line_number, category, confidence, message))
-        checker = changelog.ChangeLogChecker('ChangeLog', handle_style_error)
+            self.fail('Unexpected error: %d %s %d %s for\n%s' % (line_number, category, confidence, message, changelog_data))
+        self.lines_to_check = set(lines_to_check)
+        checker = changelog.ChangeLogChecker('ChangeLog', handle_style_error, self.mock_should_line_be_checked)
         checker.check(changelog_data.split('\n'))
 
-    def assert_error(self, expected_line_number, expected_category, changelog_data):
+    def assert_error(self, expected_line_number, lines_to_check, expected_category, changelog_data):
         self.had_error = False
 
         def handle_style_error(line_number, category, confidence, message):
             self.had_error = True
             self.assertEquals(expected_line_number, line_number)
             self.assertEquals(expected_category, category)
-
-        checker = changelog.ChangeLogChecker('ChangeLog', handle_style_error)
+        self.lines_to_check = set(lines_to_check)
+        checker = changelog.ChangeLogChecker('ChangeLog', handle_style_error, self.mock_should_line_be_checked)
         checker.check(changelog_data.split('\n'))
         self.assertTrue(self.had_error)
 
     def mock_handle_style_error(self):
         pass
 
+    def mock_should_line_be_checked(self, line_number):
+        return line_number in self.lines_to_check
+
     def test_init(self):
-        checker = changelog.ChangeLogChecker('ChangeLog', self.mock_handle_style_error)
+        checker = changelog.ChangeLogChecker('ChangeLog', self.mock_handle_style_error, self.mock_should_line_be_checked)
         self.assertEquals(checker.file_path, 'ChangeLog')
         self.assertEquals(checker.handle_style_error, self.mock_handle_style_error)
+        self.assertEquals(checker.should_line_be_checked, self.mock_should_line_be_checked)
 
     def test_missing_bug_number(self):
-        entries = [
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Example bug',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Example bug\n        http://bugs.webkit.org/show_bug.cgi?id=\n',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Example bug\n        https://bugs.webkit.org/show_bug.cgi?id=\n',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Example bug\n        http://webkit.org/b/\n',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Example bug\n        http://trac.webkit.org/changeset/12345\n',
-            'Example bug\n        https://bugs.webkit.org/show_bug.cgi\n\n2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n',
-            'Example bug\n        More text about bug.\n\n2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n',
-        ]
-
-        for entry in entries:
-            self.assert_error(1, 'changelog/bugnumber', entry)
+        self.assert_error(1, range(1, 20), 'changelog/bugnumber',
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '\n'
+                          '        Example bug\n')
+        self.assert_error(1, range(1, 20), 'changelog/bugnumber',
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '\n'
+                          '        Example bug\n'
+                          '        http://bugs.webkit.org/show_bug.cgi?id=\n')
+        self.assert_error(1, range(1, 20), 'changelog/bugnumber',
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '\n'
+                          '        Example bug\n'
+                          '        https://bugs.webkit.org/show_bug.cgi?id=\n')
+        self.assert_error(1, range(1, 20), 'changelog/bugnumber',
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '\n'
+                          '        Example bug\n'
+                          '        http://webkit.org/b/\n')
+        self.assert_error(1, range(1, 20), 'changelog/bugnumber',
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '\n'
+                          '        Example bug'
+                          '\n'
+                          '        http://trac.webkit.org/changeset/12345\n')
+        self.assert_error(2, range(2, 5), 'changelog/bugnumber',
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '        Example bug\n'
+                          '        https://bugs.webkit.org/show_bug.cgi\n'
+                          '\n'
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '        Another change\n')
+        self.assert_error(2, range(2, 6), 'changelog/bugnumber',
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '        Example bug\n'
+                          '        More text about bug.\n'
+                          '\n'
+                          '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                          '\n'
+                          '        No bug in this change.\n')
 
     def test_no_error(self):
-        entries = [
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Example bug\n        http://bugs.webkit.org/show_bug.cgi?id=12345\n',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Example bug\n        https://bugs.webkit.org/show_bug.cgi?id=12345\n',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Example bug\n        http://webkit.org/b/12345\n',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Unreview build fix for r12345.\n',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Fix build after a bad change.\n',
-            '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n\n        Fix example port build.\n',
-            'Example bug\n        https://bugs.webkit.org/show_bug.cgi?id=12345\n\n2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n',
-        ]
-
-        for entry in entries:
-            self.assert_no_error(entry)
+        self.assert_no_error([],
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '\n'
+                             '        Example ChangeLog entry out of range\n'
+                             '        http://example.com/\n')
+        self.assert_no_error([],
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '\n'
+                             '        Example bug\n'
+                             '        http://bugs.webkit.org/show_bug.cgi?id=12345\n')
+        self.assert_no_error(range(1, 20),
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '\n'
+                             '        Example bug\n'
+                             '        http://bugs.webkit.org/show_bug.cgi?id=12345\n')
+        self.assert_no_error(range(1, 20),
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '\n'
+                             '        Example bug\n'
+                             '        https://bugs.webkit.org/show_bug.cgi?id=12345\n')
+        self.assert_no_error(range(1, 20),
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '\n'
+                             '        Example bug\n'
+                             '        http://webkit.org/b/12345\n')
+        self.assert_no_error(range(1, 20),
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '\n'
+                             '        Unreview build fix for r12345.\n')
+        self.assert_no_error(range(1, 20),
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '\n'
+                             '        Fix build after a bad change.\n')
+        self.assert_no_error(range(1, 20),
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '\n'
+                             '        Fix example port build.\n')
+        self.assert_no_error(range(2, 6),
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '        Example bug\n'
+                             '        https://bugs.webkit.org/show_bug.cgi?id=12345\n'
+                             '\n'
+                             '2011-01-01  Patrick Gansterer  <paroga@paroga.com>\n'
+                             '        No bug here!\n')
 
 if __name__ == '__main__':
     unittest.main()
index 0bede24..5d8b041 100644 (file)
@@ -123,16 +123,18 @@ class DefaultStyleErrorHandler(object):
             return None
         return self._configuration.max_reports_per_category[category]
 
+    def should_line_be_checked(self, line_number):
+        "Returns if a particular line should be checked"
+        # Was the line that was modified?
+        return self._line_numbers is None or line_number in self._line_numbers
+
     def __call__(self, line_number, category, confidence, message):
         """Handle the occurrence of a style error.
 
         See the docstring of this module for more information.
 
         """
-        if (self._line_numbers is not None and
-            line_number not in self._line_numbers):
-            # Then the error occurred in a line that was not modified, so
-            # the error is not reportable.
+        if not self.should_line_be_checked(line_number):
             return
 
         if not self._configuration.is_reportable(category=category,