Attachment 49805 Details for Bug 35484 – Proposed patch 6

[patch] Proposed patch 6

_patch-35484-6.diff (text/plain), 20.01 KB, created by Chris Jerdonek on 2010-03-02 07:52:05 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Jerdonek

Created: 2010-03-02 07:52:05 PST

Size: 20.01 KB

patch

obsolete

>diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
>index e20cb40..2b11005 100644
>--- a/WebKitTools/ChangeLog
>+++ b/WebKitTools/ChangeLog
>@@ -1,3 +1,46 @@
>+2010-03-02  Chris Jerdonek  <cjerdonek@webkit.org>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Started using the logging module in check-webkit-style.
>+        This provides more options for debugging and a more flexible,
>+        uniform way to report messages to the end-user.
>+
>+        https://bugs.webkit.org/show_bug.cgi?id=35484
>+
>+        Also included classes in a central location to facilitate
>+        the unit testing of logging code (setUp and tearDown of unit
>+        test logging configurations, etc).
>+
>+        * Scripts/check-webkit-style:
>+          - Added a call to configure_logging() in the beginning of main().
>+          - Replaced two calls to sys.stderr.write() with appropriate
>+            logging calls.
>+
>+        * Scripts/webkitpy/init/__init__.py: Copied from WebKitTools/QueueStatusServer/filters/__init__.py.
>+
>+        * Scripts/webkitpy/init/logtesting.py: Added.
>+          - Added a UnitTestLogStream class to capture log output
>+            during unit tests.
>+          - Added a UnitTestLog class that provides convenience methods
>+            for unit-testing logging code.
>+
>+        * Scripts/webkitpy/style/checker.py:
>+          - Added a configure_logging() method.
>+          - Added a _LevelLoggingFilter class to filter out log messages
>+            above a certain logging level.
>+          - Removed the _stderr_write() method from the StyleChecker class
>+            and replaced its use with appropriate logging calls.
>+
>+        * Scripts/webkitpy/style/checker_unittest.py:
>+          - Added a ConfigureLoggingTest class to unit test the
>+            configure_logging() method.
>+          - Updated the StyleCheckerCheckFileTest class as necessary.
>+
>+        * Scripts/webkitpy/style_references.py:
>+          - Added references to logtesting.UnitTestLog and
>+            logtesting.UnitTestLogStream.
>+
> 2010-03-01  Chris Fleizach  <cfleizach@apple.com>
> 
>         Fixing broken DRT on Leopard/Tiger. Second try.
>diff --git a/WebKitTools/Scripts/check-webkit-style b/WebKitTools/Scripts/check-webkit-style
>index ea2e943..f1885b2 100755
>--- a/WebKitTools/Scripts/check-webkit-style
>+++ b/WebKitTools/Scripts/check-webkit-style
>@@ -43,6 +43,7 @@ same line, but it is far from perfect (in either direction).
> """
> 
> import codecs
>+import logging
> import os
> import os.path
> import sys
>@@ -50,13 +51,28 @@ import sys
> import webkitpy.style.checker as checker
> from webkitpy.style_references import SimpleScm
> 
>+_log = logging.getLogger("check-webkit-style")
>+
> def main():
>     # Change stderr to write with replacement characters so we don't die
>     # if we try to print something containing non-ASCII characters.
>-    sys.stderr = codecs.StreamReaderWriter(sys.stderr,
>-                                           codecs.getreader('utf8'),
>-                                           codecs.getwriter('utf8'),
>-                                           'replace')
>+    stderr = codecs.StreamReaderWriter(sys.stderr,
>+                                       codecs.getreader('utf8'),
>+                                       codecs.getwriter('utf8'),
>+                                       'replace')
>+    # Setting an "encoding" attribute on the stream is necessary to
>+    # prevent the logging module from raising an error.  See
>+    # the checker.configure_logging() function for more information.
>+    stderr.encoding = "UTF-8"
>+
>+    # FIXME: Change webkitpy.style so that we do not need to overwrite
>+    #        the global sys.stderr.  This involves updating the code to
>+    #        accept a stream parameter where necessary, and not calling
>+    #        sys.stderr explicitly anywhere.
>+    sys.stderr = stderr
>+
>+    checker.configure_logging(stderr)
>+
>     parser = checker.check_webkit_style_parser()
>     (files, options) = parser.parse(sys.argv[1:])
> 
>@@ -78,7 +94,8 @@ def main():
>                 # FIXME: If the range is a "...", the code should find the common ancestor and
>                 # start there (see git diff --help for information about how ... usually works).
>                 commit = commit[:commit.find('..')]
>-                print >> sys.stderr, "Warning: Ranges are not supported for --git-commit. Checking all changes since %s.\n" % commit
>+                _log.warn("Ranges are not supported for --git-commit. "
>+                          "Checking all changes since %s." % commit)
>             patch = scm.create_patch_since_local_commit(commit)
>         else:
>             patch = scm.create_patch()
>@@ -86,8 +103,8 @@ def main():
> 
>     error_count = style_checker.error_count
>     file_count = style_checker.file_count
>-    sys.stderr.write('Total errors found: %d in %d files\n'
>-                     % (error_count, file_count))
>+    _log.info("Total errors found: %d in %d files"
>+              % (error_count, file_count))
>     # We fail when style errors are found or there are no checked files.
>     sys.exit(error_count > 0 or file_count == 0)
> 
>diff --git a/WebKitTools/Scripts/webkitpy/init/__init__.py b/WebKitTools/Scripts/webkitpy/init/__init__.py
>new file mode 100644
>index 0000000..ef65bee
>--- /dev/null
>+++ b/WebKitTools/Scripts/webkitpy/init/__init__.py
>@@ -0,0 +1 @@
>+# Required for Python to search this directory for module files
>diff --git a/WebKitTools/Scripts/webkitpy/init/logtesting.py b/WebKitTools/Scripts/webkitpy/init/logtesting.py
>new file mode 100644
>index 0000000..6363115
>--- /dev/null
>+++ b/WebKitTools/Scripts/webkitpy/init/logtesting.py
>@@ -0,0 +1,122 @@
>+# Copyright (C) 2010 Chris Jerdonek (cjerdonek@webkit.org)
>+#
>+# 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.
>+
>+"""Supports the unit-testing of logging."""
>+
>+import logging
>+
>+
>+class UnitTestLogStream(object):
>+
>+    """Represents a file-like object for unit-testing logging.
>+
>+    This is meant for passing to the logging.StreamHandler constructor.
>+
>+    """
>+
>+    def __init__(self):
>+        self.messages = []
>+        """A list of log messages written to the stream."""
>+
>+    def write(self, message):
>+        self.messages.append(message)
>+
>+    def flush(self):
>+        pass
>+
>+
>+class UnitTestLog(object):
>+
>+    """Supports unit-testing logging.
>+
>+    Sample usage:
>+
>+    # The following line should go in the setUp() method of a
>+    # unittest.TestCase.  In particular, self is a TestCase instance.
>+    self._log = UnitTestLog(self)
>+
>+    # Call the following in a test method of a unittest.TestCase.
>+    log = logging.getLogger("webkitpy")
>+    log.info("message1")
>+    log.warn("message2")
>+    self._log.assertMessages(["INFO: message1\n",
>+                              "WARN: message2\n"])  # Should succeed.
>+
>+    # The following line should go in the tearDown() method of
>+    # unittest.TestCase.
>+    self._log.tearDown()
>+
>+    """
>+
>+    def __init__(self, test_case, logging_level=None):
>+        """Configure unit test logging, and return an instance.
>+
>+        This method configures the root logger to log to a testing
>+        log stream.  Only messages logged at or above the given level
>+        are logged to the stream.  Messages are formatted in the
>+        following way, for example--
>+
>+        "INFO: This is a test log message."
>+
>+        This method should normally be called in the setUp() method
>+        of a unittest.TestCase.
>+
>+        Args:
>+          test_case: A unittest.TestCase instance.
>+          logging_level: A logging level.  Only messages logged at or
>+                         above this level are written to the testing
>+                         log stream.  Defaults to logging.INFO.
>+
>+        """
>+        if logging_level is None:
>+            logging_level = logging.INFO
>+
>+        stream = UnitTestLogStream()
>+        handler = logging.StreamHandler(stream)
>+        formatter = logging.Formatter("%(levelname)s: %(message)s")
>+        handler.setFormatter(formatter)
>+
>+        logger = logging.getLogger()
>+        logger.setLevel(logging_level)
>+        logger.addHandler(handler)
>+
>+        self._handler = handler
>+        self._messages = stream.messages
>+        self._test_case = test_case
>+
>+    def _logger(self):
>+        """Return the root logger used for logging."""
>+        return logging.getLogger()
>+
>+    def assertMessages(self, messages):
>+        """Assert that the given messages match the logged messages."""
>+        self._test_case.assertEquals(messages, self._messages)
>+
>+    def tearDown(self):
>+        """Unconfigure unit test logging.
>+
>+        This should normally be called in the tearDown method of a
>+        unittest.TestCase
>+
>+        """
>+        logger = self._logger()
>+        logger.removeHandler(self._handler)
>diff --git a/WebKitTools/Scripts/webkitpy/style/checker.py b/WebKitTools/Scripts/webkitpy/style/checker.py
>index 8494e1b..c74aece 100644
>--- a/WebKitTools/Scripts/webkitpy/style/checker.py
>+++ b/WebKitTools/Scripts/webkitpy/style/checker.py
>@@ -30,6 +30,7 @@
> """Front end of some style-checker modules."""
> 
> import codecs
>+import logging
> import os.path
> import sys
> 
>@@ -44,6 +45,7 @@ from processors.common import categories as CommonCategories
> from processors.cpp import CppProcessor
> from processors.text import TextProcessor
> 
>+_log = logging.getLogger("webkitpy.style.checker")
> 
> # These are default option values for the command-line option parser.
> _DEFAULT_VERBOSITY = 1
>@@ -224,6 +226,88 @@ def check_webkit_style_configuration(options):
>                verbosity=options.verbosity)
> 
> 
>+# FIXME: Add support for more verbose logging for debug purposes.
>+#        This can use a formatter like the following, for example--
>+#
>+#        formatter = logging.Formatter("%(name)s: [%(levelname)s] %(message)s")
>+def configure_logging(stream):
>+    """Configure logging, and return the list of handlers added.
>+
>+    Configures the root logger to log INFO messages and higher.
>+    Formats WARNING messages and above to display the logging level
>+    and messages strictly below WARNING not to display it.
>+
>+    Returns:
>+      A list of references to the logging handlers added to the root
>+      logger.  This allows the caller to later remove the handlers
>+      using logger.removeHandler.  This is useful primarily during unit
>+      testing where the caller may want to configure logging temporarily
>+      and then undo the configuring.
>+
>+    Args:
>+      stream: A file-like object to which to log.  The stream must
>+              define an "encoding" data attribute, or else logging
>+              raises an error.
>+
>+    """
>+    # If the stream does not define an "encoding" data attribute, the
>+    # logging module can throw an error like the following:
>+    #
>+    # Traceback (most recent call last):
>+    #   File "/System/Library/Frameworks/Python.framework/Versions/2.6/...
>+    #         lib/python2.6/logging/__init__.py", line 761, in emit
>+    #     self.stream.write(fs % msg.encode(self.stream.encoding))
>+    # LookupError: unknown encoding: unknown
>+
>+    # Handles logging.WARNING and above.
>+    error_handler = logging.StreamHandler(stream)
>+    error_handler.setLevel(logging.WARNING)
>+    formatter = logging.Formatter("%(levelname)s: %(message)s")
>+    error_handler.setFormatter(formatter)
>+
>+    # Handles records strictly below logging.WARNING.
>+    non_error_handler = logging.StreamHandler(stream)
>+    non_error_filter = _LevelLoggingFilter(logging.WARNING)
>+    non_error_handler.addFilter(non_error_filter)
>+    formatter = logging.Formatter("%(message)s")
>+    non_error_handler.setFormatter(formatter)
>+
>+    logger = logging.getLogger()
>+    logger.setLevel(logging.INFO)
>+
>+    handlers = [error_handler, non_error_handler]
>+
>+    for handler in handlers:
>+        logger.addHandler(handler)
>+
>+    return handlers
>+
>+
>+# FIXME: Consider moving this class into a module in webkitpy.init after
>+#        getting more experience with its use.  We want to make sure
>+#        we have the right API before doing so.  For example, we may
>+#        want to provide a constructor that has both upper and lower
>+#        bounds, and not just an upper bound.
>+class _LevelLoggingFilter(object):
>+
>+    """A logging filter for blocking records at or above a certain level."""
>+
>+    def __init__(self, logging_level):
>+        """Create a _LevelLoggingFilter.
>+
>+        Args:
>+          logging_level: The logging level cut-off.  Logging levels at
>+                         or above this level will not be logged.
>+
>+        """
>+        self._logging_level = logging_level
>+
>+    # The logging module requires that this method be defined.
>+    def filter(self, log_record):
>+        """Return whether given the LogRecord should be logged."""
>+        return log_record.levelno < self._logging_level
>+
>+
> # Enum-like idiom
> class FileType:
> 
>@@ -322,6 +406,8 @@ class ProcessorDispatcher(object):
>         return processor
> 
> 
>+# FIXME: Remove the stderr_write attribute from this class and replace
>+#        its use with calls to a logging module logger.
> class StyleCheckerConfiguration(object):
> 
>     """Stores configuration values for the StyleChecker class.
>@@ -439,9 +525,6 @@ class StyleChecker(object):
>         self.error_count = 0
>         self.file_count = 0
> 
>-    def _stderr_write(self, message):
>-        self._configuration.stderr_write(message)
>-
>     def _increment_error_count(self):
>         """Increment the total count of reported errors."""
>         self.error_count += 1
>@@ -469,11 +552,15 @@ class StyleChecker(object):
>             contents = file.read()
> 
>         except IOError:
>-            self._stderr_write("Skipping input '%s': Can't open for reading\n" % file_path)
>+            message = 'Could not read file. Skipping: "%s"' % file_path
>+            _log.warn(message)
>             return
> 
>         lines = contents.split("\n")
> 
>+        # FIXME: Make a CarriageReturnProcessor for this logic, and put
>+        #        it in processors.common.  The process() method should
>+        #        return the lines with the carriage returns stripped.
>         for line_number in range(len(lines)):
>             # FIXME: We should probably use the SVN "eol-style" property
>             #        or a white list to decide whether or not to do
>@@ -520,8 +607,8 @@ class StyleChecker(object):
>         if dispatcher.should_skip_without_warning(file_path):
>             return
>         if dispatcher.should_skip_with_warning(file_path):
>-            self._stderr_write('Ignoring "%s": this file is exempt from the '
>-                               "style guide.\n" % file_path)
>+            _log.warn('File exempt from style guide. Skipping: "%s"'
>+                      % file_path)
>             return
> 
>         verbosity = self._configuration.verbosity
>diff --git a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
>index ae8cccc..d866663 100755
>--- a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
>+++ b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py
>@@ -34,15 +34,19 @@
> 
> """Unit tests for style.py."""
> 
>+import logging
> import unittest
> 
> import checker as style
>+from ..style_references import UnitTestLog
>+from ..style_references import UnitTestLogStream
> from checker import _BASE_FILTER_RULES
> from checker import _MAX_REPORTS_PER_CATEGORY
> from checker import _PATH_RULES_SPECIFIER as PATH_RULES_SPECIFIER
> from checker import _all_categories
> from checker import check_webkit_style_configuration
> from checker import check_webkit_style_parser
>+from checker import configure_logging
> from checker import ProcessorDispatcher
> from checker import StyleChecker
> from checker import StyleCheckerConfiguration
>@@ -54,6 +58,58 @@ from processors.cpp import CppProcessor
> from processors.text import TextProcessor
> 
> 
>+class ConfigureLoggingTest(unittest.TestCase):
>+
>+    """Tests the configure_logging() function."""
>+
>+    def setUp(self):
>+        log_stream = UnitTestLogStream()
>+
>+        self._handlers = configure_logging(log_stream)
>+        self._log_stream = log_stream
>+
>+    def tearDown(self):
>+        """Reset logging to its original state.
>+
>+        This method ensures that the logging configuration set up
>+        for a unit test does not affect logging in other unit tests.
>+
>+        """
>+        # This should be the same as the logger configured in the
>+        # configure_logging() method.
>+        logger = logging.getLogger()
>+        for handler in self._handlers:
>+            logger.removeHandler(handler)
>+
>+    def _log(self):
>+        return logging.getLogger("webkitpy")
>+
>+    def assert_log_messages(self, messages):
>+        """Assert that the logged messages equal the given messages."""
>+        self.assertEquals(messages, self._log_stream.messages)
>+
>+    def test_warning_message(self):
>+        self._log().warn("test message")
>+        self.assert_log_messages(["WARNING: test message\n"])
>+
>+    def test_below_warning_message(self):
>+        # We test the boundary case of a logging level equal to 29.
>+        # In practice, we will probably only be calling log.info(),
>+        # which corresponds to a logging level of 20.
>+        level = logging.WARNING - 1  # Equals 29.
>+        self._log().log(level, "test message")
>+        self.assert_log_messages(["test message\n"])
>+
>+    def test_debug_message(self):
>+        self._log().debug("test message")
>+        self.assert_log_messages([])
>+
>+    def test_two_messages(self):
>+        self._log().info("message1")
>+        self._log().info("message2")
>+        self.assert_log_messages(["message1\n", "message2\n"])
>+
>+
> class GlobalVariablesTest(unittest.TestCase):
> 
>     """Tests validity of the global variables."""
>@@ -446,11 +502,15 @@ class StyleCheckerCheckFileTest(unittest.TestCase):
> 
>     """
>     def setUp(self):
>+        self._log = UnitTestLog(self)
>         self.got_file_path = None
>         self.got_handle_style_error = None
>         self.got_processor = None
>         self.warning_messages = ""
> 
>+    def tearDown(self):
>+        self._log.tearDown()
>+
>     def mock_stderr_write(self, warning_message):
>         self.warning_messages += warning_message
> 
>@@ -523,8 +583,9 @@ class StyleCheckerCheckFileTest(unittest.TestCase):
> 
>         # Check the outcome.
>         self.call_check_file(file_path)
>-        self.assert_attributes(None, None, None,
>-                               'Ignoring "gtk2drawing.c": this file is exempt from the style guide.\n')
>+        self.assert_attributes(None, None, None, "")
>+        self._log.assertMessages(["WARNING: File exempt from style guide. "
>+                                  'Skipping: "gtk2drawing.c"\n'])
> 
>     def test_check_file_on_non_skipped(self):
> 
>diff --git a/WebKitTools/Scripts/webkitpy/style_references.py b/WebKitTools/Scripts/webkitpy/style_references.py
>index 2528c4d..cb8a985 100644
>--- a/WebKitTools/Scripts/webkitpy/style_references.py
>+++ b/WebKitTools/Scripts/webkitpy/style_references.py
>@@ -41,6 +41,8 @@
> import os
> 
> from diff_parser import DiffParser
>+from init.logtesting import UnitTestLog
>+from init.logtesting import UnitTestLogStream
> from scm import detect_scm_system
> 
>

Flags:

hamaji: review+
cjerdonek: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 35484: 49687 | 49688 | 49691 | 49787 | 49788 | 49805