Teach check-webkit-style to check .vcproj and .vsprops files for XML syntax errors
authoraroben@apple.com <aroben@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Dec 2010 23:32:25 +0000 (23:32 +0000)
committeraroben@apple.com <aroben@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 Dec 2010 23:32:25 +0000 (23:32 +0000)
Fixes <http://webkit.org/b/51103> check-webkit-style should check for
XML syntax errors in .vcproj/.vsprops files

Reviewed by Dave Levin.

* Scripts/webkitpy/style/checker.py: Added lists of file extensions
that should be treated as XML and that should be allowed to contain
carriage returns. (These lists happen to be identical currently.)
(FileType): Added a new XML type.
(CheckerDispatcher.should_check_and_strip_carriage_returns): Added.
Just does a simple file extension check.
(CheckerDispatcher._file_type): Added a case for XML files.
(CheckerDispatcher._create_checker): Ditto. We use XMLChecker for XML
files (surprise!).
(StyleProcessor.process): Ask the dispatcher whether we should pass the
lines through the carriage checker.

* Scripts/webkitpy/style/checker_unittest.py:
(CheckerDispatcherCarriageReturnTest.test_should_check_and_strip_carriage_returns):
Added. Checks a few file names to see if carriage returns are allowed
or not.
(CheckerDispatcherDispatchTest.assert_checker_xml): Added. Similar to
other assert_checker_* functions.
(CheckerDispatcherDispatchTest.test_xml_paths): Added. Similar to other
test_*_paths functions.
(CheckerDispatcherDispatchTest.test_xml_paths): Added. Similar to other
test_*_paths functions.
(CheckerDispatcherDispatchTest.test_none_paths): Removed the vcproj
file from this test case, as vcproj files now have a type.
(StyleProcessor_CodeCoverageTest.MockDispatcher.should_check_and_strip_carriage_returns):
Added. Similar to the other should_* functions.
(StyleProcessor_CodeCoverageTest.test_process__carriage_returns_not_stripped):
Added. Checks that carriage returns aren't checked for or stripped for
allowed files.

* Scripts/webkitpy/style/checkers/xml.py: Added.
(XMLChecker.__init__): Simple init method.
(XMLChecker.check): Pass each line through the expat parser, and record
a style error for any errors thrown by the parser.

* Scripts/webkitpy/style/checkers/xml_unittest.py: Added.
(XMLCheckerTest.assert_no_error): Checks that the given XML does not
produce a style error.
(XMLCheckerTest.assert_error): Checks that the given XML produces an
error of the given category on the given line.
(XMLCheckerTest.mock_handle_style_error): Does nothing. Used for
checking that the XMLChecker constructor works properly.
(XMLCheckerTest.test_conflict_marker): Tests that conflict markers
cause a style error (see, e.g., r73887).
(XMLCheckerTest.test_extra_closing_tag): Tests that extra closing tags
cause a style error (see, e.g., r73773).
(XMLCheckerTest.test_init): Tests that the XMLChecker constructor works
properly.
(XMLCheckerTest.test_missing_closing_tag): Tests that missing closing
tags cause a style error (see, e.g., r72795).
(XMLCheckerTest.test_no_error): Tests that valid XML does not cause a
style error.

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

WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/style/checker.py
WebKitTools/Scripts/webkitpy/style/checker_unittest.py
WebKitTools/Scripts/webkitpy/style/checkers/xml.py [new file with mode: 0644]
WebKitTools/Scripts/webkitpy/style/checkers/xml_unittest.py [new file with mode: 0644]

index ffdb275..8f67ea1 100644 (file)
@@ -1,3 +1,66 @@
+2010-12-15  Adam Roben  <aroben@apple.com>
+
+        Teach check-webkit-style to check .vcproj and .vsprops files for XML
+        syntax errors
+
+        Fixes <http://webkit.org/b/51103> check-webkit-style should check for
+        XML syntax errors in .vcproj/.vsprops files
+
+        Reviewed by Dave Levin.
+
+        * Scripts/webkitpy/style/checker.py: Added lists of file extensions
+        that should be treated as XML and that should be allowed to contain
+        carriage returns. (These lists happen to be identical currently.)
+        (FileType): Added a new XML type.
+        (CheckerDispatcher.should_check_and_strip_carriage_returns): Added.
+        Just does a simple file extension check.
+        (CheckerDispatcher._file_type): Added a case for XML files.
+        (CheckerDispatcher._create_checker): Ditto. We use XMLChecker for XML
+        files (surprise!).
+        (StyleProcessor.process): Ask the dispatcher whether we should pass the
+        lines through the carriage checker.
+
+        * Scripts/webkitpy/style/checker_unittest.py:
+        (CheckerDispatcherCarriageReturnTest.test_should_check_and_strip_carriage_returns):
+        Added. Checks a few file names to see if carriage returns are allowed
+        or not.
+        (CheckerDispatcherDispatchTest.assert_checker_xml): Added. Similar to
+        other assert_checker_* functions.
+        (CheckerDispatcherDispatchTest.test_xml_paths): Added. Similar to other
+        test_*_paths functions.
+        (CheckerDispatcherDispatchTest.test_xml_paths): Added. Similar to other
+        test_*_paths functions.
+        (CheckerDispatcherDispatchTest.test_none_paths): Removed the vcproj
+        file from this test case, as vcproj files now have a type.
+        (StyleProcessor_CodeCoverageTest.MockDispatcher.should_check_and_strip_carriage_returns):
+        Added. Similar to the other should_* functions.
+        (StyleProcessor_CodeCoverageTest.test_process__carriage_returns_not_stripped):
+        Added. Checks that carriage returns aren't checked for or stripped for
+        allowed files.
+
+        * Scripts/webkitpy/style/checkers/xml.py: Added.
+        (XMLChecker.__init__): Simple init method.
+        (XMLChecker.check): Pass each line through the expat parser, and record
+        a style error for any errors thrown by the parser.
+
+        * Scripts/webkitpy/style/checkers/xml_unittest.py: Added.
+        (XMLCheckerTest.assert_no_error): Checks that the given XML does not
+        produce a style error.
+        (XMLCheckerTest.assert_error): Checks that the given XML produces an
+        error of the given category on the given line.
+        (XMLCheckerTest.mock_handle_style_error): Does nothing. Used for
+        checking that the XMLChecker constructor works properly.
+        (XMLCheckerTest.test_conflict_marker): Tests that conflict markers
+        cause a style error (see, e.g., r73887).
+        (XMLCheckerTest.test_extra_closing_tag): Tests that extra closing tags
+        cause a style error (see, e.g., r73773).
+        (XMLCheckerTest.test_init): Tests that the XMLChecker constructor works
+        properly.
+        (XMLCheckerTest.test_missing_closing_tag): Tests that missing closing
+        tags cause a style error (see, e.g., r72795).
+        (XMLCheckerTest.test_no_error): Tests that valid XML does not cause a
+        style error.
+
 2010-12-15  Lucas Forschler  <lforschler@apple.com>
 
         Reviewed by Stephanie Lewis.
index e10eec5..e9ea8ee 100644 (file)
@@ -40,6 +40,7 @@ from checkers.cpp import CppChecker
 from checkers.python import PythonChecker
 from checkers.test_expectations import TestExpectationsChecker
 from checkers.text import TextChecker
+from checkers.xml import XMLChecker
 from error_handlers import DefaultStyleErrorHandler
 from filter import FilterConfiguration
 from optparser import ArgumentParser
@@ -196,9 +197,6 @@ _CPP_FILE_EXTENSIONS = [
 
 _PYTHON_FILE_EXTENSION = 'py'
 
-# FIXME: Include 'vcproj' files as text files after creating a mechanism
-#        for exempting them from the carriage-return checker (since they
-#        are Windows-only files).
 _TEXT_FILE_EXTENSIONS = [
     'ac',
     'cc',
@@ -221,12 +219,15 @@ _TEXT_FILE_EXTENSIONS = [
     'rb',
     'sh',
     'txt',
-#   'vcproj',  # See FIXME above.
     'wm',
     'xhtml',
     'y',
     ]
 
+_XML_FILE_EXTENSIONS = [
+    'vcproj',
+    'vsprops',
+    ]
 
 # Files to skip that are less obvious.
 #
@@ -252,6 +253,11 @@ _SKIPPED_FILES_WITHOUT_WARNING = [
     "LayoutTests/",
     ]
 
+# Extensions of files which are allowed to contain carriage returns.
+_CARRIAGE_RETURN_ALLOWED_FILE_EXTENSIONS = [
+    'vcproj',
+    'vsprops',
+    ]
 
 # The maximum number of errors to report per file, per category.
 # If a category is not a key, then it has no maximum.
@@ -406,6 +412,7 @@ class FileType:
     CPP = 1
     PYTHON = 2
     TEXT = 3
+    XML = 4
 
 
 class CheckerDispatcher(object):
@@ -445,6 +452,9 @@ class CheckerDispatcher(object):
                 return True
         return False
 
+    def should_check_and_strip_carriage_returns(self, file_path):
+        return self._file_extension(file_path) not in _CARRIAGE_RETURN_ALLOWED_FILE_EXTENSIONS
+
     def _file_type(self, file_path):
         """Return the file type corresponding to the given file."""
         file_extension = self._file_extension(file_path)
@@ -459,6 +469,8 @@ class CheckerDispatcher(object):
             return FileType.CPP
         elif file_extension == _PYTHON_FILE_EXTENSION:
             return FileType.PYTHON
+        elif file_extension in _XML_FILE_EXTENSIONS:
+            return FileType.XML
         elif (os.path.basename(file_path).startswith('ChangeLog') or
               (not file_extension and "WebKitTools/Scripts/" in file_path) or
               file_extension in _TEXT_FILE_EXTENSIONS):
@@ -477,6 +489,8 @@ class CheckerDispatcher(object):
                                  handle_style_error, min_confidence)
         elif file_type == FileType.PYTHON:
             checker = PythonChecker(file_path, handle_style_error)
+        elif file_type == FileType.XML:
+            checker = XMLChecker(file_path, handle_style_error)
         elif file_type == FileType.TEXT:
             basename = os.path.basename(file_path)
             if basename == 'test_expectations.txt' or basename == 'drt_expectations.txt':
@@ -718,13 +732,9 @@ class StyleProcessor(ProcessorBase):
 
         carriage_checker = self._carriage_checker_class(style_error_handler)
 
-        # FIXME: We should probably use the SVN "eol-style" property
-        #        or a white list to decide whether or not to do
-        #        the carriage-return check. Originally, we did the
-        #        check only if (os.linesep != '\r\n').
-        #
         # Check for and remove trailing carriage returns ("\r").
-        lines = carriage_checker.check(lines)
+        if self._dispatcher.should_check_and_strip_carriage_returns(file_path):
+            lines = carriage_checker.check(lines)
 
         min_confidence = self._configuration.min_confidence
         checker = self._dispatcher.dispatch(file_path,
index 94d2c29..f310143 100755 (executable)
@@ -55,6 +55,7 @@ from checker import StyleProcessorConfiguration
 from checkers.cpp import CppChecker
 from checkers.python import PythonChecker
 from checkers.text import TextChecker
+from checkers.xml import XMLChecker
 from error_handlers import DefaultStyleErrorHandler
 from filter import validate_filter_rules
 from filter import FilterConfiguration
@@ -343,6 +344,20 @@ class CheckerDispatcherSkipTest(unittest.TestCase):
                                                      expected=False)
 
 
+class CheckerDispatcherCarriageReturnTest(unittest.TestCase):
+    def test_should_check_and_strip_carriage_returns(self):
+        files = {
+            'foo.txt': True,
+            'foo.cpp': True,
+            'foo.vcproj': False,
+            'foo.vsprops': False,
+        }
+
+        dispatcher = CheckerDispatcher()
+        for file_path, expected_result in files.items():
+            self.assertEquals(dispatcher.should_check_and_strip_carriage_returns(file_path), expected_result, 'Checking: %s' % file_path)
+
+
 class CheckerDispatcherDispatchTest(unittest.TestCase):
 
     """Tests dispatch() method of CheckerDispatcher class."""
@@ -386,6 +401,10 @@ class CheckerDispatcherDispatchTest(unittest.TestCase):
         """Assert that the dispatched checker is a TextChecker."""
         self.assert_checker(file_path, TextChecker)
 
+    def assert_checker_xml(self, file_path):
+        """Assert that the dispatched checker is a XMLChecker."""
+        self.assert_checker(file_path, XMLChecker)
+
     def test_cpp_paths(self):
         """Test paths that should be checked as C++."""
         paths = [
@@ -483,6 +502,26 @@ class CheckerDispatcherDispatchTest(unittest.TestCase):
         self.assertEquals(checker.file_path, file_path)
         self.assertEquals(checker.handle_style_error, self.mock_handle_style_error)
 
+    def test_xml_paths(self):
+        """Test paths that should be checked as XML."""
+        paths = [
+           "WebCore/WebCore.vcproj/WebCore.vcproj",
+           "WebKitLibraries/win/tools/vsprops/common.vsprops",
+        ]
+
+        for path in paths:
+            self.assert_checker_xml(path)
+
+        # Check checker attributes on a typical input.
+        file_base = "foo"
+        file_extension = "vcproj"
+        file_path = file_base + "." + file_extension
+        self.assert_checker_xml(file_path)
+        checker = self.dispatch(file_path)
+        self.assertEquals(checker.file_path, file_path)
+        self.assertEquals(checker.handle_style_error,
+                          self.mock_handle_style_error)
+
     def test_none_paths(self):
         """Test paths that have no file type.."""
         paths = [
@@ -490,7 +529,6 @@ class CheckerDispatcherDispatchTest(unittest.TestCase):
            "foo.asdf",  # Non-sensical file extension.
            "foo.png",
            "foo.exe",
-           "foo.vcproj",
             ]
 
         for path in paths:
@@ -638,6 +676,9 @@ class StyleProcessor_CodeCoverageTest(LoggingTestCase):
         def should_skip_without_warning(self, file_path):
             return file_path.endswith('skip_without_warning.txt')
 
+        def should_check_and_strip_carriage_returns(self, file_path):
+            return not file_path.endswith('carriage_returns_allowed.txt')
+
         def dispatch(self, file_path, style_error_handler, min_confidence):
             if file_path.endswith('do_not_process.txt'):
                 return None
@@ -777,3 +818,15 @@ class StyleProcessor_CodeCoverageTest(LoggingTestCase):
         self.assertRaises(AssertionError, self._processor.process,
                           lines=['line1', 'line2'], file_path=path,
                           line_numbers=[100])
+
+    def test_process__carriage_returns_not_stripped(self):
+        """Test that carriage returns aren't stripped from files that are allowed to contain them."""
+        file_path = 'carriage_returns_allowed.txt'
+        lines = ['line1\r', 'line2\r']
+        line_numbers = [100]
+        self._processor.process(lines=lines,
+                                file_path=file_path,
+                                line_numbers=line_numbers)
+        # The carriage return checker should never have been invoked, and so
+        # should not have saved off any lines.
+        self.assertFalse(hasattr(self.carriage_checker, 'lines'))
diff --git a/WebKitTools/Scripts/webkitpy/style/checkers/xml.py b/WebKitTools/Scripts/webkitpy/style/checkers/xml.py
new file mode 100644 (file)
index 0000000..bd8c68a
--- /dev/null
@@ -0,0 +1,45 @@
+# Copyright (C) 2010 Apple Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions
+# are met:
+# 1.  Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer.
+# 2.  Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in the
+#     documentation and/or other materials provided with the distribution.
+#
+# THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS BE LIABLE FOR
+# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+"""Checks WebKit style for XML files."""
+
+from __future__ import absolute_import
+
+from xml.parsers import expat
+
+
+class XMLChecker(object):
+    """Processes XML lines for checking style."""
+
+    def __init__(self, file_path, handle_style_error):
+        self.file_path = file_path
+        self.handle_style_error = handle_style_error
+
+    def check(self, lines):
+        parser = expat.ParserCreate()
+        try:
+            for line in lines:
+                parser.Parse(line)
+                parser.Parse('\n')
+            parser.Parse('', True)
+        except expat.ExpatError, error:
+            self.handle_style_error(error.lineno, 'xml/syntax', 5, error.message)
diff --git a/WebKitTools/Scripts/webkitpy/style/checkers/xml_unittest.py b/WebKitTools/Scripts/webkitpy/style/checkers/xml_unittest.py
new file mode 100644 (file)
index 0000000..3825660
--- /dev/null
@@ -0,0 +1,72 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2010 Apple Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions
+# are met:
+# 1.  Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer.
+# 2.  Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in the
+#     documentation and/or other materials provided with the distribution.
+#
+# THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS BE LIABLE FOR
+# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+"""Unit test for xml.py."""
+
+import unittest
+
+import xml
+
+
+class XMLCheckerTest(unittest.TestCase):
+    """Tests XMLChecker class."""
+
+    def assert_no_error(self, xml_data):
+        def handle_style_error(line_number, category, confidence, message):
+            self.fail('Unexpected error: %d %s %d %s' % (line_number, category, confidence, message))
+        checker = xml.XMLChecker('foo.xml', handle_style_error)
+        checker.check(xml_data.split('\n'))
+
+    def assert_error(self, expected_line_number, expected_category, xml_data):
+        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 = xml.XMLChecker('foo.xml', handle_style_error)
+        checker.check(xml_data.split('\n'))
+        self.assertTrue(self.had_error)
+
+    def mock_handle_style_error(self):
+        pass
+
+    def test_conflict_marker(self):
+        self.assert_error(1, 'xml/syntax', '<<<<<<< HEAD\n<foo>\n</foo>\n')
+
+    def test_extra_closing_tag(self):
+        self.assert_error(3, 'xml/syntax', '<foo>\n</foo>\n</foo>\n')
+
+    def test_init(self):
+        checker = xml.XMLChecker('foo.xml', self.mock_handle_style_error)
+        self.assertEquals(checker.file_path, 'foo.xml')
+        self.assertEquals(checker.handle_style_error, self.mock_handle_style_error)
+
+    def test_missing_closing_tag(self):
+        self.assert_error(3, 'xml/syntax', '<foo>\n<bar>\n</foo>\n')
+
+    def test_no_error(self):
+        checker = xml.XMLChecker('foo.xml', self.assert_no_error)
+        checker.check(['<foo>', '</foo>'])
+
+if __name__ == '__main__':
+    unittest.main()