diff options
Diffstat (limited to 'WebKitTools/Scripts/webkitpy/style')
25 files changed, 3121 insertions, 1537 deletions
diff --git a/WebKitTools/Scripts/webkitpy/style/checker.py b/WebKitTools/Scripts/webkitpy/style/checker.py index fbda8cb..e3c56c5 100644 --- a/WebKitTools/Scripts/webkitpy/style/checker.py +++ b/WebKitTools/Scripts/webkitpy/style/checker.py @@ -1,5 +1,6 @@ # Copyright (C) 2009 Google Inc. All rights reserved. # Copyright (C) 2010 Chris Jerdonek (chris.jerdonek@gmail.com) +# Copyright (C) 2010 ProFUSION embedded systems # # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are @@ -29,25 +30,26 @@ """Front end of some style-checker modules.""" -import codecs -import getopt +import logging import os.path import sys -from .. style_references import parse_patch +from checkers.common import categories as CommonCategories +from checkers.common import CarriageReturnChecker +from checkers.cpp import CppChecker +from checkers.python import PythonChecker +from checkers.text import TextChecker from error_handlers import DefaultStyleErrorHandler -from error_handlers import PatchStyleErrorHandler -from filter import validate_filter_rules from filter import FilterConfiguration -from processors.common import check_no_carriage_return -from processors.common import categories as CommonCategories -from processors.cpp import CppProcessor -from processors.text import TextProcessor +from optparser import ArgumentParser +from optparser import DefaultCommandOptionValues +from webkitpy.style_references import configure_logging as _configure_logging +_log = logging.getLogger("webkitpy.style.checker") -# These defaults are used by check-webkit-style. -WEBKIT_DEFAULT_VERBOSITY = 1 -WEBKIT_DEFAULT_OUTPUT_FORMAT = 'emacs' +# These are default option values for the command-line option parser. +_DEFAULT_MIN_CONFIDENCE = 1 +_DEFAULT_OUTPUT_FORMAT = 'emacs' # FIXME: For style categories we will never want to have, remove them. @@ -55,14 +57,16 @@ WEBKIT_DEFAULT_OUTPUT_FORMAT = 'emacs' # modify the implementation and enable them. # # Throughout this module, we use "filter rule" rather than "filter" -# for an individual boolean filter flag like "+foo". This allows us to +# for an individual boolean filter flag like "+foo". This allows us to # reserve "filter" for what one gets by collectively applying all of # the filter rules. # -# The _WEBKIT_FILTER_RULES are prepended to any user-specified filter -# rules. Since by default all errors are on, only include rules that -# begin with a - sign. -WEBKIT_DEFAULT_FILTER_RULES = [ +# The base filter rules are the filter rules that begin the list of +# filter rules used to check style. For example, these rules precede +# any user-specified filter rules. Since by default all categories are +# checked, this list should normally include only rules that begin +# with a "-" sign. +_BASE_FILTER_RULES = [ '-build/endif_comment', '-build/include_what_you_use', # <string> for std::string '-build/storage_class', # const static @@ -83,32 +87,41 @@ WEBKIT_DEFAULT_FILTER_RULES = [ '-whitespace/blank_line', '-whitespace/end_of_line', '-whitespace/labels', + # List Python pep8 categories last. + # + # Because much of WebKit's Python code base does not abide by the + # PEP8 79 character limit, we ignore the 79-character-limit category + # pep8/E501 for now. + # + # FIXME: Consider bringing WebKit's Python code base into conformance + # with the 79 character limit, or some higher limit that is + # agreeable to the WebKit project. + '-pep8/E501', ] -# FIXME: Change the second value of each tuple from a tuple to a list, -# and alter the filter code so it accepts lists instead. (The -# filter code will need to convert incoming values from a list -# to a tuple prior to caching). This will make this -# configuration setting a bit simpler since tuples have an -# unusual syntax case. -# # The path-specific filter rules. # # This list is order sensitive. Only the first path substring match # is used. See the FilterConfiguration documentation in filter.py # for more information on this list. +# +# Each string appearing in this nested list should have at least +# one associated unit test assertion. These assertions are located, +# for example, in the test_path_rules_specifier() unit test method of +# checker_unittest.py. _PATH_RULES_SPECIFIER = [ # Files in these directories are consumers of the WebKit # API and therefore do not follow the same header including # discipline as WebCore. (["WebKitTools/WebKitAPITest/", "WebKit/qt/QGVLauncher/"], - ("-build/include", - "-readability/streams")), - ([# The GTK+ APIs use GTK+ naming style, which includes - # lower-cased, underscore-separated values. - "WebKit/gtk/webkit/", + ["-build/include", + "-readability/streams"]), + ([# The EFL APIs use EFL naming style, which includes + # both lower-cased and camel-cased, underscore-sparated + # values. + "WebKit/efl/ewk/", # There is no clean way to avoid "yy_*" names used by flex. "WebCore/css/CSSParser.cpp", # There is no clean way to avoid "xxx_data" methods inside @@ -116,515 +129,282 @@ _PATH_RULES_SPECIFIER = [ # QtTest module. "WebKit/qt/tests/", "JavaScriptCore/qt/tests"], - ("-readability/naming",)), - # These are test file patterns. - (["_test.cpp", - "_unittest.cpp", - "_regtest.cpp"], - ("-readability/streams", # Many unit tests use cout. - "-runtime/rtti")), + ["-readability/naming"]), + ([# The GTK+ APIs use GTK+ naming style, which includes + # lower-cased, underscore-separated values. + # Also, GTK+ allows the use of NULL. + "WebKit/gtk/webkit/", + "WebKitTools/DumpRenderTree/gtk/"], + ["-readability/naming", + "-readability/null"]), + ([# Header files in ForwardingHeaders have no header guards or + # exceptional header guards (e.g., WebCore_FWD_Debugger_h). + "/ForwardingHeaders/"], + ["-build/header_guard"]), + + # For third-party Python code, keep only the following checks-- + # + # No tabs: to avoid having to set the SVN allow-tabs property. + # No trailing white space: since this is easy to correct. + # No carriage-return line endings: since this is easy to correct. + # + (["webkitpy/thirdparty/"], + ["-", + "+pep8/W191", # Tabs + "+pep8/W291", # Trailing white space + "+whitespace/carriage_return"]), ] +_CPP_FILE_EXTENSIONS = [ + 'c', + 'cpp', + 'h', + ] + +_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', + 'cgi', + 'css', + 'exp', + 'flex', + 'gyp', + 'gypi', + 'html', + 'idl', + 'in', + 'js', + 'mm', + 'php', + 'pl', + 'pm', + 'pri', + 'pro', + 'rb', + 'sh', + 'txt', +# 'vcproj', # See FIXME above. + 'wm', + 'xhtml', + 'y', + ] + + +# Files to skip that are less obvious. +# # Some files should be skipped when checking style. For example, # WebKit maintains some files in Mozilla style on purpose to ease # future merges. -# -# Include a warning for skipped files that are less obvious. -SKIPPED_FILES_WITH_WARNING = [ +_SKIPPED_FILES_WITH_WARNING = [ # The Qt API and tests do not follow WebKit style. # They follow Qt style. :) "gtk2drawing.c", # WebCore/platform/gtk/gtk2drawing.c - "gtk2drawing.h", # WebCore/platform/gtk/gtk2drawing.h + "gtkdrawing.h", # WebCore/platform/gtk/gtkdrawing.h "JavaScriptCore/qt/api/", "WebKit/gtk/tests/", "WebKit/qt/Api/", "WebKit/qt/tests/", + "WebKit/qt/examples/", ] -# Don't include a warning for skipped files that are more common -# and more obvious. -SKIPPED_FILES_WITHOUT_WARNING = [ - "LayoutTests/" +# Files to skip that are more common or obvious. +# +# This list should be in addition to files with FileType.NONE. Files +# with FileType.NONE are automatically skipped without warning. +_SKIPPED_FILES_WITHOUT_WARNING = [ + "LayoutTests/", ] # The maximum number of errors to report per file, per category. # If a category is not a key, then it has no maximum. -MAX_REPORTS_PER_CATEGORY = { +_MAX_REPORTS_PER_CATEGORY = { "whitespace/carriage_return": 1 } -def style_categories(): +def _all_categories(): """Return the set of all categories used by check-webkit-style.""" - # Take the union across all processors. - return CommonCategories.union(CppProcessor.categories) - - -def webkit_argument_defaults(): - """Return the DefaultArguments instance for use by check-webkit-style.""" - return ArgumentDefaults(WEBKIT_DEFAULT_OUTPUT_FORMAT, - WEBKIT_DEFAULT_VERBOSITY, - WEBKIT_DEFAULT_FILTER_RULES) - - -def _create_usage(defaults): - """Return the usage string to display for command help. + # Take the union across all checkers. + categories = CommonCategories.union(CppChecker.categories) - Args: - defaults: An ArgumentDefaults instance. - - """ - usage = """ -Syntax: %(program_name)s [--verbose=#] [--git-commit=<SingleCommit>] [--output=vs7] - [--filter=-x,+y,...] [file] ... - - The style guidelines this tries to follow are here: - http://webkit.org/coding/coding-style.html - - Every style error is given a confidence score from 1-5, with 5 meaning - we are certain of the problem, and 1 meaning it could be a legitimate - construct. This can miss some errors and does not substitute for - code review. - - To prevent specific lines from being linted, add a '// NOLINT' comment to the - end of the line. - - Linted extensions are .cpp, .c and .h. Other file types are ignored. - - The file parameter is optional and accepts multiple files. Leaving - out the file parameter applies the check to all files considered changed - by your source control management system. - - Flags: - - verbose=# - A number 1-5 that restricts output to errors with a confidence - score at or above this value. In particular, the value 1 displays - all errors. The default is %(default_verbosity)s. + # FIXME: Consider adding all of the pep8 categories. Since they + # are not too meaningful for documentation purposes, for + # now we add only the categories needed for the unit tests + # (which validate the consistency of the configuration + # settings against the known categories, etc). + categories = categories.union(["pep8/W191", "pep8/W291", "pep8/E501"]) - git-commit=<SingleCommit> - Checks the style of everything from the given commit to the local tree. + return categories - output=vs7 - The output format, which may be one of - emacs : to ease emacs parsing - vs7 : compatible with Visual Studio - Defaults to "%(default_output_format)s". Other formats are unsupported. - filter=-x,+y,... - A comma-separated list of boolean filter rules used to filter - which categories of style guidelines to check. The script checks - a category if the category passes the filter rules, as follows. +def _check_webkit_style_defaults(): + """Return the default command-line options for check-webkit-style.""" + return DefaultCommandOptionValues(min_confidence=_DEFAULT_MIN_CONFIDENCE, + output_format=_DEFAULT_OUTPUT_FORMAT) - Any webkit category starts out passing. All filter rules are then - evaluated left to right, with later rules taking precedence. For - example, the rule "+foo" passes any category that starts with "foo", - and "-foo" fails any such category. The filter input "-whitespace, - +whitespace/braces" fails the category "whitespace/tab" and passes - "whitespace/braces". - - Examples: --filter=-whitespace,+whitespace/braces - --filter=-whitespace,-runtime/printf,+runtime/printf_format - --filter=-,+build/include_what_you_use - - Category names appear in error messages in brackets, for example - [whitespace/indent]. To see a list of all categories available to - %(program_name)s, along with which are enabled by default, pass - the empty filter as follows: - --filter= -""" % {'program_name': os.path.basename(sys.argv[0]), - 'default_verbosity': defaults.verbosity, - 'default_output_format': defaults.output_format} - - return usage - - -# FIXME: Eliminate support for "extra_flag_values". -# -# FIXME: Remove everything from ProcessorOptions except for the -# information that can be passed via the command line, and -# rename to something like CheckWebKitStyleOptions. This -# includes, but is not limited to, removing the -# max_reports_per_error attribute and the is_reportable() -# method. See also the FIXME below to create a new class -# called something like CheckerConfiguration. -# -# This class should not have knowledge of the flag key names. -class ProcessorOptions(object): - - """A container to store options passed via the command line. - - Attributes: - extra_flag_values: A string-string dictionary of all flag key-value - pairs that are not otherwise represented by this - class. The default is the empty dictionary. - filter_configuration: A FilterConfiguration instance. The default - is the "empty" filter configuration, which - means that all errors should be checked. +# This function assists in optparser not having to import from checker. +def check_webkit_style_parser(): + all_categories = _all_categories() + default_options = _check_webkit_style_defaults() + return ArgumentParser(all_categories=all_categories, + base_filter_rules=_BASE_FILTER_RULES, + default_options=default_options) - git_commit: A string representing the git commit to check. - The default is None. - max_reports_per_error: The maximum number of errors to report - per file, per category. +def check_webkit_style_configuration(options): + """Return a StyleProcessorConfiguration instance for check-webkit-style. - output_format: A string that is the output format. The supported - output formats are "emacs" which emacs can parse - and "vs7" which Microsoft Visual Studio 7 can parse. - - verbosity: An integer between 1-5 inclusive that restricts output - to errors with a confidence score at or above this value. - The default is 1, which reports all errors. + Args: + options: A CommandOptionValues instance. """ - def __init__(self, - extra_flag_values=None, - filter_configuration = None, - git_commit=None, - max_reports_per_category=None, - output_format="emacs", - verbosity=1): - if extra_flag_values is None: - extra_flag_values = {} - if filter_configuration is None: - filter_configuration = FilterConfiguration() - if max_reports_per_category is None: - max_reports_per_category = {} - - if output_format not in ("emacs", "vs7"): - raise ValueError('Invalid "output_format" parameter: ' - 'value must be "emacs" or "vs7". ' - 'Value given: "%s".' % output_format) - - if (verbosity < 1) or (verbosity > 5): - raise ValueError('Invalid "verbosity" parameter: ' - "value must be an integer between 1-5 inclusive. " - 'Value given: "%s".' % verbosity) - - self.extra_flag_values = extra_flag_values - self.filter_configuration = filter_configuration - self.git_commit = git_commit - self.max_reports_per_category = max_reports_per_category - self.output_format = output_format - self.verbosity = verbosity - - # Useful for unit testing. - def __eq__(self, other): - """Return whether this ProcessorOptions instance is equal to another.""" - if self.extra_flag_values != other.extra_flag_values: - return False - if self.filter_configuration != other.filter_configuration: - return False - if self.git_commit != other.git_commit: - return False - if self.max_reports_per_category != other.max_reports_per_category: - return False - if self.output_format != other.output_format: - return False - if self.verbosity != other.verbosity: - return False - - return True - - # Useful for unit testing. - def __ne__(self, other): - # Python does not automatically deduce this from __eq__(). - return not self.__eq__(other) - - def is_reportable(self, category, confidence_in_error, path): - """Return whether an error is reportable. + filter_configuration = FilterConfiguration( + base_rules=_BASE_FILTER_RULES, + path_specific=_PATH_RULES_SPECIFIER, + user_rules=options.filter_rules) - An error is reportable if the confidence in the error - is at least the current verbosity level, and if the current - filter says that the category should be checked for the - given path. + return StyleProcessorConfiguration(filter_configuration=filter_configuration, + max_reports_per_category=_MAX_REPORTS_PER_CATEGORY, + min_confidence=options.min_confidence, + output_format=options.output_format, + stderr_write=sys.stderr.write) - Args: - category: A string that is a style category. - confidence_in_error: An integer between 1 and 5, inclusive, that - represents the application's confidence in - the error. A higher number signifies greater - confidence. - path: The path of the file being checked - """ - if confidence_in_error < self.verbosity: - return False +def _create_log_handlers(stream): + """Create and return a default list of logging.Handler instances. - return self.filter_configuration.should_check(category, path) + Format WARNING messages and above to display the logging level, and + messages strictly below WARNING not to display it. - -# This class should not have knowledge of the flag key names. -class ArgumentDefaults(object): - - """A container to store default argument values. - - Attributes: - output_format: A string that is the default output format. - verbosity: An integer that is the default verbosity level. - base_filter_rules: A list of strings that are boolean filter rules - to prepend to any user-specified rules. + Args: + stream: See the configure_logging() docstring. """ + # 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) - def __init__(self, default_output_format, default_verbosity, - default_base_filter_rules): - self.output_format = default_output_format - self.verbosity = default_verbosity - self.base_filter_rules = default_base_filter_rules + # Create a logging.Filter instance that only accepts messages + # below WARNING (i.e. filters out anything WARNING or above). + non_error_filter = logging.Filter() + # The filter method accepts a logging.LogRecord instance. + non_error_filter.filter = lambda record: record.levelno < logging.WARNING + non_error_handler = logging.StreamHandler(stream) + non_error_handler.addFilter(non_error_filter) + formatter = logging.Formatter("%(message)s") + non_error_handler.setFormatter(formatter) -class ArgumentPrinter(object): + return [error_handler, non_error_handler] - """Supports the printing of check-webkit-style command arguments.""" - def _flag_pair_to_string(self, flag_key, flag_value): - return '--%(key)s=%(val)s' % {'key': flag_key, 'val': flag_value } +def _create_debug_log_handlers(stream): + """Create and return a list of logging.Handler instances for debugging. - def to_flag_string(self, options): - """Return a flag string yielding the given ProcessorOptions instance. - - This method orders the flag values alphabetically by the flag key. - - Args: - options: A ProcessorOptions instance. - - """ - flags = options.extra_flag_values.copy() - - flags['output'] = options.output_format - flags['verbose'] = options.verbosity - # Only include the filter flag if user-provided rules are present. - user_rules = options.filter_configuration.user_rules - if user_rules: - flags['filter'] = ",".join(user_rules) - if options.git_commit: - flags['git-commit'] = options.git_commit - - flag_string = '' - # Alphabetizing lets us unit test this method. - for key in sorted(flags.keys()): - flag_string += self._flag_pair_to_string(key, flags[key]) + ' ' - - return flag_string.strip() - - -class ArgumentParser(object): - - """Supports the parsing of check-webkit-style command arguments. - - Attributes: - defaults: An ArgumentDefaults instance. - create_usage: A function that accepts an ArgumentDefaults instance - and returns a string of usage instructions. - This defaults to the function used to generate the - usage string for check-webkit-style. - doc_print: A function that accepts a string parameter and that is - called to display help messages. This defaults to - sys.stderr.write(). + Args: + stream: See the configure_logging() docstring. """ + handler = logging.StreamHandler(stream) + formatter = logging.Formatter("%(name)s: %(levelname)-8s %(message)s") + handler.setFormatter(formatter) - def __init__(self, argument_defaults, create_usage=None, doc_print=None): - if create_usage is None: - create_usage = _create_usage - if doc_print is None: - doc_print = sys.stderr.write + return [handler] - self.defaults = argument_defaults - self.create_usage = create_usage - self.doc_print = doc_print - def _exit_with_usage(self, error_message=''): - """Exit and print a usage string with an optional error message. +def configure_logging(stream, logger=None, is_verbose=False): + """Configure logging, and return the list of handlers added. - Args: - error_message: A string that is an error message to print. - - """ - usage = self.create_usage(self.defaults) - self.doc_print(usage) - if error_message: - sys.exit('\nFATAL ERROR: ' + error_message) - else: - sys.exit(1) - - def _exit_with_categories(self): - """Exit and print the style categories and default filter rules.""" - self.doc_print('\nAll categories:\n') - categories = style_categories() - for category in sorted(categories): - self.doc_print(' ' + category + '\n') - - self.doc_print('\nDefault filter rules**:\n') - for filter_rule in sorted(self.defaults.base_filter_rules): - self.doc_print(' ' + filter_rule + '\n') - self.doc_print('\n**The command always evaluates the above rules, ' - 'and before any --filter flag.\n\n') - - sys.exit(0) - - def _parse_filter_flag(self, flag_value): - """Parse the --filter flag, and return a list of filter rules. - - Args: - flag_value: A string of comma-separated filter rules, for - example "-whitespace,+whitespace/indent". + 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. - """ - filters = [] - for uncleaned_filter in flag_value.split(','): - filter = uncleaned_filter.strip() - if not filter: - continue - filters.append(filter) - return filters - - def parse(self, args, extra_flags=None): - """Parse the command line arguments to check-webkit-style. - - Args: - args: A list of command-line arguments as returned by sys.argv[1:]. - extra_flags: A list of flags whose values we want to extract, but - are not supported by the ProcessorOptions class. - An example flag "new_flag=". This defaults to the - empty list. - - Returns: - A tuple of (filenames, options) - - filenames: The list of filenames to check. - options: A ProcessorOptions instance. + Args: + stream: A file-like object to which to log. The stream must + define an "encoding" data attribute, or else logging + raises an error. + logger: A logging.logger instance to configure. This parameter + should be used only in unit tests. Defaults to the + root logger. + is_verbose: A boolean value of whether logging should be verbose. - """ - if extra_flags is None: - extra_flags = [] - - output_format = self.defaults.output_format - verbosity = self.defaults.verbosity - base_rules = self.defaults.base_filter_rules - - # The flags already supported by the ProcessorOptions class. - flags = ['help', 'output=', 'verbose=', 'filter=', 'git-commit='] - - for extra_flag in extra_flags: - if extra_flag in flags: - raise ValueError('Flag \'%(extra_flag)s is duplicated ' - 'or already supported.' % - {'extra_flag': extra_flag}) - flags.append(extra_flag) - - try: - (opts, filenames) = getopt.getopt(args, '', flags) - except getopt.GetoptError: - # FIXME: Settle on an error handling approach: come up - # with a consistent guideline as to when and whether - # a ValueError should be raised versus calling - # sys.exit when needing to interrupt execution. - self._exit_with_usage('Invalid arguments.') - - extra_flag_values = {} - git_commit = None - user_rules = [] - - for (opt, val) in opts: - if opt == '--help': - self._exit_with_usage() - elif opt == '--output': - output_format = val - elif opt == '--verbose': - verbosity = val - elif opt == '--git-commit': - git_commit = val - elif opt == '--filter': - if not val: - self._exit_with_categories() - # Prepend the defaults. - user_rules = self._parse_filter_flag(val) - else: - extra_flag_values[opt] = val - - # Check validity of resulting values. - if filenames and (git_commit != None): - self._exit_with_usage('It is not possible to check files and a ' - 'specific commit at the same time.') - - if output_format not in ('emacs', 'vs7'): - raise ValueError('Invalid --output value "%s": The only ' - 'allowed output formats are emacs and vs7.' % - output_format) - - all_categories = style_categories() - validate_filter_rules(user_rules, all_categories) - - verbosity = int(verbosity) - if (verbosity < 1) or (verbosity > 5): - raise ValueError('Invalid --verbose value %s: value must ' - 'be between 1-5.' % verbosity) - - filter_configuration = FilterConfiguration(base_rules=base_rules, - path_specific=_PATH_RULES_SPECIFIER, - user_rules=user_rules) - - options = ProcessorOptions(extra_flag_values=extra_flag_values, - filter_configuration=filter_configuration, - git_commit=git_commit, - max_reports_per_category=MAX_REPORTS_PER_CATEGORY, - output_format=output_format, - verbosity=verbosity) - - return (filenames, options) + """ + # 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 + if logger is None: + logger = logging.getLogger() + + if is_verbose: + logging_level = logging.DEBUG + handlers = _create_debug_log_handlers(stream) + else: + logging_level = logging.INFO + handlers = _create_log_handlers(stream) + + handlers = _configure_logging(logging_level=logging_level, logger=logger, + handlers=handlers) + + return handlers # Enum-like idiom class FileType: - NONE = 1 + NONE = 0 # FileType.NONE evaluates to False. # Alphabetize remaining types - CPP = 2 + CPP = 1 + PYTHON = 2 TEXT = 3 -class ProcessorDispatcher(object): +class CheckerDispatcher(object): """Supports determining whether and how to check style, based on path.""" - cpp_file_extensions = ( - 'c', - 'cpp', - 'h', - ) - - text_file_extensions = ( - 'css', - 'html', - 'idl', - 'js', - 'mm', - 'php', - 'pm', - 'py', - 'txt', - ) - def _file_extension(self, file_path): """Return the file extension without the leading dot.""" return os.path.splitext(file_path)[1].lstrip(".") def should_skip_with_warning(self, file_path): """Return whether the given file should be skipped with a warning.""" - for skipped_file in SKIPPED_FILES_WITH_WARNING: + for skipped_file in _SKIPPED_FILES_WITH_WARNING: if file_path.find(skipped_file) >= 0: return True return False def should_skip_without_warning(self, file_path): """Return whether the given file should be skipped without a warning.""" - for skipped_file in SKIPPED_FILES_WITHOUT_WARNING: + if not self._file_type(file_path): # FileType.NONE. + return True + # Since "LayoutTests" is in _SKIPPED_FILES_WITHOUT_WARNING, make + # an exception to prevent files like "LayoutTests/ChangeLog" and + # "LayoutTests/ChangeLog-2009-06-16" from being skipped. + # + # FIXME: Figure out a good way to avoid having to add special logic + # for this special case. + if os.path.basename(file_path).startswith('ChangeLog'): + return False + for skipped_file in _SKIPPED_FILES_WITHOUT_WARNING: if file_path.find(skipped_file) >= 0: return True return False @@ -633,7 +413,7 @@ class ProcessorDispatcher(object): """Return the file type corresponding to the given file.""" file_extension = self._file_extension(file_path) - if (file_extension in self.cpp_file_extensions) or (file_path == '-'): + if (file_extension in _CPP_FILE_EXTENSIONS) or (file_path == '-'): # FIXME: Do something about the comment below and the issue it # raises since cpp_style already relies on the extension. # @@ -641,22 +421,28 @@ class ProcessorDispatcher(object): # reading from stdin, cpp_style tests should not rely on # the extension. return FileType.CPP - elif ("ChangeLog" in file_path - or "WebKitTools/Scripts/" in file_path - or file_extension in self.text_file_extensions): + elif file_extension == _PYTHON_FILE_EXTENSION: + return FileType.PYTHON + 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): return FileType.TEXT else: return FileType.NONE - def _create_processor(self, file_type, file_path, handle_style_error, verbosity): - """Instantiate and return a style processor based on file type.""" + def _create_checker(self, file_type, file_path, handle_style_error, + min_confidence): + """Instantiate and return a style checker based on file type.""" if file_type == FileType.NONE: - processor = None + checker = None elif file_type == FileType.CPP: file_extension = self._file_extension(file_path) - processor = CppProcessor(file_path, file_extension, handle_style_error, verbosity) + checker = CppChecker(file_path, file_extension, + handle_style_error, min_confidence) + elif file_type == FileType.PYTHON: + checker = PythonChecker(file_path, handle_style_error) elif file_type == FileType.TEXT: - processor = TextProcessor(file_path, handle_style_error) + checker = TextChecker(file_path, handle_style_error) else: raise ValueError('Invalid file type "%(file_type)s": the only valid file types ' "are %(NONE)s, %(CPP)s, and %(TEXT)s." @@ -665,164 +451,249 @@ class ProcessorDispatcher(object): "CPP": FileType.CPP, "TEXT": FileType.TEXT}) - return processor + return checker - def dispatch_processor(self, file_path, handle_style_error, verbosity): - """Instantiate and return a style processor based on file path.""" + def dispatch(self, file_path, handle_style_error, min_confidence): + """Instantiate and return a style checker based on file path.""" file_type = self._file_type(file_path) - processor = self._create_processor(file_type, - file_path, - handle_style_error, - verbosity) - return processor + checker = self._create_checker(file_type, + file_path, + handle_style_error, + min_confidence) + return checker -# FIXME: When creating the new CheckWebKitStyleOptions class as -# described in a FIXME above, add a new class here called -# something like CheckerConfiguration. The class should contain -# attributes for options needed to process a file. This includes -# a subset of the CheckWebKitStyleOptions attributes, a -# FilterConfiguration attribute, an stderr_write attribute, a -# max_reports_per_category attribute, etc. It can also include -# the is_reportable() method. The StyleChecker should accept -# an instance of this class rather than a ProcessorOptions -# instance. +# FIXME: Remove the stderr_write attribute from this class and replace +# its use with calls to a logging module logger. +class StyleProcessorConfiguration(object): + """Stores configuration values for the StyleProcessor class. -class StyleChecker(object): + Attributes: + min_confidence: An integer between 1 and 5 inclusive that is the + minimum confidence level of style errors to report. - """Supports checking style in files and patches. + max_reports_per_category: The maximum number of errors to report + per category, per file. - Attributes: - error_count: An integer that is the total number of reported - errors for the lifetime of this StyleChecker - instance. - options: A ProcessorOptions instance that controls the behavior - of style checking. + stderr_write: A function that takes a string as a parameter and + serves as stderr.write. """ - def __init__(self, options, stderr_write=None): - """Create a StyleChecker instance. + def __init__(self, + filter_configuration, + max_reports_per_category, + min_confidence, + output_format, + stderr_write): + """Create a StyleProcessorConfiguration instance. Args: - options: See options attribute. - stderr_write: A function that takes a string as a parameter - and that is called when a style error occurs. - Defaults to sys.stderr.write. This should be - used only for unit tests. + filter_configuration: A FilterConfiguration instance. The default + is the "empty" filter configuration, which + means that all errors should be checked. + + max_reports_per_category: The maximum number of errors to report + per category, per file. + + min_confidence: An integer between 1 and 5 inclusive that is the + minimum confidence level of style errors to report. + The default is 1, which reports all style errors. + + output_format: A string that is the output format. The supported + output formats are "emacs" which emacs can parse + and "vs7" which Microsoft Visual Studio 7 can parse. + + stderr_write: A function that takes a string as a parameter and + serves as stderr.write. """ - if stderr_write is None: - stderr_write = sys.stderr.write + self._filter_configuration = filter_configuration + self._output_format = output_format - self._stderr_write = stderr_write - self.error_count = 0 - self.options = options - - def _increment_error_count(self): - """Increment the total count of reported errors.""" - self.error_count += 1 - - def _process_file(self, processor, file_path, handle_style_error): - """Process the file using the given processor.""" - try: - # Support the UNIX convention of using "-" for stdin. Note that - # we are not opening the file with universal newline support - # (which codecs doesn't support anyway), so the resulting lines do - # contain trailing '\r' characters if we are reading a file that - # has CRLF endings. - # If after the split a trailing '\r' is present, it is removed - # below. If it is not expected to be present (i.e. os.linesep != - # '\r\n' as in Windows), a warning is issued below if this file - # is processed. - if file_path == '-': - file = codecs.StreamReaderWriter(sys.stdin, - codecs.getreader('utf8'), - codecs.getwriter('utf8'), - 'replace') - else: - file = codecs.open(file_path, 'r', 'utf8', 'replace') - - contents = file.read() - - except IOError: - self._stderr_write("Skipping input '%s': Can't open for reading\n" % file_path) - return - - lines = contents.split("\n") - - 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 - # the carriage-return check. Originally, we did the - # check only if (os.linesep != '\r\n'). - # - # FIXME: As a minor optimization, we can have - # check_no_carriage_return() return whether - # the line ends with "\r". - check_no_carriage_return(lines[line_number], line_number, - handle_style_error) - if lines[line_number].endswith("\r"): - lines[line_number] = lines[line_number].rstrip("\r") + self.max_reports_per_category = max_reports_per_category + self.min_confidence = min_confidence + self.stderr_write = stderr_write + + def is_reportable(self, category, confidence_in_error, file_path): + """Return whether an error is reportable. + + An error is reportable if both the confidence in the error is + at least the minimum confidence level and the current filter + says the category should be checked for the given path. + + Args: + category: A string that is a style category. + confidence_in_error: An integer between 1 and 5 inclusive that is + the application's confidence in the error. + A higher number means greater confidence. + file_path: The path of the file being checked + + """ + if confidence_in_error < self.min_confidence: + return False - processor.process(lines) + return self._filter_configuration.should_check(category, file_path) + + def write_style_error(self, + category, + confidence_in_error, + file_path, + line_number, + message): + """Write a style error to the configured stderr.""" + if self._output_format == 'vs7': + format_string = "%s(%s): %s [%s] [%d]\n" + else: + format_string = "%s:%s: %s [%s] [%d]\n" + + self.stderr_write(format_string % (file_path, + line_number, + message, + category, + confidence_in_error)) + + +class ProcessorBase(object): + + """The base class for processors of lists of lines.""" - def check_file(self, file_path, handle_style_error=None, process_file=None): - """Check style in the given file. + def should_process(self, file_path): + """Return whether the file at file_path should be processed. + + The TextFileReader class calls this method prior to reading in + the lines of a file. Use this method, for example, to prevent + the style checker from reading binary files into memory. + + """ + raise NotImplementedError('Subclasses should implement.') + + def process(self, lines, file_path, **kwargs): + """Process lines of text read from a file. Args: - file_path: A string that is the path of the file to process. - handle_style_error: The function to call when a style error - occurs. This parameter is meant for internal - use within this class. Defaults to a - DefaultStyleErrorHandler instance. - process_file: The function to call to process the file. This - parameter should be used only for unit tests. - Defaults to the file processing method of this class. + lines: A list of lines of text to process. + file_path: The path from which the lines were read. + **kwargs: This argument signifies that the process() method of + subclasses of ProcessorBase may support additional + keyword arguments. + For example, a style checker's check() method + may support a "reportable_lines" parameter that represents + the line numbers of the lines for which style errors + should be reported. """ - if handle_style_error is None: - handle_style_error = DefaultStyleErrorHandler(file_path, - self.options, - self._increment_error_count, - self._stderr_write) - if process_file is None: - process_file = self._process_file - - dispatcher = ProcessorDispatcher() - - 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) - return - - verbosity = self.options.verbosity - processor = dispatcher.dispatch_processor(file_path, - handle_style_error, - verbosity) - if processor is None: - return - - process_file(processor, file_path, handle_style_error) - - def check_patch(self, patch_string): - """Check style in the given patch. + raise NotImplementedError('Subclasses should implement.') + + +class StyleProcessor(ProcessorBase): + + """A ProcessorBase for checking style. + + Attributes: + error_count: An integer that is the total number of reported + errors for the lifetime of this instance. + + """ + + def __init__(self, configuration, mock_dispatcher=None, + mock_increment_error_count=None, + mock_carriage_checker_class=None): + """Create an instance. Args: - patch_string: A string that is a patch string. + configuration: A StyleProcessorConfiguration instance. + mock_dispatcher: A mock CheckerDispatcher instance. This + parameter is for unit testing. Defaults to a + CheckerDispatcher instance. + mock_increment_error_count: A mock error-count incrementer. + mock_carriage_checker_class: A mock class for checking and + transforming carriage returns. + This parameter is for unit testing. + Defaults to CarriageReturnChecker. """ - patch_files = parse_patch(patch_string) - for file_path, diff in patch_files.iteritems(): - style_error_handler = PatchStyleErrorHandler(diff, - file_path, - self.options, - self._increment_error_count, - self._stderr_write) + if mock_dispatcher is None: + dispatcher = CheckerDispatcher() + else: + dispatcher = mock_dispatcher + + if mock_increment_error_count is None: + # The following blank line is present to avoid flagging by pep8.py. + + def increment_error_count(): + """Increment the total count of reported errors.""" + self.error_count += 1 + else: + increment_error_count = mock_increment_error_count + + if mock_carriage_checker_class is None: + # This needs to be a class rather than an instance since the + # process() method instantiates one using parameters. + carriage_checker_class = CarriageReturnChecker + else: + carriage_checker_class = mock_carriage_checker_class + + self.error_count = 0 + + self._carriage_checker_class = carriage_checker_class + self._configuration = configuration + self._dispatcher = dispatcher + self._increment_error_count = increment_error_count + + def should_process(self, file_path): + """Return whether the file should be checked for style.""" + if self._dispatcher.should_skip_without_warning(file_path): + return False + if self._dispatcher.should_skip_with_warning(file_path): + _log.warn('File exempt from style guide. Skipping: "%s"' + % file_path) + return False + return True + + def process(self, lines, file_path, line_numbers=None): + """Check the given lines for style. + + Arguments: + lines: A list of all lines in the file to check. + file_path: The path of the file to process. If possible, the path + should be relative to the source root. Otherwise, + path-specific logic may not behave as expected. + line_numbers: A list of line numbers of the lines for which + style errors should be reported, or None if errors + for all lines should be reported. When not None, this + list normally contains the line numbers corresponding + to the modified lines of a patch. + + """ + _log.debug("Checking style: " + file_path) + + style_error_handler = DefaultStyleErrorHandler( + configuration=self._configuration, + file_path=file_path, + increment_error_count=self._increment_error_count, + line_numbers=line_numbers) + + 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) + + min_confidence = self._configuration.min_confidence + checker = self._dispatcher.dispatch(file_path, + style_error_handler, + min_confidence) + + if checker is None: + raise AssertionError("File should not be checked: '%s'" % file_path) - self.check_file(file_path, style_error_handler) + _log.debug("Using class: " + checker.__class__.__name__) + checker.check(lines) diff --git a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py index e1c9baf..5254275 100755 --- a/WebKitTools/Scripts/webkitpy/style/checker_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/checker_unittest.py @@ -34,103 +34,119 @@ """Unit tests for style.py.""" +import logging +import os import unittest import checker as style +from webkitpy.style_references import LogTesting +from webkitpy.style_references import TestLogStream +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 style_categories -from checker import ProcessorDispatcher -from checker import ProcessorOptions -from checker import StyleChecker +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 CheckerDispatcher +from checker import ProcessorBase +from checker import StyleProcessor +from checker import StyleProcessorConfiguration +from checkers.cpp import CppChecker +from checkers.python import PythonChecker +from checkers.text import TextChecker +from error_handlers import DefaultStyleErrorHandler from filter import validate_filter_rules from filter import FilterConfiguration -from processors.cpp import CppProcessor -from processors.text import TextProcessor +from optparser import ArgumentParser +from optparser import CommandOptionValues +from webkitpy.common.system.logtesting import LoggingTestCase +from webkitpy.style.filereader import TextFileReader -class ProcessorOptionsTest(unittest.TestCase): +class ConfigureLoggingTestBase(unittest.TestCase): - """Tests ProcessorOptions class.""" + """Base class for testing configure_logging(). - def test_init(self): - """Test __init__ constructor.""" - # Check default parameters. - options = ProcessorOptions() - self.assertEquals(options.extra_flag_values, {}) - self.assertEquals(options.filter_configuration, FilterConfiguration()) - self.assertEquals(options.git_commit, None) - self.assertEquals(options.max_reports_per_category, {}) - self.assertEquals(options.output_format, "emacs") - self.assertEquals(options.verbosity, 1) - - # Check argument validation. - self.assertRaises(ValueError, ProcessorOptions, output_format="bad") - ProcessorOptions(output_format="emacs") # No ValueError: works - ProcessorOptions(output_format="vs7") # works - self.assertRaises(ValueError, ProcessorOptions, verbosity=0) - self.assertRaises(ValueError, ProcessorOptions, verbosity=6) - ProcessorOptions(verbosity=1) # works - ProcessorOptions(verbosity=5) # works - - # Check attributes. - filter_configuration = FilterConfiguration(base_rules=["+"]) - options = ProcessorOptions(extra_flag_values={"extra_value" : 2}, - filter_configuration=filter_configuration, - git_commit="commit", - max_reports_per_category={"category": 3}, - output_format="vs7", - verbosity=3) - self.assertEquals(options.extra_flag_values, {"extra_value" : 2}) - self.assertEquals(options.filter_configuration, filter_configuration) - self.assertEquals(options.git_commit, "commit") - self.assertEquals(options.max_reports_per_category, {"category": 3}) - self.assertEquals(options.output_format, "vs7") - self.assertEquals(options.verbosity, 3) - - def test_eq(self): - """Test __eq__ equality function.""" - # == calls __eq__. - self.assertTrue(ProcessorOptions() == ProcessorOptions()) - - # Verify that a difference in any argument causes equality to fail. - filter_configuration = FilterConfiguration(base_rules=["+"]) - options = ProcessorOptions(extra_flag_values={"extra_value" : 1}, - filter_configuration=filter_configuration, - git_commit="commit", - max_reports_per_category={"category": 3}, - output_format="vs7", - verbosity=1) - self.assertFalse(options == ProcessorOptions(extra_flag_values={"extra_value" : 2})) - new_config = FilterConfiguration(base_rules=["-"]) - self.assertFalse(options == - ProcessorOptions(filter_configuration=new_config)) - self.assertFalse(options == ProcessorOptions(git_commit="commit2")) - self.assertFalse(options == ProcessorOptions(max_reports_per_category= - {"category": 2})) - self.assertFalse(options == ProcessorOptions(output_format="emacs")) - self.assertFalse(options == ProcessorOptions(verbosity=2)) - - def test_ne(self): - """Test __ne__ inequality function.""" - # != calls __ne__. - # By default, __ne__ always returns true on different objects. - # Thus, just check the distinguishing case to verify that the - # code defines __ne__. - self.assertFalse(ProcessorOptions() != ProcessorOptions()) + Sub-classes should implement: - def test_is_reportable(self): - """Test is_reportable().""" - filter_configuration = FilterConfiguration(base_rules=["-xyz"]) - options = ProcessorOptions(filter_configuration=filter_configuration, - verbosity=3) + is_verbose: The is_verbose value to pass to configure_logging(). + + """ + + def setUp(self): + is_verbose = self.is_verbose + + log_stream = TestLogStream(self) + + # Use a logger other than the root logger or one prefixed with + # webkit so as not to conflict with test-webkitpy logging. + logger = logging.getLogger("unittest") + + # Configure the test logger not to pass messages along to the + # root logger. This prevents test messages from being + # propagated to loggers used by test-webkitpy logging (e.g. + # the root logger). + logger.propagate = False + + self._handlers = configure_logging(stream=log_stream, logger=logger, + is_verbose=is_verbose) + self._log = logger + 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. + + """ + logger = self._log + for handler in self._handlers: + logger.removeHandler(handler) + + def assert_log_messages(self, messages): + """Assert that the logged messages equal the given messages.""" + self._log_stream.assertMessages(messages) + + +class ConfigureLoggingTest(ConfigureLoggingTestBase): + + """Tests the configure_logging() function.""" + + is_verbose = False + + 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 ConfigureLoggingVerboseTest(ConfigureLoggingTestBase): - # Test verbosity - self.assertTrue(options.is_reportable("abc", 3, "foo.h")) - self.assertFalse(options.is_reportable("abc", 2, "foo.h")) + """Tests the configure_logging() function with is_verbose True.""" - # Test filter - self.assertTrue(options.is_reportable("xy", 3, "foo.h")) - self.assertFalse(options.is_reportable("xyz", 3, "foo.h")) + is_verbose = True + + def test_debug_message(self): + self._log.debug("test message") + self.assert_log_messages(["unittest: DEBUG test message\n"]) class GlobalVariablesTest(unittest.TestCase): @@ -138,18 +154,18 @@ class GlobalVariablesTest(unittest.TestCase): """Tests validity of the global variables.""" def _all_categories(self): - return style.style_categories() + return _all_categories() def defaults(self): - return style.webkit_argument_defaults() + return style._check_webkit_style_defaults() - def test_filter_rules(self): + def test_webkit_base_filter_rules(self): + base_filter_rules = _BASE_FILTER_RULES defaults = self.defaults() already_seen = [] - validate_filter_rules(defaults.base_filter_rules, - self._all_categories()) + validate_filter_rules(base_filter_rules, self._all_categories()) # Also do some additional checks. - for rule in defaults.base_filter_rules: + for rule in base_filter_rules: # Check no leading or trailing white space. self.assertEquals(rule, rule.strip()) # All categories are on by default, so defaults should @@ -161,274 +177,186 @@ class GlobalVariablesTest(unittest.TestCase): def test_defaults(self): """Check that default arguments are valid.""" - defaults = self.defaults() + default_options = self.defaults() # FIXME: We should not need to call parse() to determine # whether the default arguments are valid. - parser = style.ArgumentParser(defaults) + parser = ArgumentParser(all_categories=self._all_categories(), + base_filter_rules=[], + default_options=default_options) # No need to test the return value here since we test parse() # on valid arguments elsewhere. - parser.parse([]) # arguments valid: no error or SystemExit + # + # The default options are valid: no error or SystemExit. + parser.parse(args=[]) def test_path_rules_specifier(self): - all_categories = style_categories() + all_categories = self._all_categories() for (sub_paths, path_rules) in PATH_RULES_SPECIFIER: - self.assertTrue(isinstance(path_rules, tuple), - "Checking: " + str(path_rules)) validate_filter_rules(path_rules, self._all_categories()) - # Try using the path specifier (as an "end-to-end" check). config = FilterConfiguration(path_specific=PATH_RULES_SPECIFIER) - self.assertTrue(config.should_check("xxx_any_category", - "xxx_non_matching_path")) - self.assertTrue(config.should_check("xxx_any_category", - "WebKitTools/WebKitAPITest/")) - self.assertFalse(config.should_check("build/include", - "WebKitTools/WebKitAPITest/")) - self.assertFalse(config.should_check("readability/naming", - "WebKit/qt/tests/qwebelement/tst_qwebelement.cpp")) + + def assertCheck(path, category): + """Assert that the given category should be checked.""" + message = ('Should check category "%s" for path "%s".' + % (category, path)) + self.assertTrue(config.should_check(category, path)) + + def assertNoCheck(path, category): + """Assert that the given category should not be checked.""" + message = ('Should not check category "%s" for path "%s".' + % (category, path)) + self.assertFalse(config.should_check(category, path), message) + + assertCheck("random_path.cpp", + "build/include") + assertNoCheck("WebKitTools/WebKitAPITest/main.cpp", + "build/include") + assertNoCheck("WebKit/qt/QGVLauncher/main.cpp", + "build/include") + assertNoCheck("WebKit/qt/QGVLauncher/main.cpp", + "readability/streams") + + assertCheck("random_path.cpp", + "readability/naming") + assertNoCheck("WebKit/gtk/webkit/webkit.h", + "readability/naming") + assertNoCheck("WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp", + "readability/null") + assertNoCheck("WebKit/efl/ewk/ewk_view.h", + "readability/naming") + assertNoCheck("WebCore/css/CSSParser.cpp", + "readability/naming") + assertNoCheck("WebKit/qt/tests/qwebelement/tst_qwebelement.cpp", + "readability/naming") + assertNoCheck( + "JavaScriptCore/qt/tests/qscriptengine/tst_qscriptengine.cpp", + "readability/naming") + assertNoCheck("WebCore/ForwardingHeaders/debugger/Debugger.h", + "build/header_guard") + + # Third-party Python code: webkitpy/thirdparty + path = "WebKitTools/Scripts/webkitpy/thirdparty/mock.py" + assertNoCheck(path, "build/include") + assertNoCheck(path, "pep8/E401") # A random pep8 category. + assertCheck(path, "pep8/W191") + assertCheck(path, "pep8/W291") + assertCheck(path, "whitespace/carriage_return") def test_max_reports_per_category(self): - """Check that MAX_REPORTS_PER_CATEGORY is valid.""" + """Check that _MAX_REPORTS_PER_CATEGORY is valid.""" all_categories = self._all_categories() - for category in style.MAX_REPORTS_PER_CATEGORY.iterkeys(): + for category in _MAX_REPORTS_PER_CATEGORY.iterkeys(): self.assertTrue(category in all_categories, 'Key "%s" is not a category' % category) -class ArgumentPrinterTest(unittest.TestCase): - - """Tests the ArgumentPrinter class.""" - - _printer = style.ArgumentPrinter() - - def _create_options(self, - output_format='emacs', - verbosity=3, - user_rules=[], - git_commit=None, - extra_flag_values={}): - filter_configuration = FilterConfiguration(user_rules=user_rules) - return style.ProcessorOptions(extra_flag_values=extra_flag_values, - filter_configuration=filter_configuration, - git_commit=git_commit, - output_format=output_format, - verbosity=verbosity) +class CheckWebKitStyleFunctionTest(unittest.TestCase): + + """Tests the functions with names of the form check_webkit_style_*.""" + + def test_check_webkit_style_configuration(self): + # Exercise the code path to make sure the function does not error out. + option_values = CommandOptionValues() + configuration = check_webkit_style_configuration(option_values) - def test_to_flag_string(self): - options = self._create_options('vs7', 5, ['+foo', '-bar'], 'git', - {'a': 0, 'z': 1}) - self.assertEquals('--a=0 --filter=+foo,-bar --git-commit=git ' - '--output=vs7 --verbose=5 --z=1', - self._printer.to_flag_string(options)) - - # This is to check that --filter and --git-commit do not - # show up when not user-specified. - options = self._create_options() - self.assertEquals('--output=emacs --verbose=3', - self._printer.to_flag_string(options)) - - -class ArgumentParserTest(unittest.TestCase): - - """Test the ArgumentParser class.""" - - def _parse(self): - """Return a default parse() function for testing.""" - return self._create_parser().parse - - def _create_defaults(self, default_output_format='vs7', - default_verbosity=3, - default_filter_rules=['-', '+whitespace']): - """Return a default ArgumentDefaults instance for testing.""" - return style.ArgumentDefaults(default_output_format, - default_verbosity, - default_filter_rules) - - def _create_parser(self, defaults=None): - """Return an ArgumentParser instance for testing.""" - def create_usage(_defaults): - """Return a usage string for testing.""" - return "usage" - - def doc_print(message): - # We do not want the usage string or style categories - # to print during unit tests, so print nothing. - return - - if defaults is None: - defaults = self._create_defaults() - - return style.ArgumentParser(defaults, create_usage, doc_print) - - def test_parse_documentation(self): - parse = self._parse() - - # FIXME: Test both the printing of the usage string and the - # filter categories help. - - # Request the usage string. - self.assertRaises(SystemExit, parse, ['--help']) - # Request default filter rules and available style categories. - self.assertRaises(SystemExit, parse, ['--filter=']) - - def test_parse_bad_values(self): - parse = self._parse() - - # Pass an unsupported argument. - self.assertRaises(SystemExit, parse, ['--bad']) - - self.assertRaises(ValueError, parse, ['--verbose=bad']) - self.assertRaises(ValueError, parse, ['--verbose=0']) - self.assertRaises(ValueError, parse, ['--verbose=6']) - parse(['--verbose=1']) # works - parse(['--verbose=5']) # works - - self.assertRaises(ValueError, parse, ['--output=bad']) - parse(['--output=vs7']) # works - - # Pass a filter rule not beginning with + or -. - self.assertRaises(ValueError, parse, ['--filter=build']) - parse(['--filter=+build']) # works - # Pass files and git-commit at the same time. - self.assertRaises(SystemExit, parse, ['--git-commit=', 'file.txt']) - # Pass an extra flag already supported. - self.assertRaises(ValueError, parse, [], ['filter=']) - parse([], ['extra=']) # works - # Pass an extra flag with typo. - self.assertRaises(SystemExit, parse, ['--extratypo='], ['extra=']) - parse(['--extra='], ['extra=']) # works - self.assertRaises(ValueError, parse, [], ['extra=', 'extra=']) - - - def test_parse_default_arguments(self): - parse = self._parse() - - (files, options) = parse([]) - - self.assertEquals(files, []) - - self.assertEquals(options.output_format, 'vs7') - self.assertEquals(options.verbosity, 3) - self.assertEquals(options.filter_configuration, - FilterConfiguration(base_rules=["-", "+whitespace"], - path_specific=PATH_RULES_SPECIFIER)) - self.assertEquals(options.git_commit, None) - - def test_parse_explicit_arguments(self): - parse = self._parse() - - # Pass non-default explicit values. - (files, options) = parse(['--output=emacs']) - self.assertEquals(options.output_format, 'emacs') - (files, options) = parse(['--verbose=4']) - self.assertEquals(options.verbosity, 4) - (files, options) = parse(['--git-commit=commit']) - self.assertEquals(options.git_commit, 'commit') + def test_check_webkit_style_parser(self): + # Exercise the code path to make sure the function does not error out. + parser = check_webkit_style_parser() - # Pass user_rules. - (files, options) = parse(['--filter=+build,-whitespace']) - config = options.filter_configuration - self.assertEquals(options.filter_configuration, - FilterConfiguration(base_rules=["-", "+whitespace"], - path_specific=PATH_RULES_SPECIFIER, - user_rules=["+build", "-whitespace"])) - - # Pass spurious white space in user rules. - (files, options) = parse(['--filter=+build, -whitespace']) - self.assertEquals(options.filter_configuration, - FilterConfiguration(base_rules=["-", "+whitespace"], - path_specific=PATH_RULES_SPECIFIER, - user_rules=["+build", "-whitespace"])) - - # Pass extra flag values. - (files, options) = parse(['--extra'], ['extra']) - self.assertEquals(options.extra_flag_values, {'--extra': ''}) - (files, options) = parse(['--extra='], ['extra=']) - self.assertEquals(options.extra_flag_values, {'--extra': ''}) - (files, options) = parse(['--extra=x'], ['extra=']) - self.assertEquals(options.extra_flag_values, {'--extra': 'x'}) - - def test_parse_files(self): - parse = self._parse() - - (files, options) = parse(['foo.cpp']) - self.assertEquals(files, ['foo.cpp']) - # Pass multiple files. - (files, options) = parse(['--output=emacs', 'foo.cpp', 'bar.cpp']) - self.assertEquals(files, ['foo.cpp', 'bar.cpp']) +class CheckerDispatcherSkipTest(unittest.TestCase): + """Tests the "should skip" methods of the CheckerDispatcher class.""" + + def setUp(self): + self._dispatcher = CheckerDispatcher() -class ProcessorDispatcherSkipTest(unittest.TestCase): - - """Tests the "should skip" methods of the ProcessorDispatcher class.""" - def test_should_skip_with_warning(self): """Test should_skip_with_warning().""" - dispatcher = ProcessorDispatcher() - # Check a non-skipped file. - self.assertFalse(dispatcher.should_skip_with_warning("foo.txt")) + self.assertFalse(self._dispatcher.should_skip_with_warning("foo.txt")) # Check skipped files. paths_to_skip = [ "gtk2drawing.c", - "gtk2drawing.h", + "gtkdrawing.h", "JavaScriptCore/qt/api/qscriptengine_p.h", "WebCore/platform/gtk/gtk2drawing.c", - "WebCore/platform/gtk/gtk2drawing.h", + "WebCore/platform/gtk/gtkdrawing.h", "WebKit/gtk/tests/testatk.c", "WebKit/qt/Api/qwebpage.h", "WebKit/qt/tests/qwebsecurityorigin/tst_qwebsecurityorigin.cpp", ] for path in paths_to_skip: - self.assertTrue(dispatcher.should_skip_with_warning(path), + self.assertTrue(self._dispatcher.should_skip_with_warning(path), "Checking: " + path) - def test_should_skip_without_warning(self): - """Test should_skip_without_warning().""" - dispatcher = ProcessorDispatcher() - - # Check a non-skipped file. - self.assertFalse(dispatcher.should_skip_without_warning("foo.txt")) - - # Check skipped files. - paths_to_skip = [ - # LayoutTests folder - "LayoutTests/foo.txt", - ] + def _assert_should_skip_without_warning(self, path, is_checker_none, + expected): + # Check the file type before asserting the return value. + checker = self._dispatcher.dispatch(file_path=path, + handle_style_error=None, + min_confidence=3) + message = 'while checking: %s' % path + self.assertEquals(checker is None, is_checker_none, message) + self.assertEquals(self._dispatcher.should_skip_without_warning(path), + expected, message) + + def test_should_skip_without_warning__true(self): + """Test should_skip_without_warning() for True return values.""" + # Check a file with NONE file type. + path = 'foo.asdf' # Non-sensical file extension. + self._assert_should_skip_without_warning(path, + is_checker_none=True, + expected=True) + + # Check files with non-NONE file type. These examples must be + # drawn from the _SKIPPED_FILES_WITHOUT_WARNING configuration + # variable. + path = os.path.join('LayoutTests', 'foo.txt') + self._assert_should_skip_without_warning(path, + is_checker_none=False, + expected=True) + + def test_should_skip_without_warning__false(self): + """Test should_skip_without_warning() for False return values.""" + paths = ['foo.txt', + os.path.join('LayoutTests', 'ChangeLog'), + ] - for path in paths_to_skip: - self.assertTrue(dispatcher.should_skip_without_warning(path), - "Checking: " + path) + for path in paths: + self._assert_should_skip_without_warning(path, + is_checker_none=False, + expected=False) -class ProcessorDispatcherDispatchTest(unittest.TestCase): +class CheckerDispatcherDispatchTest(unittest.TestCase): - """Tests dispatch_processor() method of ProcessorDispatcher class.""" + """Tests dispatch() method of CheckerDispatcher class.""" def mock_handle_style_error(self): pass - def dispatch_processor(self, file_path): - """Call dispatch_processor() with the given file path.""" - dispatcher = ProcessorDispatcher() - processor = dispatcher.dispatch_processor(file_path, - self.mock_handle_style_error, - verbosity=3) - return processor - - def assert_processor_none(self, file_path): - """Assert that the dispatched processor is None.""" - processor = self.dispatch_processor(file_path) - self.assertTrue(processor is None, 'Checking: "%s"' % file_path) - - def assert_processor(self, file_path, expected_class): - """Assert the type of the dispatched processor.""" - processor = self.dispatch_processor(file_path) - got_class = processor.__class__ + def dispatch(self, file_path): + """Call dispatch() with the given file path.""" + dispatcher = CheckerDispatcher() + checker = dispatcher.dispatch(file_path, + self.mock_handle_style_error, + min_confidence=3) + return checker + + def assert_checker_none(self, file_path): + """Assert that the dispatched checker is None.""" + checker = self.dispatch(file_path) + self.assertTrue(checker is None, 'Checking: "%s"' % file_path) + + def assert_checker(self, file_path, expected_class): + """Assert the type of the dispatched checker.""" + checker = self.dispatch(file_path) + got_class = checker.__class__ self.assertEquals(got_class, expected_class, 'For path "%(file_path)s" got %(got_class)s when ' "expecting %(expected_class)s." @@ -436,13 +364,17 @@ class ProcessorDispatcherDispatchTest(unittest.TestCase): "got_class": got_class, "expected_class": expected_class}) - def assert_processor_cpp(self, file_path): - """Assert that the dispatched processor is a CppProcessor.""" - self.assert_processor(file_path, CppProcessor) + def assert_checker_cpp(self, file_path): + """Assert that the dispatched checker is a CppChecker.""" + self.assert_checker(file_path, CppChecker) + + def assert_checker_python(self, file_path): + """Assert that the dispatched checker is a PythonChecker.""" + self.assert_checker(file_path, PythonChecker) - def assert_processor_text(self, file_path): - """Assert that the dispatched processor is a TextProcessor.""" - self.assert_processor(file_path, TextProcessor) + def assert_checker_text(self, file_path): + """Assert that the dispatched checker is a TextChecker.""" + self.assert_checker(file_path, TextChecker) def test_cpp_paths(self): """Test paths that should be checked as C++.""" @@ -454,228 +386,384 @@ class ProcessorDispatcherDispatchTest(unittest.TestCase): ] for path in paths: - self.assert_processor_cpp(path) + self.assert_checker_cpp(path) - # Check processor attributes on a typical input. + # Check checker attributes on a typical input. file_base = "foo" file_extension = "c" file_path = file_base + "." + file_extension - self.assert_processor_cpp(file_path) - processor = self.dispatch_processor(file_path) - self.assertEquals(processor.file_extension, file_extension) - self.assertEquals(processor.file_path, file_path) - self.assertEquals(processor.handle_style_error, self.mock_handle_style_error) - self.assertEquals(processor.verbosity, 3) + self.assert_checker_cpp(file_path) + checker = self.dispatch(file_path) + self.assertEquals(checker.file_extension, file_extension) + self.assertEquals(checker.file_path, file_path) + self.assertEquals(checker.handle_style_error, self.mock_handle_style_error) + self.assertEquals(checker.min_confidence, 3) # Check "-" for good measure. file_base = "-" file_extension = "" file_path = file_base - self.assert_processor_cpp(file_path) - processor = self.dispatch_processor(file_path) - self.assertEquals(processor.file_extension, file_extension) - self.assertEquals(processor.file_path, file_path) + self.assert_checker_cpp(file_path) + checker = self.dispatch(file_path) + self.assertEquals(checker.file_extension, file_extension) + self.assertEquals(checker.file_path, file_path) + + def test_python_paths(self): + """Test paths that should be checked as Python.""" + paths = [ + "foo.py", + "WebKitTools/Scripts/modules/text_style.py", + ] + + for path in paths: + self.assert_checker_python(path) + + # Check checker attributes on a typical input. + file_base = "foo" + file_extension = "css" + file_path = file_base + "." + file_extension + self.assert_checker_text(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_text_paths(self): """Test paths that should be checked as text.""" paths = [ "ChangeLog", + "ChangeLog-2009-06-16", + "foo.ac", + "foo.cc", + "foo.cgi", "foo.css", + "foo.exp", + "foo.flex", + "foo.gyp", + "foo.gypi", "foo.html", "foo.idl", + "foo.in", "foo.js", "foo.mm", "foo.php", + "foo.pl", "foo.pm", - "foo.py", + "foo.pri", + "foo.pro", + "foo.rb", + "foo.sh", "foo.txt", - "FooChangeLog.bak", - "WebCore/ChangeLog", - "WebCore/inspector/front-end/inspector.js", - "WebKitTools/Scripts/check-webkit=style", - "WebKitTools/Scripts/modules/text_style.py", - ] + "foo.wm", + "foo.xhtml", + "foo.y", + os.path.join("WebCore", "ChangeLog"), + os.path.join("WebCore", "inspector", "front-end", "inspector.js"), + os.path.join("WebKitTools", "Scripts", "check-webkit-style"), + ] for path in paths: - self.assert_processor_text(path) + self.assert_checker_text(path) - # Check processor attributes on a typical input. + # Check checker attributes on a typical input. file_base = "foo" file_extension = "css" file_path = file_base + "." + file_extension - self.assert_processor_text(file_path) - processor = self.dispatch_processor(file_path) - self.assertEquals(processor.file_path, file_path) - self.assertEquals(processor.handle_style_error, self.mock_handle_style_error) + self.assert_checker_text(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 = [ "Makefile", + "foo.asdf", # Non-sensical file extension. "foo.png", "foo.exe", + "foo.vcproj", ] for path in paths: - self.assert_processor_none(path) + self.assert_checker_none(path) -class StyleCheckerTest(unittest.TestCase): +class StyleProcessorConfigurationTest(unittest.TestCase): - """Test the StyleChecker class. + """Tests the StyleProcessorConfiguration class.""" - Attributes: - error_messages: A string containing all of the warning messages - written to the mock_stderr_write method of - this class. - - """ + def setUp(self): + self._error_messages = [] + """The messages written to _mock_stderr_write() of this class.""" def _mock_stderr_write(self, message): - pass + self._error_messages.append(message) + + def _style_checker_configuration(self, output_format="vs7"): + """Return a StyleProcessorConfiguration instance for testing.""" + base_rules = ["-whitespace", "+whitespace/tab"] + filter_configuration = FilterConfiguration(base_rules=base_rules) - def _style_checker(self, options): - return StyleChecker(options, self._mock_stderr_write) + return StyleProcessorConfiguration( + filter_configuration=filter_configuration, + max_reports_per_category={"whitespace/newline": 1}, + min_confidence=3, + output_format=output_format, + stderr_write=self._mock_stderr_write) + + def test_init(self): + """Test the __init__() method.""" + configuration = self._style_checker_configuration() + + # Check that __init__ sets the "public" data attributes correctly. + self.assertEquals(configuration.max_reports_per_category, + {"whitespace/newline": 1}) + self.assertEquals(configuration.stderr_write, self._mock_stderr_write) + self.assertEquals(configuration.min_confidence, 3) + + def test_is_reportable(self): + """Test the is_reportable() method.""" + config = self._style_checker_configuration() + + self.assertTrue(config.is_reportable("whitespace/tab", 3, "foo.txt")) + + # Test the confidence check code path by varying the confidence. + self.assertFalse(config.is_reportable("whitespace/tab", 2, "foo.txt")) + + # Test the category check code path by varying the category. + self.assertFalse(config.is_reportable("whitespace/line", 4, "foo.txt")) + + def _call_write_style_error(self, output_format): + config = self._style_checker_configuration(output_format=output_format) + config.write_style_error(category="whitespace/tab", + confidence_in_error=5, + file_path="foo.h", + line_number=100, + message="message") + + def test_write_style_error_emacs(self): + """Test the write_style_error() method.""" + self._call_write_style_error("emacs") + self.assertEquals(self._error_messages, + ["foo.h:100: message [whitespace/tab] [5]\n"]) + + def test_write_style_error_vs7(self): + """Test the write_style_error() method.""" + self._call_write_style_error("vs7") + self.assertEquals(self._error_messages, + ["foo.h(100): message [whitespace/tab] [5]\n"]) + + +class StyleProcessor_EndToEndTest(LoggingTestCase): + + """Test the StyleProcessor class with an emphasis on end-to-end tests.""" + + def setUp(self): + LoggingTestCase.setUp(self) + self._messages = [] + + def _mock_stderr_write(self, message): + """Save a message so it can later be asserted.""" + self._messages.append(message) def test_init(self): """Test __init__ constructor.""" - options = ProcessorOptions() - style_checker = self._style_checker(options) + configuration = StyleProcessorConfiguration( + filter_configuration=FilterConfiguration(), + max_reports_per_category={}, + min_confidence=3, + output_format="vs7", + stderr_write=self._mock_stderr_write) + processor = StyleProcessor(configuration) - self.assertEquals(style_checker.error_count, 0) - self.assertEquals(style_checker.options, options) + self.assertEquals(processor.error_count, 0) + self.assertEquals(self._messages, []) + def test_process(self): + configuration = StyleProcessorConfiguration( + filter_configuration=FilterConfiguration(), + max_reports_per_category={}, + min_confidence=3, + output_format="vs7", + stderr_write=self._mock_stderr_write) + processor = StyleProcessor(configuration) -class StyleCheckerCheckFileTest(unittest.TestCase): + processor.process(lines=['line1', 'Line with tab:\t'], + file_path='foo.txt') + self.assertEquals(processor.error_count, 1) + expected_messages = ['foo.txt(2): Line contains tab character. ' + '[whitespace/tab] [5]\n'] + self.assertEquals(self._messages, expected_messages) - """Test the check_file() method of the StyleChecker class. - The check_file() method calls its process_file parameter when - given a file that should not be skipped. +class StyleProcessor_CodeCoverageTest(LoggingTestCase): - The "got_*" attributes of this class are the parameters passed - to process_file by calls to check_file() made by this test - class. These attributes allow us to check the parameter values - passed internally to the process_file function. + """Test the StyleProcessor class with an emphasis on code coverage. - Attributes: - got_file_path: The file_path parameter passed by check_file() - to its process_file parameter. - got_handle_style_error: The handle_style_error parameter passed - by check_file() to its process_file - parameter. - got_processor: The processor parameter passed by check_file() to - its process_file parameter. - warning_messages: A string containing all of the warning messages - written to the mock_stderr_write method of - this class. + This class makes heavy use of mock objects. """ - def setUp(self): - self.got_file_path = None - self.got_handle_style_error = None - self.got_processor = None - self.warning_messages = "" - def mock_stderr_write(self, warning_message): - self.warning_messages += warning_message + class MockDispatchedChecker(object): - def mock_handle_style_error(self): + """A mock checker dispatched by the MockDispatcher.""" + + def __init__(self, file_path, min_confidence, style_error_handler): + self.file_path = file_path + self.min_confidence = min_confidence + self.style_error_handler = style_error_handler + + def check(self, lines): + self.lines = lines + + class MockDispatcher(object): + + """A mock CheckerDispatcher class.""" + + def __init__(self): + self.dispatched_checker = None + + def should_skip_with_warning(self, file_path): + return file_path.endswith('skip_with_warning.txt') + + def should_skip_without_warning(self, file_path): + return file_path.endswith('skip_without_warning.txt') + + def dispatch(self, file_path, style_error_handler, min_confidence): + if file_path.endswith('do_not_process.txt'): + return None + + checker = StyleProcessor_CodeCoverageTest.MockDispatchedChecker( + file_path, + min_confidence, + style_error_handler) + + # Save the dispatched checker so the current test case has a + # way to access and check it. + self.dispatched_checker = checker + + return checker + + def setUp(self): + LoggingTestCase.setUp(self) + # We can pass an error-message swallower here because error message + # output is tested instead in the end-to-end test case above. + configuration = StyleProcessorConfiguration( + filter_configuration=FilterConfiguration(), + max_reports_per_category={"whitespace/newline": 1}, + min_confidence=3, + output_format="vs7", + stderr_write=self._swallow_stderr_message) + + mock_carriage_checker_class = self._create_carriage_checker_class() + mock_dispatcher = self.MockDispatcher() + # We do not need to use a real incrementer here because error-count + # incrementing is tested instead in the end-to-end test case above. + mock_increment_error_count = self._do_nothing + + processor = StyleProcessor(configuration=configuration, + mock_carriage_checker_class=mock_carriage_checker_class, + mock_dispatcher=mock_dispatcher, + mock_increment_error_count=mock_increment_error_count) + + self._configuration = configuration + self._mock_dispatcher = mock_dispatcher + self._processor = processor + + def _do_nothing(self): + # We provide this function so the caller can pass it to the + # StyleProcessor constructor. This lets us assert the equality of + # the DefaultStyleErrorHandler instance generated by the process() + # method with an expected instance. pass - def mock_process_file(self, processor, file_path, handle_style_error): - """A mock _process_file(). + def _swallow_stderr_message(self, message): + """Swallow a message passed to stderr.write().""" + # This is a mock stderr.write() for passing to the constructor + # of the StyleProcessorConfiguration class. + pass - See the documentation for this class for more information - on this function. + def _create_carriage_checker_class(self): - """ - self.got_file_path = file_path - self.got_handle_style_error = handle_style_error - self.got_processor = processor - - def assert_attributes(self, - expected_file_path, - expected_handle_style_error, - expected_processor, - expected_warning_messages): - """Assert that the attributes of this class equal the given values.""" - self.assertEquals(self.got_file_path, expected_file_path) - self.assertEquals(self.got_handle_style_error, expected_handle_style_error) - self.assertEquals(self.got_processor, expected_processor) - self.assertEquals(self.warning_messages, expected_warning_messages) - - def call_check_file(self, file_path): - """Call the check_file() method of a test StyleChecker instance.""" - # Confirm that the attributes are reset. - self.assert_attributes(None, None, None, "") - - # Create a test StyleChecker instance. - # - # The verbosity attribute is the only ProcessorOptions - # attribute that needs to be checked in this test. - # This is because it is the only option is directly - # passed to the constructor of a style processor. - options = ProcessorOptions(verbosity=3) + # Create a reference to self with a new name so its name does not + # conflict with the self introduced below. + test_case = self - style_checker = StyleChecker(options, self.mock_stderr_write) + class MockCarriageChecker(object): - style_checker.check_file(file_path, - self.mock_handle_style_error, - self.mock_process_file) + """A mock carriage-return checker.""" - def test_check_file_on_skip_without_warning(self): - """Test check_file() for a skipped-without-warning file.""" + def __init__(self, style_error_handler): + self.style_error_handler = style_error_handler - file_path = "LayoutTests/foo.txt" + # This gives the current test case access to the + # instantiated carriage checker. + test_case.carriage_checker = self - dispatcher = ProcessorDispatcher() - # Confirm that the input file is truly a skipped-without-warning file. - self.assertTrue(dispatcher.should_skip_without_warning(file_path)) + def check(self, lines): + # Save the lines so the current test case has a way to access + # and check them. + self.lines = lines - # Check the outcome. - self.call_check_file(file_path) - self.assert_attributes(None, None, None, "") + return lines - def test_check_file_on_skip_with_warning(self): - """Test check_file() for a skipped-with-warning file.""" + return MockCarriageChecker - file_path = "gtk2drawing.c" + def test_should_process__skip_without_warning(self): + """Test should_process() for a skip-without-warning file.""" + file_path = "foo/skip_without_warning.txt" - dispatcher = ProcessorDispatcher() - # Check that the input file is truly a skipped-with-warning file. - self.assertTrue(dispatcher.should_skip_with_warning(file_path)) + self.assertFalse(self._processor.should_process(file_path)) - # 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') + def test_should_process__skip_with_warning(self): + """Test should_process() for a skip-with-warning file.""" + file_path = "foo/skip_with_warning.txt" - def test_check_file_on_non_skipped(self): + self.assertFalse(self._processor.should_process(file_path)) - # We use a C++ file since by using a CppProcessor, we can check - # that all of the possible information is getting passed to - # process_file (in particular, the verbosity). - file_base = "foo" - file_extension = "cpp" - file_path = file_base + "." + file_extension + self.assertLog(['WARNING: File exempt from style guide. ' + 'Skipping: "foo/skip_with_warning.txt"\n']) + + def test_should_process__true_result(self): + """Test should_process() for a file that should be processed.""" + file_path = "foo/skip_process.txt" - dispatcher = ProcessorDispatcher() - # Check that the input file is truly a C++ file. - self.assertEquals(dispatcher._file_type(file_path), style.FileType.CPP) + self.assertTrue(self._processor.should_process(file_path)) - # Check the outcome. - self.call_check_file(file_path) + def test_process__checker_dispatched(self): + """Test the process() method for a path with a dispatched checker.""" + file_path = 'foo.txt' + lines = ['line1', 'line2'] + line_numbers = [100] - expected_processor = CppProcessor(file_path, file_extension, self.mock_handle_style_error, 3) + expected_error_handler = DefaultStyleErrorHandler( + configuration=self._configuration, + file_path=file_path, + increment_error_count=self._do_nothing, + line_numbers=line_numbers) - self.assert_attributes(file_path, - self.mock_handle_style_error, - expected_processor, - "") + self._processor.process(lines=lines, + file_path=file_path, + line_numbers=line_numbers) + # Check that the carriage-return checker was instantiated correctly + # and was passed lines correctly. + carriage_checker = self.carriage_checker + self.assertEquals(carriage_checker.style_error_handler, + expected_error_handler) + self.assertEquals(carriage_checker.lines, ['line1', 'line2']) -if __name__ == '__main__': - import sys + # Check that the style checker was dispatched correctly and was + # passed lines correctly. + checker = self._mock_dispatcher.dispatched_checker + self.assertEquals(checker.file_path, 'foo.txt') + self.assertEquals(checker.min_confidence, 3) + self.assertEquals(checker.style_error_handler, expected_error_handler) - unittest.main() + self.assertEquals(checker.lines, ['line1', 'line2']) + def test_process__no_checker_dispatched(self): + """Test the process() method for a path with no dispatched checker.""" + path = os.path.join('foo', 'do_not_process.txt') + self.assertRaises(AssertionError, self._processor.process, + lines=['line1', 'line2'], file_path=path, + line_numbers=[100]) diff --git a/WebKitTools/Scripts/webkitpy/style/processors/__init__.py b/WebKitTools/Scripts/webkitpy/style/checkers/__init__.py index ef65bee..ef65bee 100644 --- a/WebKitTools/Scripts/webkitpy/style/processors/__init__.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/__init__.py diff --git a/WebKitTools/Scripts/webkitpy/style/processors/common.py b/WebKitTools/Scripts/webkitpy/style/checkers/common.py index dbf4bea..a2d933f 100644 --- a/WebKitTools/Scripts/webkitpy/style/processors/common.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/common.py @@ -20,10 +20,10 @@ # (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 style checking not specific to any one processor.""" +"""Supports style checking not specific to any one file type.""" -# FIXME: Test this list in the same way that the list of CppProcessor +# FIXME: Test this list in the same way that the list of CppChecker # categories is tested, for example by checking that all of its # elements appear in the unit tests. This should probably be done # after moving the relevant cpp_unittest.ErrorCollector code @@ -33,27 +33,25 @@ categories = set([ ]) -def check_no_carriage_return(line, line_number, error): - """Check that a line does not end with a carriage return. +class CarriageReturnChecker(object): - Returns true if the check is successful (i.e. if the line does not - end with a carriage return), and false otherwise. + """Supports checking for and handling carriage returns.""" - Args: - line: A string that is the line to check. - line_number: The line number. - error: The function to call with any errors found. + def __init__(self, handle_style_error): + self._handle_style_error = handle_style_error - """ + def check(self, lines): + """Check for and strip trailing carriage returns from lines.""" + for line_number in range(len(lines)): + if not lines[line_number].endswith("\r"): + continue - if line.endswith("\r"): - error(line_number, - "whitespace/carriage_return", - 1, - "One or more unexpected \\r (^M) found; " - "better to use only a \\n") - return False - - return True + self._handle_style_error(line_number + 1, # Correct for offset. + "whitespace/carriage_return", + 1, + "One or more unexpected \\r (^M) found; " + "better to use only a \\n") + lines[line_number] = lines[line_number].rstrip("\r") + return lines diff --git a/WebKitTools/Scripts/webkitpy/style/processors/common_unittest.py b/WebKitTools/Scripts/webkitpy/style/checkers/common_unittest.py index 9362b65..b67b7b0 100644 --- a/WebKitTools/Scripts/webkitpy/style/processors/common_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/common_unittest.py @@ -22,24 +22,25 @@ """Unit tests for common.py.""" - import unittest -from common import check_no_carriage_return +from common import CarriageReturnChecker -# FIXME: The unit tests for the cpp, text, and common processors should +# FIXME: The unit tests for the cpp, text, and common checkers should # share supporting test code. This can include, for example, the # mock style error handling code and the code to check that all -# of a processor's categories are covered by the unit tests. +# of a checker's categories are covered by the unit tests. # Such shared code can be located in a shared test file, perhaps -# ilke this one. -class CarriageReturnTest(unittest.TestCase): +# even this file. +class CarriageReturnCheckerTest(unittest.TestCase): """Tests check_no_carriage_return().""" _category = "whitespace/carriage_return" _confidence = 1 + _expected_message = ("One or more unexpected \\r (^M) found; " + "better to use only a \\n") def setUp(self): self._style_errors = [] # The list of accumulated style errors. @@ -50,33 +51,44 @@ class CarriageReturnTest(unittest.TestCase): error = (line_number, category, confidence, message) self._style_errors.append(error) - def assert_carriage_return(self, line, is_error): - """Call check_no_carriage_return() and assert the result.""" - line_number = 100 + def assert_carriage_return(self, input_lines, expected_lines, error_lines): + """Process the given line and assert that the result is correct.""" handle_style_error = self._mock_style_error_handler - check_no_carriage_return(line, line_number, handle_style_error) + checker = CarriageReturnChecker(handle_style_error) + output_lines = checker.check(input_lines) - expected_message = ("One or more unexpected \\r (^M) found; " - "better to use only a \\n") + # Check both the return value and error messages. + self.assertEquals(output_lines, expected_lines) - if is_error: - expected_errors = [(line_number, self._category, self._confidence, - expected_message)] - self.assertEquals(self._style_errors, expected_errors) - else: - self.assertEquals(self._style_errors, []) + expected_errors = [(line_number, self._category, self._confidence, + self._expected_message) + for line_number in error_lines] + self.assertEquals(self._style_errors, expected_errors) def test_ends_with_carriage(self): - self.assert_carriage_return("carriage return\r", is_error=True) + self.assert_carriage_return(["carriage return\r"], + ["carriage return"], + [1]) def test_ends_with_nothing(self): - self.assert_carriage_return("no carriage return", is_error=False) + self.assert_carriage_return(["no carriage return"], + ["no carriage return"], + []) def test_ends_with_newline(self): - self.assert_carriage_return("no carriage return\n", is_error=False) - - def test_ends_with_carriage_newline(self): - # Check_no_carriage_return only() checks the final character. - self.assert_carriage_return("carriage\r in a string", is_error=False) - + self.assert_carriage_return(["no carriage return\n"], + ["no carriage return\n"], + []) + + def test_carriage_in_middle(self): + # The CarriageReturnChecker checks only the final character + # of each line. + self.assert_carriage_return(["carriage\r in a string"], + ["carriage\r in a string"], + []) + + def test_multiple_errors(self): + self.assert_carriage_return(["line1", "line2\r", "line3\r"], + ["line1", "line2", "line3"], + [2, 3]) diff --git a/WebKitTools/Scripts/webkitpy/style/processors/cpp.py b/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py index 182c967..770ab40 100644 --- a/WebKitTools/Scripts/webkitpy/style/processors/cpp.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/cpp.py @@ -161,7 +161,7 @@ def up_to_unmatched_closing_paren(s): Returns: A pair of strings (prefix before first unmatched ')', - reminder of s after first unmatched ')'), e.g., + remainder of s after first unmatched ')'), e.g., up_to_unmatched_closing_paren("a == (b + c)) { ") returns "a == (b + c)", " {". Returns None, None if there is no unmatched ')' @@ -272,18 +272,18 @@ class _FunctionState(object): """Tracks current function name and the number of lines in its body. Attributes: - verbosity: The verbosity level to use while checking style. + min_confidence: The minimum confidence level to use while checking style. """ _NORMAL_TRIGGER = 250 # for --v=0, 500 for --v=1, etc. _TEST_TRIGGER = 400 # about 50% more than _NORMAL_TRIGGER. - def __init__(self, verbosity): - self.verbosity = verbosity + def __init__(self, min_confidence): + self.min_confidence = min_confidence + self.current_function = '' self.in_a_function = False self.lines_in_function = 0 - self.current_function = '' def begin(self, function_name): """Start analyzing function body. @@ -311,7 +311,7 @@ class _FunctionState(object): base_trigger = self._TEST_TRIGGER else: base_trigger = self._NORMAL_TRIGGER - trigger = base_trigger * 2 ** self.verbosity + trigger = base_trigger * 2 ** self.min_confidence if self.lines_in_function > trigger: error_level = int(math.log(self.lines_in_function / base_trigger, 2)) @@ -642,6 +642,10 @@ def get_header_guard_cpp_variable(filename): """ + # Restores original filename in case that style checker is invoked from Emacs's + # flymake. + filename = re.sub(r'_flymake\.h$', '.h', filename) + return sub(r'[-.\s]', '_', os.path.basename(filename)) @@ -1333,26 +1337,25 @@ def check_spacing(file_extension, clean_lines, line_number, error): # there should either be zero or one spaces inside the parens. # We don't want: "if ( foo)" or "if ( foo )". # Exception: "for ( ; foo; bar)" and "for (foo; bar; )" are allowed. - matched = search(r'\b(?P<statement>if|for|foreach|while|switch)\s*\((?P<reminder>.*)$', line) + matched = search(r'\b(?P<statement>if|for|foreach|while|switch)\s*\((?P<remainder>.*)$', line) if matched: statement = matched.group('statement') - condition, rest = up_to_unmatched_closing_paren(matched.group('reminder')) + condition, rest = up_to_unmatched_closing_paren(matched.group('remainder')) if condition is not None: condition_match = search(r'(?P<leading>[ ]*)(?P<separator>.).*[^ ]+(?P<trailing>[ ]*)', condition) if condition_match: n_leading = len(condition_match.group('leading')) n_trailing = len(condition_match.group('trailing')) - if n_leading != n_trailing: - for_exception = statement == 'for' and ( - (condition.startswith(' ;') and n_trailing == 0) or - (condition.endswith('; ') and n_leading == 0)) + if n_leading != 0: + for_exception = statement == 'for' and condition.startswith(' ;') + if not for_exception: + error(line_number, 'whitespace/parens', 5, + 'Extra space after ( in %s' % statement) + if n_trailing != 0: + for_exception = statement == 'for' and condition.endswith('; ') if not for_exception: error(line_number, 'whitespace/parens', 5, - 'Mismatching spaces inside () in %s' % statement) - if n_leading > 1: - error(line_number, 'whitespace/parens', 5, - 'Should have zero or one spaces inside ( and ) in %s' % - statement) + 'Extra space before ) in %s' % statement) # Do not check for more than one command in macros in_macro = match(r'\s*#define', line) @@ -1365,9 +1368,14 @@ def check_spacing(file_extension, clean_lines, line_number, error): error(line_number, 'whitespace/comma', 3, 'Missing space after ,') + matched = search(r'^\s*(?P<token1>[a-zA-Z0-9_\*&]+)\s\s+(?P<token2>[a-zA-Z0-9_\*&]+)', line) + if matched: + error(line_number, 'whitespace/declaration', 3, + 'Extra space between %s and %s' % (matched.group('token1'), matched.group('token2'))) + if file_extension == 'cpp': # C++ should have the & or * beside the type not the variable name. - matched = match(r'\s*\w+(?<!\breturn)\s+(?P<pointer_operator>\*|\&)\w+', line) + matched = match(r'\s*\w+(?<!\breturn|\bdelete)\s+(?P<pointer_operator>\*|\&)\w+', line) if matched: error(line_number, 'whitespace/declaration', 3, 'Declaration has space between type name and %s in %s' % (matched.group('pointer_operator'), matched.group(0).strip())) @@ -1652,7 +1660,7 @@ def check_braces(clean_lines, line_number, error): # We check if a closed brace has started a line to see if a # one line control statement was previous. previous_line = clean_lines.elided[line_number - 2] - if (previous_line.find('{') > 0 + if (previous_line.find('{') > 0 and previous_line.find('}') < 0 and search(r'\b(if|for|foreach|while|else)\b', previous_line)): error(line_number, 'whitespace/braces', 4, 'One line control clauses should not use braces.') @@ -1864,8 +1872,20 @@ def check_for_null(file_extension, clean_lines, line_number, error): line = clean_lines.elided[line_number] - # Don't warn about NULL usage in g_object_{get,set}(). See Bug 32858 - if search(r'\bg_object_[sg]et\b', line): + # Don't warn about NULL usage in g_*(). See Bug 32858 and 39372. + if search(r'\bg(_[a-z]+)+\b', line): + return + + # Don't warn about NULL usage in gst_*_many(). See Bug 39740 + if search(r'\bgst_\w+_many\b', line): + return + + # Don't warn about NULL usage in g_str{join,concat}(). See Bug 34834 + if search(r'\bg_str(join|concat)\b', line): + return + + # Don't warn about NULL usage in gdk_pixbuf_save_to_*{join,concat}(). See Bug 43090. + if search(r'\bgdk_pixbuf_save_to\w+\b', line): return if search(r'\bNULL\b', line): @@ -1900,7 +1920,7 @@ def get_line_width(line): return len(line) -def check_style(clean_lines, line_number, file_extension, file_state, error): +def check_style(clean_lines, line_number, file_extension, class_state, file_state, error): """Checks rules from the 'C++ style rules' section of cppguide.html. Most of these rules are hard to test (naming, comment style), but we @@ -1911,6 +1931,8 @@ def check_style(clean_lines, line_number, file_extension, file_state, error): clean_lines: A CleansedLines instance containing the file. line_number: The number of the line to check. file_extension: The extension (without the dot) of the filename. + class_state: A _ClassState instance which maintains information about + the current stack of nested class declarations being parsed. file_state: A _FileState instance which maintains information about the state of things in the file. error: The function to call with any errors found. @@ -1971,6 +1993,10 @@ def check_style(clean_lines, line_number, file_extension, file_state, error): and not ((cleansed_line.find('case ') != -1 or cleansed_line.find('default:') != -1) and cleansed_line.find('break;') != -1) + # Also it's ok to have many commands in trivial single-line accessors in class definitions. + and not (match(r'.*\(.*\).*{.*.}', line) + and class_state.classinfo_stack + and line.count('{') == line.count('}')) and not cleansed_line.startswith('#define ')): error(line_number, 'whitespace/newline', 4, 'More than one command on the same line') @@ -2421,7 +2447,9 @@ def check_identifier_name_in_declaration(filename, line_number, line, error): # Convert "long long", "long double", and "long long int" to # simple types, but don't remove simple "long". line = sub(r'long (long )?(?=long|double|int)', '', line) - line = sub(r'\b(unsigned|signed|inline|using|static|const|volatile|auto|register|extern|typedef|restrict|struct|class|virtual)(?=\W)', '', line) + # Convert unsigned/signed types to simple types, too. + line = sub(r'(unsigned|signed) (?=char|short|int|long)', '', line) + line = sub(r'\b(inline|using|static|const|volatile|auto|register|extern|typedef|restrict|struct|class|virtual)(?=\W)', '', line) # Remove all template parameters by removing matching < and >. # Loop until no templates are removed to remove nested templates. @@ -2449,8 +2477,9 @@ def check_identifier_name_in_declaration(filename, line_number, line, error): # Detect variable and functions. type_regexp = r'\w([\w]|\s*[*&]\s*|::)+' identifier_regexp = r'(?P<identifier>[\w:]+)' + maybe_bitfield_regexp = r'(:\s*\d+\s*)?' character_after_identifier_regexp = r'(?P<character_after_identifier>[[;()=,])(?!=)' - declaration_without_type_regexp = r'\s*' + identifier_regexp + r'\s*' + character_after_identifier_regexp + declaration_without_type_regexp = r'\s*' + identifier_regexp + r'\s*' + maybe_bitfield_regexp + character_after_identifier_regexp declaration_with_type_regexp = r'\s*' + type_regexp + r'\s' + declaration_without_type_regexp is_function_arguments = False number_of_identifiers = 0 @@ -2488,6 +2517,10 @@ def check_identifier_name_in_declaration(filename, line_number, line, error): and not modified_identifier == "const_iterator"): error(line_number, 'readability/naming', 4, identifier + " is incorrectly named. Don't use underscores in your identifier names.") + # Check for variables named 'l', these are too easy to confuse with '1' in some fonts + if modified_identifier == 'l': + error(line_number, 'readability/naming', 4, identifier + " is incorrectly named. Don't use the single letter 'l' as an identifier name.") + # There can be only one declaration in non-for-control statements. if control_statement: return @@ -2500,7 +2533,6 @@ def check_identifier_name_in_declaration(filename, line_number, line, error): number_of_identifiers += 1 line = line[matched.end():] - def check_c_style_cast(line_number, line, raw_line, cast_type, pattern, error): """Checks for a C-style cast by looking for the pattern. @@ -2762,9 +2794,7 @@ def check_for_include_what_you_use(filename, clean_lines, include_state, error, # found. # e.g. If the file name is 'foo_flymake.cpp', we should search for 'foo.h' # instead of 'foo_flymake.h' - emacs_flymake_suffix = '_flymake.cpp' - if abs_filename.endswith(emacs_flymake_suffix): - abs_filename = abs_filename[:-len(emacs_flymake_suffix)] + '.cpp' + abs_filename = re.sub(r'_flymake\.cpp$', '.cpp', abs_filename) # include_state is modified during iteration, so we iterate over a copy of # the keys. @@ -2821,7 +2851,7 @@ def process_line(filename, file_extension, if search(r'\bNOLINT\b', raw_lines[line]): # ignore nolint lines return check_for_multiline_comments_and_strings(clean_lines, line, error) - check_style(clean_lines, line, file_extension, file_state, error) + check_style(clean_lines, line, file_extension, class_state, file_state, error) check_language(filename, clean_lines, line, file_extension, include_state, error) check_for_non_standard_constructs(clean_lines, line, class_state, error) @@ -2829,7 +2859,7 @@ def process_line(filename, file_extension, check_invalid_increment(clean_lines, line, error) -def _process_lines(filename, file_extension, lines, error, verbosity): +def _process_lines(filename, file_extension, lines, error, min_confidence): """Performs lint checks and reports any errors to the given error function. Args: @@ -2843,7 +2873,7 @@ def _process_lines(filename, file_extension, lines, error, verbosity): ['// marker so line numbers end in a known way']) include_state = _IncludeState() - function_state = _FunctionState(verbosity) + function_state = _FunctionState(min_confidence) class_state = _ClassState() file_state = _FileState() @@ -2868,7 +2898,7 @@ def _process_lines(filename, file_extension, lines, error, verbosity): check_for_new_line_at_eof(lines, error) -class CppProcessor(object): +class CppChecker(object): """Processes C++ lines for checking style.""" @@ -2941,8 +2971,9 @@ class CppProcessor(object): 'whitespace/todo', ]) - def __init__(self, file_path, file_extension, handle_style_error, verbosity): - """Create a CppProcessor instance. + def __init__(self, file_path, file_extension, handle_style_error, + min_confidence): + """Create a CppChecker instance. Args: file_extension: A string that is the file extension, without @@ -2952,18 +2983,18 @@ class CppProcessor(object): self.file_extension = file_extension self.file_path = file_path self.handle_style_error = handle_style_error - self.verbosity = verbosity + self.min_confidence = min_confidence # Useful for unit testing. def __eq__(self, other): - """Return whether this CppProcessor instance is equal to another.""" + """Return whether this CppChecker instance is equal to another.""" if self.file_extension != other.file_extension: return False if self.file_path != other.file_path: return False if self.handle_style_error != other.handle_style_error: return False - if self.verbosity != other.verbosity: + if self.min_confidence != other.min_confidence: return False return True @@ -2973,13 +3004,12 @@ class CppProcessor(object): # Python does not automatically deduce __ne__() from __eq__(). return not self.__eq__(other) - def process(self, lines): + def check(self, lines): _process_lines(self.file_path, self.file_extension, lines, - self.handle_style_error, self.verbosity) + self.handle_style_error, self.min_confidence) # FIXME: Remove this function (requires refactoring unit tests). -def process_file_data(filename, file_extension, lines, error, verbosity): - processor = CppProcessor(filename, file_extension, error, verbosity) - processor.process(lines) - +def process_file_data(filename, file_extension, lines, error, min_confidence): + checker = CppChecker(filename, file_extension, error, min_confidence) + checker.check(lines) diff --git a/WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py b/WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py index fb5a487..ee829aa 100644 --- a/WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/cpp_unittest.py @@ -42,13 +42,13 @@ import random import re import unittest import cpp as cpp_style -from cpp import CppProcessor +from cpp import CppChecker # This class works as an error collector and replaces cpp_style.Error # function for the unit tests. We also verify each category we see # is in STYLE_CATEGORIES, to help keep that list up to date. class ErrorCollector: - _all_style_categories = CppProcessor.categories + _all_style_categories = CppChecker.categories # This is a list including all categories seen in any unit test. _seen_style_categories = {} @@ -119,20 +119,21 @@ class CppStyleTestBase(unittest.TestCase): """Provides some useful helper functions for cpp_style tests. Attributes: - verbosity: An integer that is the current verbosity level for - the tests. + min_confidence: An integer that is the current minimum confidence + level for the tests. """ - # FIXME: Refactor the unit tests so the verbosity level is passed + # FIXME: Refactor the unit tests so the confidence level is passed # explicitly, just like it is in the real code. - verbosity = 1; + min_confidence = 1; - # Helper function to avoid needing to explicitly pass verbosity + # Helper function to avoid needing to explicitly pass confidence # in all the unit test calls to cpp_style.process_file_data(). def process_file_data(self, filename, file_extension, lines, error): - """Call cpp_style.process_file_data() with the current verbosity.""" - return cpp_style.process_file_data(filename, file_extension, lines, error, self.verbosity) + """Call cpp_style.process_file_data() with the min_confidence.""" + return cpp_style.process_file_data(filename, file_extension, lines, + error, self.min_confidence) # Perform lint on single line of input and return the error message. def perform_single_line_lint(self, code, file_name): @@ -141,7 +142,7 @@ class CppStyleTestBase(unittest.TestCase): cpp_style.remove_multi_line_comments(lines, error_collector) clean_lines = cpp_style.CleansedLines(lines) include_state = cpp_style._IncludeState() - function_state = cpp_style._FunctionState(self.verbosity) + function_state = cpp_style._FunctionState(self.min_confidence) ext = file_name[file_name.rfind('.') + 1:] class_state = cpp_style._ClassState() file_state = cpp_style._FileState() @@ -163,7 +164,7 @@ class CppStyleTestBase(unittest.TestCase): class_state = cpp_style._ClassState() file_state = cpp_style._FileState() for i in xrange(lines.num_lines()): - cpp_style.check_style(lines, i, file_extension, file_state, error_collector) + cpp_style.check_style(lines, i, file_extension, class_state, file_state, error_collector) cpp_style.check_for_non_standard_constructs(lines, i, class_state, error_collector) class_state.check_finished(error_collector) @@ -199,7 +200,7 @@ class CppStyleTestBase(unittest.TestCase): The accumulated errors. """ error_collector = ErrorCollector(self.assert_) - function_state = cpp_style._FunctionState(self.verbosity) + function_state = cpp_style._FunctionState(self.min_confidence) lines = code.split('\n') cpp_style.remove_multi_line_comments(lines, error_collector) lines = cpp_style.CleansedLines(lines) @@ -238,7 +239,7 @@ class CppStyleTestBase(unittest.TestCase): if re.search(expected_message_re, message): return - self.assertEquals(expected_message, messages) + self.assertEquals(expected_message_re, messages) def assert_multi_line_lint(self, code, expected_message, file_name='foo.h'): file_extension = file_name[file_name.rfind('.') + 1:] @@ -1163,28 +1164,30 @@ class CppStyleTest(CppStyleTestBase): '') def test_mismatching_spaces_in_parens(self): - self.assert_lint('if (foo ) {', 'Mismatching spaces inside () in if' + self.assert_lint('if (foo ) {', 'Extra space before ) in if' ' [whitespace/parens] [5]') - self.assert_lint('switch ( foo) {', 'Mismatching spaces inside () in switch' + self.assert_lint('switch ( foo) {', 'Extra space after ( in switch' ' [whitespace/parens] [5]') - self.assert_lint('for (foo; ba; bar ) {', 'Mismatching spaces inside () in for' + self.assert_lint('for (foo; ba; bar ) {', 'Extra space before ) in for' ' [whitespace/parens] [5]') - self.assert_lint('for ((foo); (ba); (bar) ) {', 'Mismatching spaces inside () in for' + self.assert_lint('for ((foo); (ba); (bar) ) {', 'Extra space before ) in for' ' [whitespace/parens] [5]') self.assert_lint('for (; foo; bar) {', '') self.assert_lint('for (; (foo); (bar)) {', '') self.assert_lint('for ( ; foo; bar) {', '') self.assert_lint('for ( ; (foo); (bar)) {', '') - self.assert_lint('for ( ; foo; bar ) {', '') - self.assert_lint('for ( ; (foo); (bar) ) {', '') + self.assert_lint('for ( ; foo; bar ) {', 'Extra space before ) in for' + ' [whitespace/parens] [5]') + self.assert_lint('for ( ; (foo); (bar) ) {', 'Extra space before ) in for' + ' [whitespace/parens] [5]') self.assert_lint('for (foo; bar; ) {', '') self.assert_lint('for ((foo); (bar); ) {', '') - self.assert_lint('foreach (foo, foos ) {', 'Mismatching spaces inside () in foreach' + self.assert_lint('foreach (foo, foos ) {', 'Extra space before ) in foreach' ' [whitespace/parens] [5]') - self.assert_lint('foreach ( foo, foos) {', 'Mismatching spaces inside () in foreach' + self.assert_lint('foreach ( foo, foos) {', 'Extra space after ( in foreach' + ' [whitespace/parens] [5]') + self.assert_lint('while ( foo) {', 'Extra space after ( in while' ' [whitespace/parens] [5]') - self.assert_lint('while ( foo ) {', 'Should have zero or one spaces inside' - ' ( and ) in while [whitespace/parens] [5]') def test_spacing_for_fncall(self): self.assert_lint('if (foo) {', '') @@ -1541,12 +1544,20 @@ class CppStyleTest(CppStyleTestBase): self.assert_lint('f(a, /* name */ b);', '') self.assert_lint('f(a, /* name */b);', '') + def test_declaration(self): + self.assert_lint('int a;', '') + self.assert_lint('int a;', 'Extra space between int and a [whitespace/declaration] [3]') + self.assert_lint('int* a;', 'Extra space between int* and a [whitespace/declaration] [3]') + self.assert_lint('else if { }', '') + self.assert_lint('else if { }', 'Extra space between else and if [whitespace/declaration] [3]') + def test_pointer_reference_marker_location(self): self.assert_lint('int* b;', '', 'foo.cpp') self.assert_lint('int *b;', 'Declaration has space between type name and * in int *b [whitespace/declaration] [3]', 'foo.cpp') self.assert_lint('return *b;', '', 'foo.cpp') + self.assert_lint('delete *b;', '', 'foo.cpp') self.assert_lint('int *b;', '', 'foo.c') self.assert_lint('int* b;', 'Declaration has space between * and variable name in int* b [whitespace/declaration] [3]', @@ -1734,6 +1745,26 @@ class CppStyleTest(CppStyleTestBase): ' [build/header_guard] [5]' % expected_guard), error_collector.result_list()) + # Special case for flymake + error_collector = ErrorCollector(self.assert_) + self.process_file_data('mydir/Foo_flymake.h', 'h', + ['#ifndef %s' % expected_guard, + '#define %s' % expected_guard, + '#endif // %s' % expected_guard], + error_collector) + for line in error_collector.result_list(): + if line.find('build/header_guard') != -1: + self.fail('Unexpected error: %s' % line) + + error_collector = ErrorCollector(self.assert_) + self.process_file_data('mydir/Foo_flymake.h', 'h', [], error_collector) + self.assertEquals( + 1, + error_collector.result_list().count( + 'No #ifndef header guard found, suggested CPP variable is: %s' + ' [build/header_guard] [5]' % expected_guard), + error_collector.result_list()) + def test_build_printf_format(self): self.assert_lint( r'printf("\%%d", value);', @@ -2227,11 +2258,11 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): cpp_style._FunctionState._TEST_TRIGGER = self.old_test_trigger # FIXME: Eliminate the need for this function. - def set_verbosity(self, verbosity): - """Set new test verbosity and return old test verbosity.""" - old_verbosity = self.verbosity - self.verbosity = verbosity - return old_verbosity + def set_min_confidence(self, min_confidence): + """Set new test confidence and return old test confidence.""" + old_min_confidence = self.min_confidence + self.min_confidence = min_confidence + return old_min_confidence def assert_function_lengths_check(self, code, expected_message): """Check warnings for long function bodies are as expected. @@ -2272,7 +2303,7 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): lines: Number of lines to generate. error_level: --v setting for cpp_style. """ - trigger_level = self.trigger_lines(self.verbosity) + trigger_level = self.trigger_lines(self.min_confidence) self.assert_function_lengths_check( 'void test(int x)' + self.function_body(lines), ('Small and focused functions are preferred: ' @@ -2355,29 +2386,29 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): '') def test_function_length_check_definition_below_severity0(self): - old_verbosity = self.set_verbosity(0) + old_min_confidence = self.set_min_confidence(0) self.assert_function_length_check_definition_ok(self.trigger_lines(0) - 1) - self.set_verbosity(old_verbosity) + self.set_min_confidence(old_min_confidence) def test_function_length_check_definition_at_severity0(self): - old_verbosity = self.set_verbosity(0) + old_min_confidence = self.set_min_confidence(0) self.assert_function_length_check_definition_ok(self.trigger_lines(0)) - self.set_verbosity(old_verbosity) + self.set_min_confidence(old_min_confidence) def test_function_length_check_definition_above_severity0(self): - old_verbosity = self.set_verbosity(0) + old_min_confidence = self.set_min_confidence(0) self.assert_function_length_check_above_error_level(0) - self.set_verbosity(old_verbosity) + self.set_min_confidence(old_min_confidence) def test_function_length_check_definition_below_severity1v0(self): - old_verbosity = self.set_verbosity(0) + old_min_confidence = self.set_min_confidence(0) self.assert_function_length_check_below_error_level(1) - self.set_verbosity(old_verbosity) + self.set_min_confidence(old_min_confidence) def test_function_length_check_definition_at_severity1v0(self): - old_verbosity = self.set_verbosity(0) + old_min_confidence = self.set_min_confidence(0) self.assert_function_length_check_at_error_level(1) - self.set_verbosity(old_verbosity) + self.set_min_confidence(old_min_confidence) def test_function_length_check_definition_below_severity1(self): self.assert_function_length_check_definition_ok(self.trigger_lines(1) - 1) @@ -2391,7 +2422,7 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): def test_function_length_check_definition_severity1_plus_blanks(self): error_level = 1 error_lines = self.trigger_lines(error_level) + 1 - trigger_level = self.trigger_lines(self.verbosity) + trigger_level = self.trigger_lines(self.min_confidence) self.assert_function_lengths_check( 'void test_blanks(int x)' + self.function_body(error_lines), ('Small and focused functions are preferred: ' @@ -2403,7 +2434,7 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): def test_function_length_check_complex_definition_severity1(self): error_level = 1 error_lines = self.trigger_lines(error_level) + 1 - trigger_level = self.trigger_lines(self.verbosity) + trigger_level = self.trigger_lines(self.min_confidence) self.assert_function_lengths_check( ('my_namespace::my_other_namespace::MyVeryLongTypeName*\n' 'my_namespace::my_other_namespace::MyFunction(int arg1, char* arg2)' @@ -2418,7 +2449,7 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): def test_function_length_check_definition_severity1_for_test(self): error_level = 1 error_lines = self.trigger_test_lines(error_level) + 1 - trigger_level = self.trigger_test_lines(self.verbosity) + trigger_level = self.trigger_test_lines(self.min_confidence) self.assert_function_lengths_check( 'TEST_F(Test, Mutator)' + self.function_body(error_lines), ('Small and focused functions are preferred: ' @@ -2430,7 +2461,7 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): def test_function_length_check_definition_severity1_for_split_line_test(self): error_level = 1 error_lines = self.trigger_test_lines(error_level) + 1 - trigger_level = self.trigger_test_lines(self.verbosity) + trigger_level = self.trigger_test_lines(self.min_confidence) self.assert_function_lengths_check( ('TEST_F(GoogleUpdateRecoveryRegistryProtectedTest,\n' ' FixGoogleUpdate_AllValues_MachineApp)' # note: 4 spaces @@ -2445,7 +2476,7 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): def test_function_length_check_definition_severity1_for_bad_test_doesnt_break(self): error_level = 1 error_lines = self.trigger_test_lines(error_level) + 1 - trigger_level = self.trigger_test_lines(self.verbosity) + trigger_level = self.trigger_test_lines(self.min_confidence) self.assert_function_lengths_check( ('TEST_F(' + self.function_body(error_lines)), @@ -2458,7 +2489,7 @@ class CheckForFunctionLengthsTest(CppStyleTestBase): def test_function_length_check_definition_severity1_with_embedded_no_lints(self): error_level = 1 error_lines = self.trigger_lines(error_level) + 1 - trigger_level = self.trigger_lines(self.verbosity) + trigger_level = self.trigger_lines(self.min_confidence) self.assert_function_lengths_check( 'void test(int x)' + self.function_body_with_no_lints(error_lines), ('Small and focused functions are preferred: ' @@ -2563,6 +2594,19 @@ class NoNonVirtualDestructorsTest(CppStyleTestBase): self.assert_multi_line_lint( 'class Foo { void foo(); };', 'More than one command on the same line [whitespace/newline] [4]') + self.assert_multi_line_lint( + 'class MyClass {\n' + ' int getIntValue() { ASSERT(m_ptr); return *m_ptr; }\n' + '};\n', + '') + self.assert_multi_line_lint( + 'class MyClass {\n' + ' int getIntValue()\n' + ' {\n' + ' ASSERT(m_ptr); return *m_ptr;\n' + ' }\n' + '};\n', + 'More than one command on the same line [whitespace/newline] [4]') self.assert_multi_line_lint( '''class Qualified::Goo : public Foo { @@ -3037,7 +3081,7 @@ class WebKitStyleTest(CppStyleTestBase): '') self.assert_multi_line_lint( '#define TEST_ASSERT(expression) do { if ( !(expression)) { TestsController::shared().testFailed(__FILE__, __LINE__, #expression); return; } } while (0)\n', - 'Mismatching spaces inside () in if [whitespace/parens] [5]') + 'Extra space after ( in if [whitespace/parens] [5]') # FIXME: currently we only check first conditional, so we cannot detect errors in next ones. # self.assert_multi_line_lint( # '#define TEST_ASSERT(expression) do { if (!(expression)) { TestsController::shared().testFailed(__FILE__, __LINE__, #expression); return; } } while (0 )\n', @@ -3063,6 +3107,20 @@ class WebKitStyleTest(CppStyleTestBase): '}\n', ['More than one command on the same line in if [whitespace/parens] [4]', 'One line control clauses should not use braces. [whitespace/braces] [4]']) + self.assert_multi_line_lint( + 'void func()\n' + '{\n' + ' while (condition) { }\n' + ' return 0;\n' + '}\n', + '') + self.assert_multi_line_lint( + 'void func()\n' + '{\n' + ' for (i = 0; i < 42; i++) { foobar(); }\n' + ' return 0;\n' + '}\n', + 'More than one command on the same line in for [whitespace/parens] [4]') # 3. An else if statement should be written as an if statement # when the prior if concludes with a return statement. @@ -3392,13 +3450,50 @@ class WebKitStyleTest(CppStyleTestBase): '', 'foo.m') - # Make sure that the NULL check does not apply to g_object_{set,get} + # Make sure that the NULL check does not apply to g_object_{set,get} and + # g_str{join,concat} self.assert_lint( 'g_object_get(foo, "prop", &bar, NULL);', '') self.assert_lint( 'g_object_set(foo, "prop", bar, NULL);', '') + self.assert_lint( + 'g_build_filename(foo, bar, NULL);', + '') + self.assert_lint( + 'gst_bin_add_many(foo, bar, boo, NULL);', + '') + self.assert_lint( + 'gst_bin_remove_many(foo, bar, boo, NULL);', + '') + self.assert_lint( + 'gst_element_link_many(foo, bar, boo, NULL);', + '') + self.assert_lint( + 'gst_element_unlink_many(foo, bar, boo, NULL);', + '') + self.assert_lint( + 'gchar* result = g_strconcat("part1", "part2", "part3", NULL);', + '') + self.assert_lint( + 'gchar* result = g_strconcat("part1", NULL);', + '') + self.assert_lint( + 'gchar* result = g_strjoin(",", "part1", "part2", "part3", NULL);', + '') + self.assert_lint( + 'gchar* result = g_strjoin(",", "part1", NULL);', + '') + self.assert_lint( + 'gchar* result = gdk_pixbuf_save_to_callback(pixbuf, function, data, type, error, NULL);', + '') + self.assert_lint( + 'gchar* result = gdk_pixbuf_save_to_buffer(pixbuf, function, data, type, error, NULL);', + '') + self.assert_lint( + 'gchar* result = gdk_pixbuf_save_to_stream(pixbuf, function, data, type, error, NULL);', + '') # 2. C++ and C bool values should be written as true and # false. Objective-C BOOL values should be written as YES and NO. @@ -3489,7 +3584,8 @@ class WebKitStyleTest(CppStyleTestBase): 'foo.h') def test_names(self): - name_error_message = " is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]" + name_underscore_error_message = " is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]" + name_tooshort_error_message = " is incorrectly named. Don't use the single letter 'l' as an identifier name. [readability/naming] [4]" # Basic cases from WebKit style guide. self.assert_lint('struct Data;', '') @@ -3497,54 +3593,65 @@ class WebKitStyleTest(CppStyleTestBase): self.assert_lint('class HTMLDocument;', '') self.assert_lint('String mimeType();', '') self.assert_lint('size_t buffer_size;', - 'buffer_size' + name_error_message) + 'buffer_size' + name_underscore_error_message) self.assert_lint('short m_length;', '') self.assert_lint('short _length;', - '_length' + name_error_message) + '_length' + name_underscore_error_message) self.assert_lint('short length_;', - 'length_' + name_error_message) + 'length_' + name_underscore_error_message) + self.assert_lint('unsigned _length;', + '_length' + name_underscore_error_message) + self.assert_lint('unsigned int _length;', + '_length' + name_underscore_error_message) + self.assert_lint('unsigned long long _length;', + '_length' + name_underscore_error_message) + + # Variable name 'l' is easy to confuse with '1' + self.assert_lint('int l;', 'l' + name_tooshort_error_message) + self.assert_lint('size_t l;', 'l' + name_tooshort_error_message) + self.assert_lint('long long l;', 'l' + name_tooshort_error_message) # Pointers, references, functions, templates, and adjectives. self.assert_lint('char* under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('const int UNDER_SCORE;', - 'UNDER_SCORE' + name_error_message) + 'UNDER_SCORE' + name_underscore_error_message) self.assert_lint('static inline const char const& const under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('WebCore::RenderObject* under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('int func_name();', - 'func_name' + name_error_message) + 'func_name' + name_underscore_error_message) self.assert_lint('RefPtr<RenderObject*> under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('WTF::Vector<WTF::RefPtr<const RenderObject* const> > under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('int under_score[];', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('struct dirent* under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('long under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('long long under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('long double under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('long long int under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) # Declarations in control statement. self.assert_lint('if (int under_score = 42) {', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('else if (int under_score = 42) {', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('for (int under_score = 42; cond; i++) {', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('while (foo & under_score = bar) {', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('for (foo * under_score = p; cond; i++) {', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('for (foo * under_score; cond; i++) {', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('while (foo & value_in_thirdparty_library) {', '') self.assert_lint('while (foo * value_in_thirdparty_library) {', '') self.assert_lint('if (mli && S_OK == mli->foo()) {', '') @@ -3552,38 +3659,38 @@ class WebKitStyleTest(CppStyleTestBase): # More member variables and functions. self.assert_lint('int SomeClass::s_validName', '') self.assert_lint('int m_under_score;', - 'm_under_score' + name_error_message) + 'm_under_score' + name_underscore_error_message) self.assert_lint('int SomeClass::s_under_score = 0;', - 'SomeClass::s_under_score' + name_error_message) + 'SomeClass::s_under_score' + name_underscore_error_message) self.assert_lint('int SomeClass::under_score = 0;', - 'SomeClass::under_score' + name_error_message) + 'SomeClass::under_score' + name_underscore_error_message) # Other statements. self.assert_lint('return INT_MAX;', '') self.assert_lint('return_t under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('goto under_score;', - 'under_score' + name_error_message) + 'under_score' + name_underscore_error_message) self.assert_lint('delete static_cast<Foo*>(p);', '') # Multiple variables in one line. self.assert_lint('void myFunction(int variable1, int another_variable);', - 'another_variable' + name_error_message) + 'another_variable' + name_underscore_error_message) self.assert_lint('int variable1, another_variable;', - 'another_variable' + name_error_message) + 'another_variable' + name_underscore_error_message) self.assert_lint('int first_variable, secondVariable;', - 'first_variable' + name_error_message) + 'first_variable' + name_underscore_error_message) self.assert_lint('void my_function(int variable_1, int variable_2);', - ['my_function' + name_error_message, - 'variable_1' + name_error_message, - 'variable_2' + name_error_message]) + ['my_function' + name_underscore_error_message, + 'variable_1' + name_underscore_error_message, + 'variable_2' + name_underscore_error_message]) self.assert_lint('for (int variable_1, variable_2;;) {', - ['variable_1' + name_error_message, - 'variable_2' + name_error_message]) + ['variable_1' + name_underscore_error_message, + 'variable_2' + name_underscore_error_message]) # There is an exception for op code functions but only in the JavaScriptCore directory. self.assert_lint('void this_op_code(int var1, int var2)', '', 'JavaScriptCore/foo.cpp') - self.assert_lint('void this_op_code(int var1, int var2)', 'this_op_code' + name_error_message) + self.assert_lint('void this_op_code(int var1, int var2)', 'this_op_code' + name_underscore_error_message) # GObject requires certain magical names in class declarations. self.assert_lint('void webkit_dom_object_init();', '') @@ -3599,6 +3706,10 @@ class WebKitStyleTest(CppStyleTestBase): # const_iterator is allowed as well. self.assert_lint('typedef VectorType::const_iterator const_iterator;', '') + # Bitfields. + self.assert_lint('unsigned _fillRule : 1;', + '_fillRule' + name_underscore_error_message) + def test_comments(self): # A comment at the beginning of a line is ok. @@ -3614,52 +3725,52 @@ class WebKitStyleTest(CppStyleTestBase): pass -class CppProcessorTest(unittest.TestCase): +class CppCheckerTest(unittest.TestCase): - """Tests CppProcessor class.""" + """Tests CppChecker class.""" def mock_handle_style_error(self): pass - def _processor(self): - return CppProcessor("foo", "h", self.mock_handle_style_error, 3) + def _checker(self): + return CppChecker("foo", "h", self.mock_handle_style_error, 3) def test_init(self): """Test __init__ constructor.""" - processor = self._processor() - self.assertEquals(processor.file_extension, "h") - self.assertEquals(processor.file_path, "foo") - self.assertEquals(processor.handle_style_error, self.mock_handle_style_error) - self.assertEquals(processor.verbosity, 3) + checker = self._checker() + self.assertEquals(checker.file_extension, "h") + self.assertEquals(checker.file_path, "foo") + self.assertEquals(checker.handle_style_error, self.mock_handle_style_error) + self.assertEquals(checker.min_confidence, 3) def test_eq(self): """Test __eq__ equality function.""" - processor1 = self._processor() - processor2 = self._processor() + checker1 = self._checker() + checker2 = self._checker() # == calls __eq__. - self.assertTrue(processor1 == processor2) + self.assertTrue(checker1 == checker2) def mock_handle_style_error2(self): pass # Verify that a difference in any argument cause equality to fail. - processor = CppProcessor("foo", "h", self.mock_handle_style_error, 3) - self.assertFalse(processor == CppProcessor("bar", "h", self.mock_handle_style_error, 3)) - self.assertFalse(processor == CppProcessor("foo", "c", self.mock_handle_style_error, 3)) - self.assertFalse(processor == CppProcessor("foo", "h", mock_handle_style_error2, 3)) - self.assertFalse(processor == CppProcessor("foo", "h", self.mock_handle_style_error, 4)) + checker = CppChecker("foo", "h", self.mock_handle_style_error, 3) + self.assertFalse(checker == CppChecker("bar", "h", self.mock_handle_style_error, 3)) + self.assertFalse(checker == CppChecker("foo", "c", self.mock_handle_style_error, 3)) + self.assertFalse(checker == CppChecker("foo", "h", mock_handle_style_error2, 3)) + self.assertFalse(checker == CppChecker("foo", "h", self.mock_handle_style_error, 4)) def test_ne(self): """Test __ne__ inequality function.""" - processor1 = self._processor() - processor2 = self._processor() + checker1 = self._checker() + checker2 = self._checker() # != calls __ne__. # By default, __ne__ always returns true on different objects. # Thus, just check the distinguishing case to verify that the # code defines __ne__. - self.assertFalse(processor1 != processor2) + self.assertFalse(checker1 != checker2) def tearDown(): diff --git a/WebKitTools/Scripts/webkitpy/style/checkers/python.py b/WebKitTools/Scripts/webkitpy/style/checkers/python.py new file mode 100644 index 0000000..70d4450 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/checkers/python.py @@ -0,0 +1,56 @@ +# 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 checking WebKit style in Python files.""" + +from ...style_references import pep8 + + +class PythonChecker(object): + + """Processes text 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): + # Initialize pep8.options, which is necessary for + # Checker.check_all() to execute. + pep8.process_options(arglist=[self._file_path]) + + checker = pep8.Checker(self._file_path) + + def _pep8_handle_error(line_number, offset, text, check): + # FIXME: Incorporate the character offset into the error output. + # This will require updating the error handler __call__ + # signature to include an optional "offset" parameter. + pep8_code = text[:4] + pep8_message = text[5:] + + category = "pep8/" + pep8_code + + self._handle_style_error(line_number, category, 5, pep8_message) + + checker.report_error = _pep8_handle_error + + errors = checker.check_all() diff --git a/WebKitTools/Scripts/webkitpy/style/checkers/python_unittest.py b/WebKitTools/Scripts/webkitpy/style/checkers/python_unittest.py new file mode 100644 index 0000000..e003eb8 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/checkers/python_unittest.py @@ -0,0 +1,62 @@ +# 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. + +"""Unit tests for python.py.""" + +import os +import unittest + +from python import PythonChecker + + +class PythonCheckerTest(unittest.TestCase): + + """Tests the PythonChecker class.""" + + def test_init(self): + """Test __init__() method.""" + def _mock_handle_style_error(self): + pass + + checker = PythonChecker("foo.txt", _mock_handle_style_error) + self.assertEquals(checker._file_path, "foo.txt") + self.assertEquals(checker._handle_style_error, + _mock_handle_style_error) + + def test_check(self): + """Test check() method.""" + errors = [] + + def _mock_handle_style_error(line_number, category, confidence, + message): + error = (line_number, category, confidence, message) + errors.append(error) + + current_dir = os.path.dirname(__file__) + file_path = os.path.join(current_dir, "python_unittest_input.py") + + checker = PythonChecker(file_path, _mock_handle_style_error) + checker.check(lines=[]) + + self.assertEquals(len(errors), 1) + self.assertEquals(errors[0], + (2, "pep8/W291", 5, "trailing whitespace")) diff --git a/WebKitTools/Scripts/webkitpy/style/checkers/python_unittest_input.py b/WebKitTools/Scripts/webkitpy/style/checkers/python_unittest_input.py new file mode 100644 index 0000000..9f1d118 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/checkers/python_unittest_input.py @@ -0,0 +1,2 @@ +# This file is sample input for python_unittest.py and includes a single +# error which is an extra space at the end of this line. diff --git a/WebKitTools/Scripts/webkitpy/style/processors/text.py b/WebKitTools/Scripts/webkitpy/style/checkers/text.py index 307e5b8..0d03938 100644 --- a/WebKitTools/Scripts/webkitpy/style/processors/text.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/text.py @@ -30,7 +30,7 @@ """Checks WebKit style for text files.""" -class TextProcessor(object): +class TextChecker(object): """Processes text lines for checking style.""" @@ -38,7 +38,7 @@ class TextProcessor(object): self.file_path = file_path self.handle_style_error = handle_style_error - def process(self, lines): + def check(self, lines): lines = (["// adjust line numbers to make the first line 1."] + lines) # FIXME: share with cpp_style. @@ -51,6 +51,6 @@ class TextProcessor(object): # FIXME: Remove this function (requires refactoring unit tests). def process_file_data(filename, lines, error): - processor = TextProcessor(filename, error) - processor.process(lines) + checker = TextChecker(filename, error) + checker.check(lines) diff --git a/WebKitTools/Scripts/webkitpy/style/processors/text_unittest.py b/WebKitTools/Scripts/webkitpy/style/checkers/text_unittest.py index 62f825b..ced49a9 100644 --- a/WebKitTools/Scripts/webkitpy/style/processors/text_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/checkers/text_unittest.py @@ -32,7 +32,7 @@ import unittest import text as text_style -from text import TextProcessor +from text import TextChecker class TextStyleTestCase(unittest.TestCase): """TestCase for text_style.py""" @@ -76,18 +76,18 @@ class TextStyleTestCase(unittest.TestCase): '\tReviewed by NOBODY.'], 3) -class TextProcessorTest(unittest.TestCase): +class TextCheckerTest(unittest.TestCase): - """Tests TextProcessor class.""" + """Tests TextChecker class.""" def mock_handle_style_error(self): pass def test_init(self): """Test __init__ constructor.""" - processor = TextProcessor("foo.txt", self.mock_handle_style_error) - self.assertEquals(processor.file_path, "foo.txt") - self.assertEquals(processor.handle_style_error, self.mock_handle_style_error) + checker = TextChecker("foo.txt", self.mock_handle_style_error) + self.assertEquals(checker.file_path, "foo.txt") + self.assertEquals(checker.handle_style_error, self.mock_handle_style_error) if __name__ == '__main__': diff --git a/WebKitTools/Scripts/webkitpy/style/error_handlers.py b/WebKitTools/Scripts/webkitpy/style/error_handlers.py index 1940e03..0bede24 100644 --- a/WebKitTools/Scripts/webkitpy/style/error_handlers.py +++ b/WebKitTools/Scripts/webkitpy/style/error_handlers.py @@ -40,10 +40,10 @@ Methods: line_number: The integer line number of the line containing the error. category: The name of the category of the error, for example "whitespace/newline". - confidence: An integer between 1-5 that represents the level of - confidence in the error. The value 5 means that we are - certain of the problem, and the value 1 means that it - could be a legitimate construct. + confidence: An integer between 1 and 5 inclusive that represents the + application's level of confidence in the error. The value + 5 means that we are certain of the problem, and the + value 1 means that it could be a legitimate construct. message: The error message to report. """ @@ -56,35 +56,55 @@ class DefaultStyleErrorHandler(object): """The default style error handler.""" - def __init__(self, file_path, options, increment_error_count, - stderr_write=None): + def __init__(self, file_path, configuration, increment_error_count, + line_numbers=None): """Create a default style error handler. Args: file_path: The path to the file containing the error. This is used for reporting to the user. - options: A ProcessorOptions instance. + configuration: A StyleProcessorConfiguration instance. increment_error_count: A function that takes no arguments and increments the total count of reportable errors. - stderr_write: A function that takes a string as a parameter - and that is called when a style error occurs. - Defaults to sys.stderr.write. This should be - used only for unit tests. + line_numbers: An array of line numbers of the lines for which + style errors should be reported, or None if errors + for all lines should be reported. When it is not + None, this array normally contains the line numbers + corresponding to the modified lines of a patch. """ - if stderr_write is None: - stderr_write = sys.stderr.write + if line_numbers is not None: + line_numbers = set(line_numbers) self._file_path = file_path + self._configuration = configuration self._increment_error_count = increment_error_count - self._options = options - self._stderr_write = stderr_write + self._line_numbers = line_numbers # A string to integer dictionary cache of the number of reportable # errors per category passed to this instance. self._category_totals = {} + # Useful for unit testing. + def __eq__(self, other): + """Return whether this instance is equal to another.""" + if self._configuration != other._configuration: + return False + if self._file_path != other._file_path: + return False + if self._increment_error_count != other._increment_error_count: + return False + if self._line_numbers != other._line_numbers: + return False + + return True + + # Useful for unit testing. + def __ne__(self, other): + # Python does not automatically deduce __ne__ from __eq__. + return not self.__eq__(other) + def _add_reportable_error(self, category): """Increment the error count and return the new category total.""" self._increment_error_count() # Increment the total. @@ -99,9 +119,9 @@ class DefaultStyleErrorHandler(object): def _max_reports(self, category): """Return the maximum number of errors to report.""" - if not category in self._options.max_reports_per_category: + if not category in self._configuration.max_reports_per_category: return None - return self._options.max_reports_per_category[category] + return self._configuration.max_reports_per_category[category] def __call__(self, line_number, category, confidence, message): """Handle the occurrence of a style error. @@ -109,9 +129,15 @@ class DefaultStyleErrorHandler(object): See the docstring of this module for more information. """ - if not self._options.is_reportable(category, - confidence, - self._file_path): + 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. + return + + if not self._configuration.is_reportable(category=category, + confidence_in_error=confidence, + file_path=self._file_path): return category_total = self._add_reportable_error(category) @@ -122,69 +148,12 @@ class DefaultStyleErrorHandler(object): # Then suppress displaying the error. return - if self._options.output_format == 'vs7': - format_string = "%s(%s): %s [%s] [%d]\n" - else: - format_string = "%s:%s: %s [%s] [%d]\n" + self._configuration.write_style_error(category=category, + confidence_in_error=confidence, + file_path=self._file_path, + line_number=line_number, + message=message) if category_total == max_reports: - format_string += ("Suppressing further [%s] reports for this " - "file.\n" % category) - - self._stderr_write(format_string % (self._file_path, - line_number, - message, - category, - confidence)) - - -class PatchStyleErrorHandler(object): - - """The style error function for patch files.""" - - def __init__(self, diff, file_path, options, increment_error_count, - stderr_write): - """Create a patch style error handler for the given path. - - Args: - diff: A DiffFile instance. - Other arguments: see the DefaultStyleErrorHandler.__init__() - documentation for the other arguments. - - """ - self._diff = diff - self._default_error_handler = DefaultStyleErrorHandler(file_path, - options, - increment_error_count, - stderr_write) - - # The line numbers of the modified lines. This is set lazily. - self._line_numbers = set() - - def _get_line_numbers(self): - """Return the line numbers of the modified lines.""" - if not self._line_numbers: - for line in self._diff.lines: - # When deleted line is not set, it means that - # the line is newly added (or modified). - if not line[0]: - self._line_numbers.add(line[1]) - - return self._line_numbers - - def __call__(self, line_number, category, confidence, message): - """Handle the occurrence of a style error. - - This function does not report errors occurring in lines not - marked as modified or added in the patch. - - See the docstring of this module for more information. - - """ - if line_number not in self._get_line_numbers(): - # Then the error is not reportable. - return - - self._default_error_handler(line_number, category, confidence, - message) - + self._configuration.stderr_write("Suppressing further [%s] reports " + "for this file.\n" % category) diff --git a/WebKitTools/Scripts/webkitpy/style/error_handlers_unittest.py b/WebKitTools/Scripts/webkitpy/style/error_handlers_unittest.py index 1d7e998..23619cc 100644 --- a/WebKitTools/Scripts/webkitpy/style/error_handlers_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/error_handlers_unittest.py @@ -25,172 +25,163 @@ import unittest -from .. style_references import parse_patch -from checker import ProcessorOptions +from checker import StyleProcessorConfiguration from error_handlers import DefaultStyleErrorHandler -from error_handlers import PatchStyleErrorHandler +from filter import FilterConfiguration -class StyleErrorHandlerTestBase(unittest.TestCase): +class DefaultStyleErrorHandlerTest(unittest.TestCase): + + """Tests the DefaultStyleErrorHandler class.""" def setUp(self): - self._error_messages = "" + self._error_messages = [] self._error_count = 0 - def _mock_increment_error_count(self): - self._error_count += 1 - - def _mock_stderr_write(self, message): - self._error_messages += message - - -class DefaultStyleErrorHandlerTest(StyleErrorHandlerTestBase): - - """Tests DefaultStyleErrorHandler class.""" + _category = "whitespace/tab" + """The category name for the tests in this class.""" _file_path = "foo.h" + """The file path for the tests in this class.""" - _category = "whitespace/tab" + def _mock_increment_error_count(self): + self._error_count += 1 - def _error_handler(self, options): - return DefaultStyleErrorHandler(self._file_path, - options, - self._mock_increment_error_count, - self._mock_stderr_write) + def _mock_stderr_write(self, message): + self._error_messages.append(message) + + def _style_checker_configuration(self): + """Return a StyleProcessorConfiguration instance for testing.""" + base_rules = ["-whitespace", "+whitespace/tab"] + filter_configuration = FilterConfiguration(base_rules=base_rules) + + return StyleProcessorConfiguration( + filter_configuration=filter_configuration, + max_reports_per_category={"whitespace/tab": 2}, + min_confidence=3, + output_format="vs7", + stderr_write=self._mock_stderr_write) + + def _error_handler(self, configuration, line_numbers=None): + return DefaultStyleErrorHandler(configuration=configuration, + file_path=self._file_path, + increment_error_count=self._mock_increment_error_count, + line_numbers=line_numbers) def _check_initialized(self): """Check that count and error messages are initialized.""" self.assertEquals(0, self._error_count) - self.assertEquals("", self._error_messages) - - def _call(self, handle_error, options, confidence): - """Handle an error with the given error handler.""" - line_number = 100 - message = "message" - - handle_error(line_number, self._category, confidence, message) - - def _call_error_handler(self, options, confidence): - """Handle an error using a new error handler.""" - handle_error = self._error_handler(options) - self._call(handle_error, options, confidence) - - def test_call_non_reportable(self): - """Test __call__() method with a non-reportable error.""" - confidence = 1 - options = ProcessorOptions(verbosity=3) + self.assertEquals(0, len(self._error_messages)) + + def _call_error_handler(self, handle_error, confidence, line_number=100): + """Call the given error handler with a test error.""" + handle_error(line_number=line_number, + category=self._category, + confidence=confidence, + message="message") + + def test_eq__true_return_value(self): + """Test the __eq__() method for the return value of True.""" + handler1 = self._error_handler(configuration=None) + handler2 = self._error_handler(configuration=None) + + self.assertTrue(handler1.__eq__(handler2)) + + def test_eq__false_return_value(self): + """Test the __eq__() method for the return value of False.""" + def make_handler(configuration=self._style_checker_configuration(), + file_path='foo.txt', increment_error_count=lambda: True, + line_numbers=[100]): + return DefaultStyleErrorHandler(configuration=configuration, + file_path=file_path, + increment_error_count=increment_error_count, + line_numbers=line_numbers) + + handler = make_handler() + + # Establish a baseline for our comparisons below. + self.assertTrue(handler.__eq__(make_handler())) + + # Verify that a difference in any argument causes equality to fail. + self.assertFalse(handler.__eq__(make_handler(configuration=None))) + self.assertFalse(handler.__eq__(make_handler(file_path='bar.txt'))) + self.assertFalse(handler.__eq__(make_handler(increment_error_count=None))) + self.assertFalse(handler.__eq__(make_handler(line_numbers=[50]))) + + def test_ne(self): + """Test the __ne__() method.""" + # By default, __ne__ always returns true on different objects. + # Thus, check just the distinguishing case to verify that the + # code defines __ne__. + handler1 = self._error_handler(configuration=None) + handler2 = self._error_handler(configuration=None) + + self.assertFalse(handler1.__ne__(handler2)) + + def test_non_reportable_error(self): + """Test __call__() with a non-reportable error.""" self._check_initialized() + configuration = self._style_checker_configuration() + confidence = 1 # Confirm the error is not reportable. - self.assertFalse(options.is_reportable(self._category, - confidence, - self._file_path)) - - self._call_error_handler(options, confidence) + self.assertFalse(configuration.is_reportable(self._category, + confidence, + self._file_path)) + error_handler = self._error_handler(configuration) + self._call_error_handler(error_handler, confidence) self.assertEquals(0, self._error_count) - self.assertEquals("", self._error_messages) - - def test_call_reportable_emacs(self): - """Test __call__() method with a reportable error and emacs format.""" - confidence = 5 - options = ProcessorOptions(verbosity=3, output_format="emacs") - self._check_initialized() - - self._call_error_handler(options, confidence) - - self.assertEquals(1, self._error_count) - self.assertEquals(self._error_messages, - "foo.h:100: message [whitespace/tab] [5]\n") + self.assertEquals([], self._error_messages) - def test_call_reportable_vs7(self): - """Test __call__() method with a reportable error and vs7 format.""" - confidence = 5 - options = ProcessorOptions(verbosity=3, output_format="vs7") + # Also serves as a reportable error test. + def test_max_reports_per_category(self): + """Test error report suppression in __call__() method.""" self._check_initialized() + configuration = self._style_checker_configuration() + error_handler = self._error_handler(configuration) - self._call_error_handler(options, confidence) - - self.assertEquals(1, self._error_count) - self.assertEquals(self._error_messages, - "foo.h(100): message [whitespace/tab] [5]\n") - - def test_call_max_reports_per_category(self): - """Test error report suppression in __call__() method.""" confidence = 5 - options = ProcessorOptions(verbosity=3, - max_reports_per_category={self._category: 2}) - error_handler = self._error_handler(options) - - self._check_initialized() # First call: usual reporting. - self._call(error_handler, options, confidence) + self._call_error_handler(error_handler, confidence) self.assertEquals(1, self._error_count) + self.assertEquals(1, len(self._error_messages)) self.assertEquals(self._error_messages, - "foo.h:100: message [whitespace/tab] [5]\n") + ["foo.h(100): message [whitespace/tab] [5]\n"]) # Second call: suppression message reported. - self._error_messages = "" - self._call(error_handler, options, confidence) + self._call_error_handler(error_handler, confidence) + # The "Suppressing further..." message counts as an additional + # message (but not as an addition to the error count). self.assertEquals(2, self._error_count) - self.assertEquals(self._error_messages, - "foo.h:100: message [whitespace/tab] [5]\n" - "Suppressing further [%s] reports for this file.\n" - % self._category) + self.assertEquals(3, len(self._error_messages)) + self.assertEquals(self._error_messages[-2], + "foo.h(100): message [whitespace/tab] [5]\n") + self.assertEquals(self._error_messages[-1], + "Suppressing further [whitespace/tab] reports " + "for this file.\n") # Third call: no report. - self._error_messages = "" - self._call(error_handler, options, confidence) + self._call_error_handler(error_handler, confidence) self.assertEquals(3, self._error_count) - self.assertEquals(self._error_messages, "") - - -class PatchStyleErrorHandlerTest(StyleErrorHandlerTestBase): + self.assertEquals(3, len(self._error_messages)) - """Tests PatchStyleErrorHandler class.""" - - _file_path = "__init__.py" - - _patch_string = """diff --git a/__init__.py b/__init__.py -index ef65bee..e3db70e 100644 ---- a/__init__.py -+++ b/__init__.py -@@ -1 +1,2 @@ - # Required for Python to search this directory for module files -+# New line - -""" - - def test_call(self): - patch_files = parse_patch(self._patch_string) - diff = patch_files[self._file_path] - - options = ProcessorOptions(verbosity=3) - - handle_error = PatchStyleErrorHandler(diff, - self._file_path, - options, - self._mock_increment_error_count, - self._mock_stderr_write) - - category = "whitespace/tab" + def test_line_numbers(self): + """Test the line_numbers parameter.""" + self._check_initialized() + configuration = self._style_checker_configuration() + error_handler = self._error_handler(configuration, + line_numbers=[50]) confidence = 5 - message = "message" - - # Confirm error is reportable. - self.assertTrue(options.is_reportable(category, - confidence, - self._file_path)) - - # Confirm error count initialized to zero. - self.assertEquals(0, self._error_count) - # Test error in unmodified line (error count does not increment). - handle_error(1, category, confidence, message) + # Error on non-modified line: no error. + self._call_error_handler(error_handler, confidence, line_number=60) self.assertEquals(0, self._error_count) + self.assertEquals([], self._error_messages) - # Test error in modified line (error count increments). - handle_error(2, category, confidence, message) + # Error on modified line: error. + self._call_error_handler(error_handler, confidence, line_number=50) self.assertEquals(1, self._error_count) - + self.assertEquals(self._error_messages, + ["foo.h(50): message [whitespace/tab] [5]\n"]) diff --git a/WebKitTools/Scripts/webkitpy/style/filereader.py b/WebKitTools/Scripts/webkitpy/style/filereader.py new file mode 100644 index 0000000..48455b3 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/filereader.py @@ -0,0 +1,148 @@ +# Copyright (C) 2009 Google Inc. All rights reserved. +# Copyright (C) 2010 Chris Jerdonek (chris.jerdonek@gmail.com) +# Copyright (C) 2010 ProFUSION embedded systems +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * 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. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND 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 THE COPYRIGHT +# OWNER OR 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 reading and processing text files.""" + +import codecs +import logging +import os +import sys + + +_log = logging.getLogger(__name__) + + +class TextFileReader(object): + + """Supports reading and processing text files. + + Attributes: + file_count: The total number of files passed to this instance + for processing, including non-text files and files + that should be skipped. + + """ + + def __init__(self, processor): + """Create an instance. + + Arguments: + processor: A ProcessorBase instance. + + """ + self._processor = processor + self.file_count = 0 + + def _read_lines(self, file_path): + """Read the file at a path, and return its lines. + + Raises: + IOError: If the file does not exist or cannot be read. + + """ + # Support the UNIX convention of using "-" for stdin. + if file_path == '-': + file = codecs.StreamReaderWriter(sys.stdin, + codecs.getreader('utf8'), + codecs.getwriter('utf8'), + 'replace') + else: + # We do not open the file with universal newline support + # (codecs does not support it anyway), so the resulting + # lines contain trailing "\r" characters if we are reading + # a file with CRLF endings. + file = codecs.open(file_path, 'r', 'utf8', 'replace') + + try: + contents = file.read() + finally: + file.close() + + lines = contents.split('\n') + return lines + + def process_file(self, file_path, **kwargs): + """Process the given file by calling the processor's process() method. + + Args: + file_path: The path of the file to process. + **kwargs: Any additional keyword parameters that should be passed + to the processor's process() method. The process() + method should support these keyword arguments. + + Raises: + SystemExit: If no file at file_path exists. + + """ + self.file_count += 1 + + if not os.path.exists(file_path) and file_path != "-": + _log.error("File does not exist: '%s'" % file_path) + sys.exit(1) + + if not self._processor.should_process(file_path): + _log.debug("Skipping file: '%s'" % file_path) + return + _log.debug("Processing file: '%s'" % file_path) + + try: + lines = self._read_lines(file_path) + except IOError, err: + message = ("Could not read file. Skipping: '%s'\n %s" + % (file_path, err)) + _log.warn(message) + return + + self._processor.process(lines, file_path, **kwargs) + + def _process_directory(self, directory): + """Process all files in the given directory, recursively. + + Args: + directory: A directory path. + + """ + for dir_path, dir_names, file_names in os.walk(directory): + for file_name in file_names: + file_path = os.path.join(dir_path, file_name) + self.process_file(file_path) + + def process_paths(self, paths): + """Process the given file and directory paths. + + Args: + paths: A list of file and directory paths. + + """ + for path in paths: + if os.path.isdir(path): + self._process_directory(directory=path) + else: + self.process_file(path) diff --git a/WebKitTools/Scripts/webkitpy/style/filereader_unittest.py b/WebKitTools/Scripts/webkitpy/style/filereader_unittest.py new file mode 100644 index 0000000..558ec5a --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/filereader_unittest.py @@ -0,0 +1,161 @@ +# 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. + +"""Contains unit tests for filereader.py.""" + +from __future__ import with_statement + +import codecs +import os +import shutil +import tempfile +import unittest + +from webkitpy.common.system.logtesting import LoggingTestCase +from webkitpy.style.checker import ProcessorBase +from webkitpy.style.filereader import TextFileReader + + +class TextFileReaderTest(LoggingTestCase): + + class MockProcessor(ProcessorBase): + + """A processor for test purposes. + + This processor simply records the parameters passed to its process() + method for later checking by the unittest test methods. + + """ + + def __init__(self): + self.processed = [] + """The parameters passed for all calls to the process() method.""" + + def should_process(self, file_path): + return not file_path.endswith('should_not_process.txt') + + def process(self, lines, file_path, test_kwarg=None): + self.processed.append((lines, file_path, test_kwarg)) + + def setUp(self): + LoggingTestCase.setUp(self) + processor = TextFileReaderTest.MockProcessor() + + temp_dir = tempfile.mkdtemp() + + self._file_reader = TextFileReader(processor) + self._processor = processor + self._temp_dir = temp_dir + + def tearDown(self): + LoggingTestCase.tearDown(self) + shutil.rmtree(self._temp_dir) + + def _create_file(self, rel_path, text, encoding="utf-8"): + """Create a file with given text and return the path to the file.""" + # FIXME: There are better/more secure APIs for creatin tmp file paths. + file_path = os.path.join(self._temp_dir, rel_path) + with codecs.open(file_path, "w", encoding) as file: + file.write(text) + return file_path + + def _passed_to_processor(self): + """Return the parameters passed to MockProcessor.process().""" + return self._processor.processed + + def _assert_file_reader(self, passed_to_processor, file_count): + """Assert the state of the file reader.""" + self.assertEquals(passed_to_processor, self._passed_to_processor()) + self.assertEquals(file_count, self._file_reader.file_count) + + def test_process_file__does_not_exist(self): + try: + self._file_reader.process_file('does_not_exist.txt') + except SystemExit, err: + self.assertEquals(str(err), '1') + else: + self.fail('No Exception raised.') + self._assert_file_reader([], 1) + self.assertLog(["ERROR: File does not exist: 'does_not_exist.txt'\n"]) + + def test_process_file__is_dir(self): + temp_dir = os.path.join(self._temp_dir, 'test_dir') + os.mkdir(temp_dir) + + self._file_reader.process_file(temp_dir) + + # Because the log message below contains exception text, it is + # possible that the text varies across platforms. For this reason, + # we check only the portion of the log message that we control, + # namely the text at the beginning. + log_messages = self.logMessages() + # We remove the message we are looking at to prevent the tearDown() + # from raising an exception when it asserts that no log messages + # remain. + message = log_messages.pop() + + self.assertTrue(message.startswith('WARNING: Could not read file. ' + "Skipping: '%s'\n " % temp_dir)) + + self._assert_file_reader([], 1) + + def test_process_file__should_not_process(self): + file_path = self._create_file('should_not_process.txt', 'contents') + + self._file_reader.process_file(file_path) + self._assert_file_reader([], 1) + + def test_process_file__multiple_lines(self): + file_path = self._create_file('foo.txt', 'line one\r\nline two\n') + + self._file_reader.process_file(file_path) + processed = [(['line one\r', 'line two', ''], file_path, None)] + self._assert_file_reader(processed, 1) + + def test_process_file__file_stdin(self): + file_path = self._create_file('-', 'file contents') + + self._file_reader.process_file(file_path=file_path, test_kwarg='foo') + processed = [(['file contents'], file_path, 'foo')] + self._assert_file_reader(processed, 1) + + def test_process_file__with_kwarg(self): + file_path = self._create_file('foo.txt', 'file contents') + + self._file_reader.process_file(file_path=file_path, test_kwarg='foo') + processed = [(['file contents'], file_path, 'foo')] + self._assert_file_reader(processed, 1) + + def test_process_paths(self): + # We test a list of paths that contains both a file and a directory. + dir = os.path.join(self._temp_dir, 'foo_dir') + os.mkdir(dir) + + file_path1 = self._create_file('file1.txt', 'foo') + + rel_path = os.path.join('foo_dir', 'file2.txt') + file_path2 = self._create_file(rel_path, 'bar') + + self._file_reader.process_paths([dir, file_path1]) + processed = [(['bar'], file_path2, None), + (['foo'], file_path1, None)] + self._assert_file_reader(processed, 2) diff --git a/WebKitTools/Scripts/webkitpy/style/filter.py b/WebKitTools/Scripts/webkitpy/style/filter.py index 19c2f4d..608a9e6 100644 --- a/WebKitTools/Scripts/webkitpy/style/filter.py +++ b/WebKitTools/Scripts/webkitpy/style/filter.py @@ -139,12 +139,8 @@ class FilterConfiguration(object): are appended. The first substring match takes precedence, i.e. only the first match triggers an append. - The "path_rules" value is the tuple of filter + The "path_rules" value is a list of filter rules that can be appended to the base rules. - The value is a tuple rather than a list so it - can be used as a dictionary key. The dictionary - is for caching purposes in the implementation of - this class. user_rules: A list of filter rules that is always appended to the base rules and any path rules. In other @@ -165,11 +161,7 @@ class FilterConfiguration(object): self._path_specific_lower = None """The backing store for self._get_path_specific_lower().""" - # FIXME: Make user rules internal after the FilterConfiguration - # attribute is removed from ProcessorOptions (since at - # that point ArgumentPrinter will no longer need to - # access FilterConfiguration.user_rules). - self.user_rules = user_rules + self._user_rules = user_rules self._path_rules_to_filter = {} """Cached dictionary of path rules to CategoryFilter instance.""" @@ -188,7 +180,7 @@ class FilterConfiguration(object): return False if self._path_specific != other._path_specific: return False - if self.user_rules != other.user_rules: + if self._user_rules != other._user_rules: return False return True @@ -210,22 +202,34 @@ class FilterConfiguration(object): return self._path_specific_lower def _path_rules_from_path(self, path): - """Determine the path-specific rules to use, and return as a tuple.""" + """Determine the path-specific rules to use, and return as a tuple. + + This method returns a tuple rather than a list so the return + value can be passed to _filter_from_path_rules() without change. + + """ path = path.lower() for (sub_paths, path_rules) in self._get_path_specific_lower(): for sub_path in sub_paths: if path.find(sub_path) > -1: - return path_rules + return tuple(path_rules) return () # Default to the empty tuple. def _filter_from_path_rules(self, path_rules): - """Return the CategoryFilter associated to a path rules tuple.""" + """Return the CategoryFilter associated to the given path rules. + + Args: + path_rules: A tuple of path rules. We require a tuple rather + than a list so the value can be used as a dictionary + key in self._path_rules_to_filter. + + """ # We reuse the same CategoryFilter where possible to take # advantage of the caching they do. if path_rules not in self._path_rules_to_filter: rules = list(self._base_rules) # Make a copy rules.extend(path_rules) - rules.extend(self.user_rules) + rules.extend(self._user_rules) self._path_rules_to_filter[path_rules] = _CategoryFilter(rules) return self._path_rules_to_filter[path_rules] diff --git a/WebKitTools/Scripts/webkitpy/style/filter_unittest.py b/WebKitTools/Scripts/webkitpy/style/filter_unittest.py index 84760a5..7b8a5402 100644 --- a/WebKitTools/Scripts/webkitpy/style/filter_unittest.py +++ b/WebKitTools/Scripts/webkitpy/style/filter_unittest.py @@ -157,14 +157,14 @@ class FilterConfigurationTest(unittest.TestCase): # Test that the attributes are getting set correctly. # We use parameter values that are different from the defaults. base_rules = ["-"] - path_specific = [(["path"], ("+a",))] + path_specific = [(["path"], ["+a"])] user_rules = ["+"] config = self._config(base_rules, path_specific, user_rules) self.assertEquals(base_rules, config._base_rules) self.assertEquals(path_specific, config._path_specific) - self.assertEquals(user_rules, config.user_rules) + self.assertEquals(user_rules, config._user_rules) def test_default_arguments(self): # Test that the attributes are getting set correctly to the defaults. @@ -172,7 +172,7 @@ class FilterConfigurationTest(unittest.TestCase): self.assertEquals([], config._base_rules) self.assertEquals([], config._path_specific) - self.assertEquals([], config.user_rules) + self.assertEquals([], config._user_rules) def test_eq(self): """Test __eq__ method.""" @@ -185,7 +185,7 @@ class FilterConfigurationTest(unittest.TestCase): # These parameter values are different from the defaults. base_rules = ["-"] - path_specific = [(["path"], ("+a",))] + path_specific = [(["path"], ["+a"])] user_rules = ["+"] self.assertFalse(config.__eq__(FilterConfiguration( @@ -219,8 +219,8 @@ class FilterConfigurationTest(unittest.TestCase): def test_path_specific(self): """Test effect of path_rules_specifier on should_check().""" base_rules = ["-"] - path_specific = [(["path1"], ("+b",)), - (["path2"], ("+c",))] + path_specific = [(["path1"], ["+b"]), + (["path2"], ["+c"])] user_rules = [] config = self._config(base_rules, path_specific, user_rules) @@ -233,7 +233,7 @@ class FilterConfigurationTest(unittest.TestCase): def test_path_with_different_case(self): """Test a path that differs only in case.""" base_rules = ["-"] - path_specific = [(["Foo/"], ("+whitespace",))] + path_specific = [(["Foo/"], ["+whitespace"])] user_rules = [] config = self._config(base_rules, path_specific, user_rules) diff --git a/WebKitTools/Scripts/webkitpy/style/main.py b/WebKitTools/Scripts/webkitpy/style/main.py new file mode 100644 index 0000000..c933c6d --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/main.py @@ -0,0 +1,130 @@ +# 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. + +import logging +import os +import sys + +from webkitpy.common.system.ospath import relpath as _relpath + + +_log = logging.getLogger(__name__) + + +def change_directory(checkout_root, paths, mock_os=None): + """Change the working directory to the WebKit checkout root, if possible. + + If every path in the paths parameter is below the checkout root (or if + the paths parameter is empty or None), this method changes the current + working directory to the checkout root and converts the paths parameter + as described below. + This allows the paths being checked to be displayed relative to the + checkout root, and for path-specific style checks to work as expected. + Path-specific checks include whether files should be skipped, whether + custom style rules should apply to certain files, etc. + If the checkout root is None or the empty string, this method returns + the paths parameter unchanged. + + Returns: + paths: A copy of the paths parameter -- possibly converted, as follows. + If this method changed the current working directory to the + checkout root, then the list is the paths parameter converted to + normalized paths relative to the checkout root. Otherwise, the + paths are not converted. + + Args: + paths: A list of paths to the files that should be checked for style. + This argument can be None or the empty list if a git commit + or all changes under the checkout root should be checked. + checkout_root: The path to the root of the WebKit checkout, or None or + the empty string if no checkout could be detected. + mock_os: A replacement module for unit testing. Defaults to os. + + """ + os_module = os if mock_os is None else mock_os + + if paths is not None: + paths = list(paths) + + if not checkout_root: + if not paths: + raise Exception("The paths parameter must be non-empty if " + "there is no checkout root.") + + # FIXME: Consider trying to detect the checkout root for each file + # being checked rather than only trying to detect the checkout + # root for the current working directory. This would allow + # files to be checked correctly even if the script is being + # run from outside any WebKit checkout. + # + # Moreover, try to find the "source root" for each file + # using path-based heuristics rather than using only the + # presence of a WebKit checkout. For example, we could + # examine parent directories until a directory is found + # containing JavaScriptCore, WebCore, WebKit, WebKitSite, + # and WebKitTools. + # Then log an INFO message saying that a source root not + # in a WebKit checkout was found. This will allow us to check + # the style of non-scm copies of the source tree (e.g. + # nightlies). + _log.warn("WebKit checkout root not found:\n" + " Path-dependent style checks may not work correctly.\n" + " See the help documentation for more info.") + + return paths + + if paths: + # Then try converting all of the paths to paths relative to + # the checkout root. + rel_paths = [] + for path in paths: + rel_path = _relpath(path, checkout_root) + if rel_path is None: + # Then the path is not below the checkout root. Since all + # paths should be interpreted relative to the same root, + # do not interpret any of the paths as relative to the + # checkout root. Interpret all of them relative to the + # current working directory, and do not change the current + # working directory. + _log.warn( +"""Path-dependent style checks may not work correctly: + + One of the given paths is outside the WebKit checkout of the current + working directory: + + Path: %s + Checkout root: %s + + Pass only files below the checkout root to ensure correct results. + See the help documentation for more info. +""" + % (path, checkout_root)) + + return paths + rel_paths.append(rel_path) + # If we got here, the conversion was successful. + paths = rel_paths + + _log.debug("Changing to checkout root: " + checkout_root) + os_module.chdir(checkout_root) + + return paths diff --git a/WebKitTools/Scripts/webkitpy/style/main_unittest.py b/WebKitTools/Scripts/webkitpy/style/main_unittest.py new file mode 100644 index 0000000..fe448f5 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/main_unittest.py @@ -0,0 +1,124 @@ +# 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. + +"""Unit tests for main.py.""" + +import os +import unittest + +from main import change_directory +from webkitpy.style_references import LogTesting + + +class ChangeDirectoryTest(unittest.TestCase): + + """Tests change_directory().""" + + _original_directory = "/original" + _checkout_root = "/WebKit" + + class _MockOs(object): + + """A mock os module for unit testing.""" + + def __init__(self, test_case): + self._test_case = test_case + self._current_directory = \ + ChangeDirectoryTest._original_directory + + def chdir(self, current_directory): + self._current_directory = current_directory + + def assertCurrentDirectory(self, expected_directory): + self._test_case.assertEquals(expected_directory, + self._current_directory) + + def setUp(self): + self._log = LogTesting.setUp(self) + self._mock_os = self._MockOs(self) + + def tearDown(self): + self._log.tearDown() + + # This method is a convenient wrapper for change_working_directory() that + # passes the mock_os for this unit testing class. + def _change_directory(self, paths, checkout_root): + return change_directory(paths=paths, + checkout_root=checkout_root, + mock_os=self._mock_os) + + def _assert_result(self, actual_return_value, expected_return_value, + expected_log_messages, expected_current_directory): + self.assertEquals(actual_return_value, expected_return_value) + self._log.assertMessages(expected_log_messages) + self._mock_os.assertCurrentDirectory(expected_current_directory) + + def test_checkout_root_none_paths_none(self): + self.assertRaises(Exception, self._change_directory, + checkout_root=None, paths=None) + self._log.assertMessages([]) + self._mock_os.assertCurrentDirectory(self._original_directory) + + def test_checkout_root_none(self): + paths = self._change_directory(checkout_root=None, + paths=["path1"]) + log_messages = [ +"""WARNING: WebKit checkout root not found: + Path-dependent style checks may not work correctly. + See the help documentation for more info. +"""] + self._assert_result(paths, ["path1"], log_messages, + self._original_directory) + + def test_paths_none(self): + paths = self._change_directory(checkout_root=self._checkout_root, + paths=None) + self._assert_result(paths, None, [], self._checkout_root) + + def test_paths_convertible(self): + paths=["/WebKit/foo1.txt", + "/WebKit/foo2.txt"] + paths = self._change_directory(checkout_root=self._checkout_root, + paths=paths) + self._assert_result(paths, ["foo1.txt", "foo2.txt"], [], + self._checkout_root) + + def test_with_scm_paths_unconvertible(self): + paths=["/WebKit/foo1.txt", + "/outside/foo2.txt"] + paths = self._change_directory(checkout_root=self._checkout_root, + paths=paths) + log_messages = [ +"""WARNING: Path-dependent style checks may not work correctly: + + One of the given paths is outside the WebKit checkout of the current + working directory: + + Path: /outside/foo2.txt + Checkout root: /WebKit + + Pass only files below the checkout root to ensure correct results. + See the help documentation for more info. + +"""] + self._assert_result(paths, paths, log_messages, + self._original_directory) diff --git a/WebKitTools/Scripts/webkitpy/style/optparser.py b/WebKitTools/Scripts/webkitpy/style/optparser.py new file mode 100644 index 0000000..3ba0fae --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/optparser.py @@ -0,0 +1,450 @@ +# 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 parsing of command-line options for check-webkit-style.""" + +import logging +from optparse import OptionParser +import os.path +import sys + +from filter import validate_filter_rules +# This module should not import anything from checker.py. + +_log = logging.getLogger(__name__) + +_USAGE = """usage: %prog [--help] [options] [path1] [path2] ... + +Overview: + Check coding style according to WebKit style guidelines: + + http://webkit.org/coding/coding-style.html + + Path arguments can be files and directories. If neither a git commit nor + paths are passed, then all changes in your source control working directory + are checked. + +Style errors: + This script assigns to every style error a confidence score from 1-5 and + a category name. A confidence score of 5 means the error is certainly + a problem, and 1 means it could be fine. + + Category names appear in error messages in brackets, for example + [whitespace/indent]. See the options section below for an option that + displays all available categories and which are reported by default. + +Filters: + Use filters to configure what errors to report. Filters are specified using + a comma-separated list of boolean filter rules. The script reports errors + in a category if the category passes the filter, as described below. + + All categories start out passing. Boolean filter rules are then evaluated + from left to right, with later rules taking precedence. For example, the + rule "+foo" passes any category that starts with "foo", and "-foo" fails + any such category. The filter input "-whitespace,+whitespace/braces" fails + the category "whitespace/tab" and passes "whitespace/braces". + + Examples: --filter=-whitespace,+whitespace/braces + --filter=-whitespace,-runtime/printf,+runtime/printf_format + --filter=-,+build/include_what_you_use + +Paths: + Certain style-checking behavior depends on the paths relative to + the WebKit source root of the files being checked. For example, + certain types of errors may be handled differently for files in + WebKit/gtk/webkit/ (e.g. by suppressing "readability/naming" errors + for files in this directory). + + Consequently, if the path relative to the source root cannot be + determined for a file being checked, then style checking may not + work correctly for that file. This can occur, for example, if no + WebKit checkout can be found, or if the source root can be detected, + but one of the files being checked lies outside the source tree. + + If a WebKit checkout can be detected and all files being checked + are in the source tree, then all paths will automatically be + converted to paths relative to the source root prior to checking. + This is also useful for display purposes. + + Currently, this command can detect the source root only if the + command is run from within a WebKit checkout (i.e. if the current + working directory is below the root of a checkout). In particular, + it is not recommended to run this script from a directory outside + a checkout. + + Running this script from a top-level WebKit source directory and + checking only files in the source tree will ensure that all style + checking behaves correctly -- whether or not a checkout can be + detected. This is because all file paths will already be relative + to the source root and so will not need to be converted.""" + +_EPILOG = ("This script can miss errors and does not substitute for " + "code review.") + + +# This class should not have knowledge of the flag key names. +class DefaultCommandOptionValues(object): + + """Stores the default check-webkit-style command-line options. + + Attributes: + output_format: A string that is the default output format. + min_confidence: An integer that is the default minimum confidence level. + + """ + + def __init__(self, min_confidence, output_format): + self.min_confidence = min_confidence + self.output_format = output_format + + +# This class should not have knowledge of the flag key names. +class CommandOptionValues(object): + + """Stores the option values passed by the user via the command line. + + Attributes: + is_verbose: A boolean value of whether verbose logging is enabled. + + filter_rules: The list of filter rules provided by the user. + These rules are appended to the base rules and + path-specific rules and so take precedence over + the base filter rules, etc. + + git_commit: A string representing the git commit to check. + The default is None. + + min_confidence: An integer between 1 and 5 inclusive that is the + minimum confidence level of style errors to report. + The default is 1, which reports all errors. + + output_format: A string that is the output format. The supported + output formats are "emacs" which emacs can parse + and "vs7" which Microsoft Visual Studio 7 can parse. + + """ + def __init__(self, + filter_rules=None, + git_commit=None, + is_verbose=False, + min_confidence=1, + output_format="emacs"): + if filter_rules is None: + filter_rules = [] + + if (min_confidence < 1) or (min_confidence > 5): + raise ValueError('Invalid "min_confidence" parameter: value ' + "must be an integer between 1 and 5 inclusive. " + 'Value given: "%s".' % min_confidence) + + if output_format not in ("emacs", "vs7"): + raise ValueError('Invalid "output_format" parameter: ' + 'value must be "emacs" or "vs7". ' + 'Value given: "%s".' % output_format) + + self.filter_rules = filter_rules + self.git_commit = git_commit + self.is_verbose = is_verbose + self.min_confidence = min_confidence + self.output_format = output_format + + # Useful for unit testing. + def __eq__(self, other): + """Return whether this instance is equal to another.""" + if self.filter_rules != other.filter_rules: + return False + if self.git_commit != other.git_commit: + return False + if self.is_verbose != other.is_verbose: + return False + if self.min_confidence != other.min_confidence: + return False + if self.output_format != other.output_format: + return False + + return True + + # Useful for unit testing. + def __ne__(self, other): + # Python does not automatically deduce this from __eq__(). + return not self.__eq__(other) + + +class ArgumentPrinter(object): + + """Supports the printing of check-webkit-style command arguments.""" + + def _flag_pair_to_string(self, flag_key, flag_value): + return '--%(key)s=%(val)s' % {'key': flag_key, 'val': flag_value } + + def to_flag_string(self, options): + """Return a flag string of the given CommandOptionValues instance. + + This method orders the flag values alphabetically by the flag key. + + Args: + options: A CommandOptionValues instance. + + """ + flags = {} + flags['min-confidence'] = options.min_confidence + flags['output'] = options.output_format + # Only include the filter flag if user-provided rules are present. + filter_rules = options.filter_rules + if filter_rules: + flags['filter'] = ",".join(filter_rules) + if options.git_commit: + flags['git-commit'] = options.git_commit + + flag_string = '' + # Alphabetizing lets us unit test this method. + for key in sorted(flags.keys()): + flag_string += self._flag_pair_to_string(key, flags[key]) + ' ' + + return flag_string.strip() + + +class ArgumentParser(object): + + # FIXME: Move the documentation of the attributes to the __init__ + # docstring after making the attributes internal. + """Supports the parsing of check-webkit-style command arguments. + + Attributes: + create_usage: A function that accepts a DefaultCommandOptionValues + instance and returns a string of usage instructions. + Defaults to the function that generates the usage + string for check-webkit-style. + default_options: A DefaultCommandOptionValues instance that provides + the default values for options not explicitly + provided by the user. + stderr_write: A function that takes a string as a parameter and + serves as stderr.write. Defaults to sys.stderr.write. + This parameter should be specified only for unit tests. + + """ + + def __init__(self, + all_categories, + default_options, + base_filter_rules=None, + mock_stderr=None, + usage=None): + """Create an ArgumentParser instance. + + Args: + all_categories: The set of all available style categories. + default_options: See the corresponding attribute in the class + docstring. + Keyword Args: + base_filter_rules: The list of filter rules at the beginning of + the list of rules used to check style. This + list has the least precedence when checking + style and precedes any user-provided rules. + The class uses this parameter only for display + purposes to the user. Defaults to the empty list. + create_usage: See the documentation of the corresponding + attribute in the class docstring. + stderr_write: See the documentation of the corresponding + attribute in the class docstring. + + """ + if base_filter_rules is None: + base_filter_rules = [] + stderr = sys.stderr if mock_stderr is None else mock_stderr + if usage is None: + usage = _USAGE + + self._all_categories = all_categories + self._base_filter_rules = base_filter_rules + + # FIXME: Rename these to reflect that they are internal. + self.default_options = default_options + self.stderr_write = stderr.write + + self._parser = self._create_option_parser(stderr=stderr, + usage=usage, + default_min_confidence=self.default_options.min_confidence, + default_output_format=self.default_options.output_format) + + def _create_option_parser(self, stderr, usage, + default_min_confidence, default_output_format): + # Since the epilog string is short, it is not necessary to replace + # the epilog string with a mock epilog string when testing. + # For this reason, we use _EPILOG directly rather than passing it + # as an argument like we do for the usage string. + parser = OptionParser(usage=usage, epilog=_EPILOG) + + filter_help = ('set a filter to control what categories of style ' + 'errors to report. Specify a filter using a comma-' + 'delimited list of boolean filter rules, for example ' + '"--filter -whitespace,+whitespace/braces". To display ' + 'all categories and which are enabled by default, pass ' + """no value (e.g. '-f ""' or '--filter=').""") + parser.add_option("-f", "--filter-rules", metavar="RULES", + dest="filter_value", help=filter_help) + + git_commit_help = ("check all changes in the given commit. " + "Use 'commit_id..' to check all changes after commmit_id") + parser.add_option("-g", "--git-diff", "--git-commit", + metavar="COMMIT", dest="git_commit", help=git_commit_help,) + + min_confidence_help = ("set the minimum confidence of style errors " + "to report. Can be an integer 1-5, with 1 " + "displaying all errors. Defaults to %default.") + parser.add_option("-m", "--min-confidence", metavar="INT", + type="int", dest="min_confidence", + default=default_min_confidence, + help=min_confidence_help) + + output_format_help = ('set the output format, which can be "emacs" ' + 'or "vs7" (for Visual Studio). ' + 'Defaults to "%default".') + parser.add_option("-o", "--output-format", metavar="FORMAT", + choices=["emacs", "vs7"], + dest="output_format", default=default_output_format, + help=output_format_help) + + verbose_help = "enable verbose logging." + parser.add_option("-v", "--verbose", dest="is_verbose", default=False, + action="store_true", help=verbose_help) + + # Override OptionParser's error() method so that option help will + # also display when an error occurs. Normally, just the usage + # string displays and not option help. + parser.error = self._parse_error + + # Override OptionParser's print_help() method so that help output + # does not render to the screen while running unit tests. + print_help = parser.print_help + parser.print_help = lambda: print_help(file=stderr) + + return parser + + def _parse_error(self, error_message): + """Print the help string and an error message, and exit.""" + # The method format_help() includes both the usage string and + # the flag options. + help = self._parser.format_help() + # Separate help from the error message with a single blank line. + self.stderr_write(help + "\n") + if error_message: + _log.error(error_message) + + # Since we are using this method to replace/override the Python + # module optparse's OptionParser.error() method, we match its + # behavior and exit with status code 2. + # + # As additional background, Python documentation says-- + # + # "Unix programs generally use 2 for command line syntax errors + # and 1 for all other kind of errors." + # + # (from http://docs.python.org/library/sys.html#sys.exit ) + sys.exit(2) + + def _exit_with_categories(self): + """Exit and print the style categories and default filter rules.""" + self.stderr_write('\nAll categories:\n') + for category in sorted(self._all_categories): + self.stderr_write(' ' + category + '\n') + + self.stderr_write('\nDefault filter rules**:\n') + for filter_rule in sorted(self._base_filter_rules): + self.stderr_write(' ' + filter_rule + '\n') + self.stderr_write('\n**The command always evaluates the above rules, ' + 'and before any --filter flag.\n\n') + + sys.exit(0) + + def _parse_filter_flag(self, flag_value): + """Parse the --filter flag, and return a list of filter rules. + + Args: + flag_value: A string of comma-separated filter rules, for + example "-whitespace,+whitespace/indent". + + """ + filters = [] + for uncleaned_filter in flag_value.split(','): + filter = uncleaned_filter.strip() + if not filter: + continue + filters.append(filter) + return filters + + def parse(self, args): + """Parse the command line arguments to check-webkit-style. + + Args: + args: A list of command-line arguments as returned by sys.argv[1:]. + + Returns: + A tuple of (paths, options) + + paths: The list of paths to check. + options: A CommandOptionValues instance. + + """ + (options, paths) = self._parser.parse_args(args=args) + + filter_value = options.filter_value + git_commit = options.git_commit + is_verbose = options.is_verbose + min_confidence = options.min_confidence + output_format = options.output_format + + if filter_value is not None and not filter_value: + # Then the user explicitly passed no filter, for + # example "-f ''" or "--filter=". + self._exit_with_categories() + + # Validate user-provided values. + + if paths and git_commit: + self._parse_error('You cannot provide both paths and a git ' + 'commit at the same time.') + + min_confidence = int(min_confidence) + if (min_confidence < 1) or (min_confidence > 5): + self._parse_error('option --min-confidence: invalid integer: ' + '%s: value must be between 1 and 5' + % min_confidence) + + if filter_value: + filter_rules = self._parse_filter_flag(filter_value) + else: + filter_rules = [] + + try: + validate_filter_rules(filter_rules, self._all_categories) + except ValueError, err: + self._parse_error(err) + + options = CommandOptionValues(filter_rules=filter_rules, + git_commit=git_commit, + is_verbose=is_verbose, + min_confidence=min_confidence, + output_format=output_format) + + return (paths, options) + diff --git a/WebKitTools/Scripts/webkitpy/style/optparser_unittest.py b/WebKitTools/Scripts/webkitpy/style/optparser_unittest.py new file mode 100644 index 0000000..b7e3eda --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/optparser_unittest.py @@ -0,0 +1,260 @@ +# 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. + +"""Unit tests for parser.py.""" + +import unittest + +from webkitpy.common.system.logtesting import LoggingTestCase +from webkitpy.style.optparser import ArgumentParser +from webkitpy.style.optparser import ArgumentPrinter +from webkitpy.style.optparser import CommandOptionValues as ProcessorOptions +from webkitpy.style.optparser import DefaultCommandOptionValues + + +class ArgumentPrinterTest(unittest.TestCase): + + """Tests the ArgumentPrinter class.""" + + _printer = ArgumentPrinter() + + def _create_options(self, + output_format='emacs', + min_confidence=3, + filter_rules=[], + git_commit=None): + return ProcessorOptions(filter_rules=filter_rules, + git_commit=git_commit, + min_confidence=min_confidence, + output_format=output_format) + + def test_to_flag_string(self): + options = self._create_options('vs7', 5, ['+foo', '-bar'], 'git') + self.assertEquals('--filter=+foo,-bar --git-commit=git ' + '--min-confidence=5 --output=vs7', + self._printer.to_flag_string(options)) + + # This is to check that --filter and --git-commit do not + # show up when not user-specified. + options = self._create_options() + self.assertEquals('--min-confidence=3 --output=emacs', + self._printer.to_flag_string(options)) + + +class ArgumentParserTest(LoggingTestCase): + + """Test the ArgumentParser class.""" + + class _MockStdErr(object): + + def write(self, message): + # We do not want the usage string or style categories + # to print during unit tests, so print nothing. + return + + def _parse(self, args): + """Call a test parser.parse().""" + parser = self._create_parser() + return parser.parse(args) + + def _create_defaults(self): + """Return a DefaultCommandOptionValues instance for testing.""" + base_filter_rules = ["-", "+whitespace"] + return DefaultCommandOptionValues(min_confidence=3, + output_format="vs7") + + def _create_parser(self): + """Return an ArgumentParser instance for testing.""" + default_options = self._create_defaults() + + all_categories = ["build" ,"whitespace"] + + mock_stderr = self._MockStdErr() + + return ArgumentParser(all_categories=all_categories, + base_filter_rules=[], + default_options=default_options, + mock_stderr=mock_stderr, + usage="test usage") + + def test_parse_documentation(self): + parse = self._parse + + # FIXME: Test both the printing of the usage string and the + # filter categories help. + + # Request the usage string. + self.assertRaises(SystemExit, parse, ['--help']) + # Request default filter rules and available style categories. + self.assertRaises(SystemExit, parse, ['--filter=']) + + def test_parse_bad_values(self): + parse = self._parse + + # Pass an unsupported argument. + self.assertRaises(SystemExit, parse, ['--bad']) + self.assertLog(['ERROR: no such option: --bad\n']) + + self.assertRaises(SystemExit, parse, ['--min-confidence=bad']) + self.assertLog(['ERROR: option --min-confidence: ' + "invalid integer value: 'bad'\n"]) + self.assertRaises(SystemExit, parse, ['--min-confidence=0']) + self.assertLog(['ERROR: option --min-confidence: invalid integer: 0: ' + 'value must be between 1 and 5\n']) + self.assertRaises(SystemExit, parse, ['--min-confidence=6']) + self.assertLog(['ERROR: option --min-confidence: invalid integer: 6: ' + 'value must be between 1 and 5\n']) + parse(['--min-confidence=1']) # works + parse(['--min-confidence=5']) # works + + self.assertRaises(SystemExit, parse, ['--output=bad']) + self.assertLog(['ERROR: option --output-format: invalid choice: ' + "'bad' (choose from 'emacs', 'vs7')\n"]) + parse(['--output=vs7']) # works + + # Pass a filter rule not beginning with + or -. + self.assertRaises(SystemExit, parse, ['--filter=build']) + self.assertLog(['ERROR: Invalid filter rule "build": ' + 'every rule must start with + or -.\n']) + parse(['--filter=+build']) # works + # Pass files and git-commit at the same time. + self.assertRaises(SystemExit, parse, ['--git-commit=committish', + 'file.txt']) + self.assertLog(['ERROR: You cannot provide both paths and ' + 'a git commit at the same time.\n']) + + def test_parse_default_arguments(self): + parse = self._parse + + (files, options) = parse([]) + + self.assertEquals(files, []) + + self.assertEquals(options.filter_rules, []) + self.assertEquals(options.git_commit, None) + self.assertEquals(options.is_verbose, False) + self.assertEquals(options.min_confidence, 3) + self.assertEquals(options.output_format, 'vs7') + + def test_parse_explicit_arguments(self): + parse = self._parse + + # Pass non-default explicit values. + (files, options) = parse(['--min-confidence=4']) + self.assertEquals(options.min_confidence, 4) + (files, options) = parse(['--output=emacs']) + self.assertEquals(options.output_format, 'emacs') + (files, options) = parse(['-g', 'commit']) + self.assertEquals(options.git_commit, 'commit') + (files, options) = parse(['--git-commit=commit']) + self.assertEquals(options.git_commit, 'commit') + (files, options) = parse(['--git-diff=commit']) + self.assertEquals(options.git_commit, 'commit') + (files, options) = parse(['--verbose']) + self.assertEquals(options.is_verbose, True) + + # Pass user_rules. + (files, options) = parse(['--filter=+build,-whitespace']) + self.assertEquals(options.filter_rules, + ["+build", "-whitespace"]) + + # Pass spurious white space in user rules. + (files, options) = parse(['--filter=+build, -whitespace']) + self.assertEquals(options.filter_rules, + ["+build", "-whitespace"]) + + def test_parse_files(self): + parse = self._parse + + (files, options) = parse(['foo.cpp']) + self.assertEquals(files, ['foo.cpp']) + + # Pass multiple files. + (files, options) = parse(['--output=emacs', 'foo.cpp', 'bar.cpp']) + self.assertEquals(files, ['foo.cpp', 'bar.cpp']) + + +class CommandOptionValuesTest(unittest.TestCase): + + """Tests CommandOptionValues class.""" + + def test_init(self): + """Test __init__ constructor.""" + # Check default parameters. + options = ProcessorOptions() + self.assertEquals(options.filter_rules, []) + self.assertEquals(options.git_commit, None) + self.assertEquals(options.is_verbose, False) + self.assertEquals(options.min_confidence, 1) + self.assertEquals(options.output_format, "emacs") + + # Check argument validation. + self.assertRaises(ValueError, ProcessorOptions, output_format="bad") + ProcessorOptions(output_format="emacs") # No ValueError: works + ProcessorOptions(output_format="vs7") # works + self.assertRaises(ValueError, ProcessorOptions, min_confidence=0) + self.assertRaises(ValueError, ProcessorOptions, min_confidence=6) + ProcessorOptions(min_confidence=1) # works + ProcessorOptions(min_confidence=5) # works + + # Check attributes. + options = ProcessorOptions(filter_rules=["+"], + git_commit="commit", + is_verbose=True, + min_confidence=3, + output_format="vs7") + self.assertEquals(options.filter_rules, ["+"]) + self.assertEquals(options.git_commit, "commit") + self.assertEquals(options.is_verbose, True) + self.assertEquals(options.min_confidence, 3) + self.assertEquals(options.output_format, "vs7") + + def test_eq(self): + """Test __eq__ equality function.""" + self.assertTrue(ProcessorOptions().__eq__(ProcessorOptions())) + + # Also verify that a difference in any argument causes equality to fail. + + # Explicitly create a ProcessorOptions instance with all default + # values. We do this to be sure we are assuming the right default + # values in our self.assertFalse() calls below. + options = ProcessorOptions(filter_rules=[], + git_commit=None, + is_verbose=False, + min_confidence=1, + output_format="emacs") + # Verify that we created options correctly. + self.assertTrue(options.__eq__(ProcessorOptions())) + + self.assertFalse(options.__eq__(ProcessorOptions(filter_rules=["+"]))) + self.assertFalse(options.__eq__(ProcessorOptions(git_commit="commit"))) + self.assertFalse(options.__eq__(ProcessorOptions(is_verbose=True))) + self.assertFalse(options.__eq__(ProcessorOptions(min_confidence=2))) + self.assertFalse(options.__eq__(ProcessorOptions(output_format="vs7"))) + + def test_ne(self): + """Test __ne__ inequality function.""" + # By default, __ne__ always returns true on different objects. + # Thus, just check the distinguishing case to verify that the + # code defines __ne__. + self.assertFalse(ProcessorOptions().__ne__(ProcessorOptions())) + diff --git a/WebKitTools/Scripts/webkitpy/style/patchreader.py b/WebKitTools/Scripts/webkitpy/style/patchreader.py new file mode 100644 index 0000000..7ba2b66 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/patchreader.py @@ -0,0 +1,75 @@ +# Copyright (C) 2010 Google Inc. All rights reserved. +# Copyright (C) 2010 Chris Jerdonek (chris.jerdonek@gmail.com) +# Copyright (C) 2010 ProFUSION embedded systems +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * 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. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND 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 THE COPYRIGHT +# OWNER OR 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. + +import logging + +from webkitpy.common.checkout.diff_parser import DiffParser + + +_log = logging.getLogger("webkitpy.style.patchreader") + + +class PatchReader(object): + + """Supports checking style in patches.""" + + def __init__(self, text_file_reader): + """Create a PatchReader instance. + + Args: + text_file_reader: A TextFileReader instance. + + """ + self._text_file_reader = text_file_reader + + def check(self, patch_string): + """Check style in the given patch.""" + patch_files = DiffParser(patch_string.splitlines()).files + + # The diff variable is a DiffFile instance. + for path, diff in patch_files.iteritems(): + line_numbers = set() + for line in diff.lines: + # When deleted line is not set, it means that + # the line is newly added (or modified). + if not line[0]: + line_numbers.add(line[1]) + + _log.debug('Found %s new or modified lines in: %s' + % (len(line_numbers), path)) + + # If line_numbers is empty, the file has no new or + # modified lines. In this case, we don't check the file + # because we'll never output errors for the file. + # This optimization also prevents the program from exiting + # due to a deleted file. + if line_numbers: + self._text_file_reader.process_file(file_path=path, + line_numbers=line_numbers) diff --git a/WebKitTools/Scripts/webkitpy/style/patchreader_unittest.py b/WebKitTools/Scripts/webkitpy/style/patchreader_unittest.py new file mode 100644 index 0000000..10791e4 --- /dev/null +++ b/WebKitTools/Scripts/webkitpy/style/patchreader_unittest.py @@ -0,0 +1,85 @@ +#!/usr/bin/python +# +# Copyright (C) 2010 Google Inc. All rights reserved. +# Copyright (C) 2009 Torch Mobile Inc. +# Copyright (C) 2009 Apple Inc. All rights reserved. +# Copyright (C) 2010 Chris Jerdonek (chris.jerdonek@gmail.com) +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * 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. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND 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 THE COPYRIGHT +# OWNER OR 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. + +import unittest + +from webkitpy.style.patchreader import PatchReader + + +class PatchReaderTest(unittest.TestCase): + + """Test the PatchReader class.""" + + class MockTextFileReader(object): + + def __init__(self): + self.passed_to_process_file = [] + """A list of (file_path, line_numbers) pairs.""" + + def process_file(self, file_path, line_numbers): + self.passed_to_process_file.append((file_path, line_numbers)) + + def setUp(self): + file_reader = self.MockTextFileReader() + self._file_reader = file_reader + self._patch_checker = PatchReader(file_reader) + + def _call_check_patch(self, patch_string): + self._patch_checker.check(patch_string) + + def _assert_checked(self, passed_to_process_file): + self.assertEquals(self._file_reader.passed_to_process_file, + passed_to_process_file) + + def test_check_patch(self): + # The modified line_numbers array for this patch is: [2]. + self._call_check_patch("""diff --git a/__init__.py b/__init__.py +index ef65bee..e3db70e 100644 +--- a/__init__.py ++++ b/__init__.py +@@ -1,1 +1,2 @@ + # Required for Python to search this directory for module files ++# New line +""") + self._assert_checked([("__init__.py", set([2]))]) + + def test_check_patch_with_deletion(self): + self._call_check_patch("""Index: __init__.py +=================================================================== +--- __init__.py (revision 3593) ++++ __init__.py (working copy) +@@ -1 +0,0 @@ +-foobar +""") + # _mock_check_file should not be called for the deletion patch. + self._assert_checked([]) diff --git a/WebKitTools/Scripts/webkitpy/style/unittests.py b/WebKitTools/Scripts/webkitpy/style/unittests.py deleted file mode 100644 index f8e3f71..0000000 --- a/WebKitTools/Scripts/webkitpy/style/unittests.py +++ /dev/null @@ -1,43 +0,0 @@ -#!/usr/bin/env python -# -# Copyright (C) 2010 Chris Jerdonek (chris.jerdonek@gmail.com) -# -# Redistribution and use in source and binary forms, with or without -# modification, are permitted provided that the following conditions are -# met: -# -# * Redistributions of source code must retain the above copyright -# notice, this list of conditions and the following disclaimer. -# * 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. -# * Neither the name of Apple Computer, Inc. ("Apple") nor the names of -# its contributors may be used to endorse or promote products derived -# from this software without specific prior written permission. -# -# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND 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 THE COPYRIGHT -# OWNER OR 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. - -"""Runs style package unit tests.""" - -# This module is imported by test-webkitpy. - -import sys -import unittest - -from checker_unittest import * -from error_handlers_unittest import * -from filter_unittest import * -from processors.common_unittest import * -from processors.cpp_unittest import * -from processors.text_unittest import * |