Diff
Modified: trunk/Tools/ChangeLog (96389 => 96390)
--- trunk/Tools/ChangeLog 2011-09-30 05:15:58 UTC (rev 96389)
+++ trunk/Tools/ChangeLog 2011-09-30 05:22:55 UTC (rev 96390)
@@ -1,3 +1,20 @@
+2011-09-29 David Levin <[email protected]>
+
+ watchlist: Add cross-checks for WatchList once it is filled.
+ https://bugs.webkit.org/show_bug.cgi?id=68975
+
+ Reviewed by Eric Seidel.
+
+ * Scripts/webkitpy/common/watchlist/watchlist.py: Made the data members public
+ instead of having trivial getter and setters.
+ * Scripts/webkitpy/common/watchlist/watchlist_unittest.py: Fix the unit tests to
+ pass the validation checks.
+ * Scripts/webkitpy/common/watchlist/watchlistparser.py: Add validation checks
+ and fix a few style nits.
+ * Scripts/webkitpy/common/watchlist/watchlistparser_unittest.py: Add tests for the
+ validation checks.
+ * Scripts/webkitpy/common/watchlist/watchlistrule.py: Make definitions_to_match public.
+
2011-09-29 Xianzhu Wang <[email protected]>
run-api-tests fails on chromium-win bot
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/watchlist.py (96389 => 96390)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/watchlist.py 2011-09-30 05:15:58 UTC (rev 96389)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/watchlist.py 2011-09-30 05:22:55 UTC (rev 96390)
@@ -31,31 +31,22 @@
class WatchList(object):
def __init__(self):
- self._definitions = {}
- self._cc_rules = set()
- self._message_rules = set()
+ self.definitions = {}
+ self.cc_rules = set()
+ self.message_rules = set()
- def set_definitions(self, definitions):
- self._definitions = definitions
-
- def set_cc_rules(self, cc_rules):
- self._cc_rules = cc_rules
-
- def set_message_rules(self, message_rules):
- self._message_rules = message_rules
-
def find_matching_definitions(self, diff):
matching_definitions = set()
patch_files = DiffParser(diff.splitlines()).files
for path, diff_file in patch_files.iteritems():
- for definition in self._definitions:
+ for definition in self.definitions:
# If a definition has already matched, there is no need to process it.
if definition in matching_definitions:
continue
# See if the definition matches within one file.
- for pattern in self._definitions[definition]:
+ for pattern in self.definitions[definition]:
if not pattern.match(path, diff_file.lines):
break
else:
@@ -70,10 +61,10 @@
return instructions
def determine_cc_set(self, matching_definitions):
- return self._determine_instructions(matching_definitions, self._cc_rules)
+ return self._determine_instructions(matching_definitions, self.cc_rules)
def determine_messages(self, matching_definitions):
- return self._determine_instructions(matching_definitions, self._message_rules)
+ return self._determine_instructions(matching_definitions, self.message_rules)
def determine_cc_set_and_messages(self, diff):
definitions = self.find_matching_definitions(diff)
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/watchlist_unittest.py (96389 => 96390)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/watchlist_unittest.py 2011-09-30 05:15:58 UTC (rev 96389)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/watchlist_unittest.py 2011-09-30 05:22:55 UTC (rev 96390)
@@ -46,6 +46,11 @@
' "filename": r".*\\MyFileName\\.cpp",'
' },'
' },'
+ ' "CC_RULES": {'
+ ' "WatchList1": ['
+ ' "[email protected]",'
+ ' ],'
+ ' },'
'}')
self.assertEquals(set([]), watch_list.find_matching_definitions(DIFF_TEST_DATA))
@@ -57,6 +62,11 @@
' "filename": r"WebCore/rendering/style/StyleFlexibleBoxData\.h",'
' },'
' },'
+ ' "CC_RULES": {'
+ ' "WatchList1": ['
+ ' "[email protected]",'
+ ' ],'
+ ' },'
'}')
self.assertEquals(set(['WatchList1']), watch_list.find_matching_definitions(DIFF_TEST_DATA))
@@ -87,6 +97,12 @@
' "WatchList1": {'
' "filename": r"WebCore/rendering/style/StyleFlexibleBoxData\.h",'
' },'
+ ' "WatchList2": {'
+ ' "filename": r"WillNotMatch",'
+ ' },'
+ ' "WatchList3": {'
+ ' "filename": r"WillNotMatch",'
+ ' },'
' },'
' "CC_RULES": {'
' "WatchList2|WatchList1|WatchList3": [ "[email protected]", ],'
@@ -105,6 +121,12 @@
' "WatchList1": {'
' "filename": r"WebCore/rendering/style/StyleFlexibleBoxData\.h",'
' },'
+ ' "WatchList2": {'
+ ' "filename": r"WillNotMatch",'
+ ' },'
+ ' "WatchList3": {'
+ ' "filename": r"WillNotMatch",'
+ ' },'
' },'
' "CC_RULES": {'
' "WatchList2|WatchList1|WatchList3": [ "[email protected]", ],'
@@ -126,6 +148,12 @@
' "WatchList1": {'
' "filename": r"WebCore/rendering/style/ThisFileDoesNotExist\.h",'
' },'
+ ' "WatchList2": {'
+ ' "filename": r"WillNotMatch",'
+ ' },'
+ ' "WatchList3": {'
+ ' "filename": r"WillNotMatch",'
+ ' },'
' },'
' "CC_RULES": {'
' "WatchList2|WatchList1|WatchList3": [ "[email protected]", ],'
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py (96389 => 96390)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py 2011-09-30 05:15:58 UTC (rev 96389)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser.py 2011-09-30 05:22:55 UTC (rev 96390)
@@ -28,6 +28,7 @@
import difflib
import re
+
from webkitpy.common.watchlist.changedlinepattern import ChangedLinePattern
from webkitpy.common.watchlist.filenamepattern import FilenamePattern
from webkitpy.common.watchlist.watchlist import WatchList
@@ -45,12 +46,12 @@
self._DEFINITIONS: self._parse_definition_section,
self._CC_RULES: self._parse_cc_rules,
self._MESSAGE_RULES: self._parse_message_rules,
- }
+ }
self._definition_pattern_parsers = {
'filename': FilenamePattern,
'in_added_lines': (lambda regex: ChangedLinePattern(regex, 0)),
'in_deleted_lines': (lambda regex: ChangedLinePattern(regex, 1)),
- }
+ }
def parse(self, watch_list_contents):
watch_list = WatchList()
@@ -67,6 +68,7 @@
% section)
parser(dictionary[section], watch_list)
+ self._validate(watch_list)
return watch_list
def _eval_watch_list(self, watch_list_contents):
@@ -96,16 +98,52 @@
pattern = pattern_parser(definition[pattern_type])
definitions[name].append(pattern)
- watch_list.set_definitions(definitions)
+ if not definitions[name]:
+ raise Exception('The definition "%s" has no patterns, so it should be deleted.' % name)
+ watch_list.definitions = definitions
def _parse_rules(self, rules_section):
rules = []
for complex_definition in rules_section:
- rules.append(WatchListRule(complex_definition, rules_section[complex_definition]))
+ instructions = rules_section[complex_definition]
+ if not instructions:
+ raise Exception('A rule for definition "%s" is empty, so it should be deleted.' % complex_definition)
+ rules.append(WatchListRule(complex_definition, instructions))
return rules
def _parse_cc_rules(self, cc_section, watch_list):
- watch_list.set_cc_rules(self._parse_rules(cc_section))
+ watch_list.cc_rules = self._parse_rules(cc_section)
def _parse_message_rules(self, message_section, watch_list):
- watch_list.set_message_rules(self._parse_rules(message_section))
+ watch_list.message_rules = self._parse_rules(message_section)
+
+ def _validate(self, watch_list):
+ cc_definitions_set = self._rule_definitions_as_set(watch_list.cc_rules)
+ messages_definitions_set = self._rule_definitions_as_set(watch_list.message_rules)
+ self._verify_all_definitions_are_used(watch_list, cc_definitions_set.union(messages_definitions_set))
+
+ self._validate_definitions(cc_definitions_set, self._CC_RULES, watch_list)
+ self._validate_definitions(messages_definitions_set, self._MESSAGE_RULES, watch_list)
+
+ def _verify_all_definitions_are_used(self, watch_list, used_definitions):
+ definitions_not_used = set(watch_list.definitions.keys())
+ definitions_not_used.difference_update(used_definitions)
+ if definitions_not_used:
+ raise Exception('The following definitions are not used and should be removed: %s' % (', '.join(definitions_not_used)))
+
+ def _validate_definitions(self, definitions, rules_section_name, watch_list):
+ declared_definitions = watch_list.definitions.keys()
+ definition_set = set(definitions)
+ definition_set.difference_update(declared_definitions)
+
+ if definition_set:
+ suggestions = ''
+ if len(definition_set) == 1:
+ suggestions = self._suggest_words(set().union(definition_set).pop(), declared_definitions)
+ raise Exception('In section "%s", the following definitions are not used and should be removed: %s%s' % (rules_section_name, ', '.join(definition_set), suggestions))
+
+ def _rule_definitions_as_set(self, rules):
+ definition_set = set()
+ for rule in rules:
+ definition_set = definition_set.union(rule.definitions_to_match)
+ return definition_set
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser_unittest.py (96389 => 96390)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser_unittest.py 2011-09-30 05:15:58 UTC (rev 96389)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistparser_unittest.py 2011-09-30 05:22:55 UTC (rev 96390)
@@ -87,3 +87,103 @@
self.assertRaisesRegexp(r'Unknown pattern type "iflename" in definition "WatchList1"\.\s*'
+ r'Perhaps it should be filename\.',
self._watch_list_parser.parse, watch_list_with_bad_match_type)
+
+ def test_empty_definition(self):
+ watch_list = (
+ '{'
+ ' "DEFINITIONS": {'
+ ' "WatchList1": {'
+ ' },'
+ ' },'
+ '}')
+
+ self.assertRaisesRegexp(r'The definition "WatchList1" has no patterns, so it should be deleted.',
+ self._watch_list_parser.parse, watch_list)
+
+ def test_empty_cc_rule(self):
+ watch_list = (
+ '{'
+ ' "DEFINITIONS": {'
+ ' "WatchList1": {'
+ ' "filename": r".*\\MyFileName\\.cpp",'
+ ' },'
+ ' },'
+ ' "CC_RULES": {'
+ ' "WatchList1": [],'
+ ' },'
+ '}')
+
+ self.assertRaisesRegexp(r'A rule for definition "WatchList1" is empty, so it should be deleted.',
+ self._watch_list_parser.parse, watch_list)
+
+ def test_empty_message_rule(self):
+ watch_list = (
+ '{'
+ ' "DEFINITIONS": {'
+ ' "WatchList1": {'
+ ' "filename": r".*\\MyFileName\\.cpp",'
+ ' },'
+ ' },'
+ ' "MESSAGE_RULES": {'
+ ' "WatchList1": ['
+ ' ],'
+ ' },'
+ '}')
+
+ self.assertRaisesRegexp(r'A rule for definition "WatchList1" is empty, so it should be deleted.',
+ self._watch_list_parser.parse, watch_list)
+
+ def test_unused_defintion(self):
+ watch_list = (
+ '{'
+ ' "DEFINITIONS": {'
+ ' "WatchList1": {'
+ ' "filename": r".*\\MyFileName\\.cpp",'
+ ' },'
+ ' },'
+ '}')
+
+ self.assertRaisesRegexp(r'The following definitions are not used and should be removed: WatchList1',
+ self._watch_list_parser.parse, watch_list)
+
+ def test_cc_rule_with_undefined_defintion(self):
+ watch_list = (
+ '{'
+ ' "CC_RULES": {'
+ ' "WatchList1": ["[email protected]"]'
+ ' },'
+ '}')
+
+ self.assertRaisesRegexp(r'In section "CC_RULES", the following definitions are not used and should be removed: WatchList1',
+ self._watch_list_parser.parse, watch_list)
+
+ def test_message_rule_with_undefined_defintion(self):
+ watch_list = (
+ '{'
+ ' "MESSAGE_RULES": {'
+ ' "WatchList1": ["The message."]'
+ ' },'
+ '}')
+
+ self.assertRaisesRegexp(r'In section "MESSAGE_RULES", the following definitions are not used and should be removed: WatchList1',
+ self._watch_list_parser.parse, watch_list)
+
+ def test_cc_rule_with_undefined_defintion_with_suggestion(self):
+ watch_list = (
+ '{'
+ ' "DEFINITIONS": {'
+ ' "WatchList1": {'
+ ' "filename": r".*\\MyFileName\\.cpp",'
+ ' },'
+ ' },'
+ ' "CC_RULES": {'
+ ' "WatchList": ["[email protected]"]'
+ ' },'
+ ' "MESSAGE_RULES": {'
+ ' "WatchList1": ["[email protected]"]'
+ ' },'
+ '}')
+
+ self.assertRaisesRegexp(r'In section "CC_RULES", the following definitions are not used and should be removed: WatchList\s*'
+ r'Perhaps it should be WatchList1\.',
+ self._watch_list_parser.parse, watch_list)
Modified: trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistrule.py (96389 => 96390)
--- trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistrule.py 2011-09-30 05:15:58 UTC (rev 96389)
+++ trunk/Tools/Scripts/webkitpy/common/watchlist/watchlistrule.py 2011-09-30 05:22:55 UTC (rev 96390)
@@ -30,11 +30,11 @@
class WatchListRule:
'''A rule with instructions to do when the rule is satisified.'''
def __init__(self, complex_definition, instructions):
- self._definitions_to_match = complex_definition.split('|')
+ self.definitions_to_match = complex_definition.split('|')
self._instructions = instructions
def match(self, matching_definitions):
- for test_definition in self._definitions_to_match:
+ for test_definition in self.definitions_to_match:
if test_definition in matching_definitions:
return True
return False
