Diff
Modified: trunk/Tools/ChangeLog (91070 => 91071)
--- trunk/Tools/ChangeLog 2011-07-15 15:34:28 UTC (rev 91070)
+++ trunk/Tools/ChangeLog 2011-07-15 15:51:37 UTC (rev 91071)
@@ -1,3 +1,21 @@
+2011-07-15 Dimitri Glazkov <[email protected]>
+
+ Remove the notion of TestExpectationLine.valid, start storing parsing errors in expectations themselves.
+ https://bugs.webkit.org/show_bug.cgi?id=64554
+
+ This moves us toward the world where errors are collected on the expectations, which allows us to
+ easily enumerate them, keep association with the point of origin, and freely pass TestExpectationLine instances around.
+
+ Also eliminate the validator idea, since validation is a context-dependent concept and has to be decoupled from initial
+ parsing.
+
+ Reviewed by Adam Barth.
+
+ * Scripts/webkitpy/layout_tests/models/test_expectations.py: Removed TestExpectation.valid, validator,
+ changed TestExpectationParser to collect errors in TestExpectationLine, refactored surrounding code.
+ * Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py: Changed tests to accommodate changes.
+ * Scripts/webkitpy/style/checkers/test_expectations_unittest.py: Ditto.
+
2011-07-15 Adam Roben <[email protected]>
Rename TestFailureBugForm to FailingTestsBugForm
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py (91070 => 91071)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py 2011-07-15 15:34:28 UTC (rev 91070)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py 2011-07-15 15:51:37 UTC (rev 91071)
@@ -143,7 +143,7 @@
@classmethod
def to_string(cls, expectation):
result = []
- if expectation.malformed:
+ if expectation.is_malformed():
result.append(expectation.comment)
else:
if expectation.name is None:
@@ -178,7 +178,6 @@
"""
expectation = TestExpectationLine()
- errors = []
comment_index = expectation_string.find("//")
if comment_index == -1:
comment_index = len(expectation_string)
@@ -187,41 +186,31 @@
remaining_string = re.sub(r"\s+", " ", expectation_string[:comment_index].strip())
if len(remaining_string) == 0:
- expectation.malformed = False
- expectation.valid = True
- return expectation, errors
+ return expectation
parts = remaining_string.split(':')
if len(parts) != 2:
- errors.append(("Missing a ':' in" if len(parts) < 2 else "Extraneous ':' in", "'" + expectation_string + "'"))
+ expectation.errors.append("Missing a ':'" if len(parts) < 2 else "Extraneous ':'")
else:
test_and_expectation = parts[1].split('=')
if len(test_and_expectation) != 2:
- errors.append(("Missing expectations in" if len(test_and_expectation) < 2 else "Extraneous '=' in", "'" + expectation_string + "'"))
+ expectation.errors.append("Missing expectations" if len(test_and_expectation) < 2 else "Extraneous '='")
- if len(errors) > 0:
+ if expectation.is_malformed():
expectation.comment = expectation_string
- expectation.malformed = True
- expectation.valid = False
else:
- expectation.malformed = False
- expectation.valid = True
expectation.modifiers = cls._split_space_separated(parts[0])
expectation.name = test_and_expectation[0].strip()
expectation.expectations = cls._split_space_separated(test_and_expectation[1])
- return expectation, errors
+ return expectation
@classmethod
- def parse_list(cls, expectations_string, validator):
+ def parse_list(cls, expectations_string):
"""Returns a list of TestExpectationLines, one for each line in expectations_string."""
expectations = []
- line_number = 0
for line in expectations_string.split("\n"):
- expectation, errors = cls.parse(line)
- line_number += 1
- expectation.valid = validator.validate(line_number, expectation, errors)
- expectations.append(expectation)
+ expectations.append(cls.parse(line))
return expectations
@classmethod
@@ -240,10 +229,10 @@
self.modifiers = []
self.expectations = []
self.comment = None
- # FIXME: Should valid and malformed be a single state flag? Probably not, since "malformed" is also "not valid".
- self.valid = False
- self.malformed = False
+ self.errors = []
+ def is_malformed(self):
+ return len(self.errors) > 0
# FIXME: Refactor to use TestExpectationLine as data item
# FIXME: Refactor API to be a proper CRUD.
@@ -576,14 +565,13 @@
# invalid ones.
self._all_expectations = {}
- self._matcher = ModifierMatcher(self._test_config)
- self._overrides_allowed = False
- self._expectations = TestExpectationParser.parse_list(expectations, self)
+ self._expectations = TestExpectationParser.parse_list(expectations)
+ self._add_expectations(self._expectations, overrides_allowed=False)
if overrides:
- self._overrides_allowed = True
- self._expectations += TestExpectationParser.parse_list(overrides, self)
- self._overrides_allowed = False
+ overrides_expectations = TestExpectationParser.parse_list(overrides)
+ self._add_expectations(overrides_expectations, overrides_allowed=True)
+ self._expectations += overrides_expectations
self._handle_any_read_errors()
self._process_tests_without_expectations()
@@ -686,7 +674,7 @@
def remove_rebaselined_tests(self, tests):
"""Returns a copy of the expectations with the tests removed."""
def without_rebaseline_modifier(expectation):
- return not (expectation.valid and expectation.name in tests and "rebaseline" in expectation.modifiers)
+ return not (not expectation.is_malformed() and expectation.name in tests and "rebaseline" in expectation.modifiers)
return TestExpectationSerializer.list_to_string(filter(without_rebaseline_modifier, self._expectations))
@@ -696,50 +684,52 @@
self._all_expectations[test].append(
ModifiersAndExpectations(options, expectations))
- def validate(self, lineno, expectation, syntax_errors):
- test_list_path = expectation.name
- options = expectation.modifiers
- expectations = expectation.expectations
- matcher = self._matcher
- overrides_allowed = self._overrides_allowed
+ def _add_expectations(self, expectation_list, overrides_allowed):
+ matcher = ModifierMatcher(self._test_config)
- for (message, source) in syntax_errors:
- self._add_error(lineno, message, source)
+ lineno = 0
+ for expectation in expectation_list:
+ lineno += 1
+ test_list_path = expectation.name
+ options = expectation.modifiers
+ expectations = expectation.expectations
- if not expectations:
- return False
+ for error in expectation.errors:
+ self._add_error(lineno, error, expectation.comment)
- self._add_to_all_expectations(test_list_path,
- " ".join(options).upper(),
- " ".join(expectations).upper())
+ if not expectations:
+ continue
- num_matches = self._check_options(matcher, options, lineno,
- test_list_path)
- if num_matches == ModifierMatcher.NO_MATCH:
- return False
+ self._add_to_all_expectations(test_list_path,
+ " ".join(options).upper(),
+ " ".join(expectations).upper())
- expectations = self._parse_expectations(expectations, lineno,
- test_list_path)
+ num_matches = self._check_options(matcher, options, lineno,
+ test_list_path)
+ if num_matches == ModifierMatcher.NO_MATCH:
+ continue
- self._check_options_against_expectations(options, expectations,
- lineno, test_list_path)
+ expectations = self._parse_expectations(expectations, lineno,
+ test_list_path)
- if self._check_path_does_not_exist(lineno, test_list_path):
- return False
+ self._check_options_against_expectations(options, expectations,
+ lineno, test_list_path)
- if not self._full_test_list:
- tests = [test_list_path]
- else:
- tests = self._expand_tests(test_list_path)
+ if self._check_path_does_not_exist(lineno, test_list_path):
+ continue
- modifiers = [o for o in options if o in self.MODIFIERS]
- # FIXME: Eliminate this awful error plumbing
- modifier_errors = self._model.add_tests(tests, expectations, test_list_path, lineno, modifiers, num_matches, options, overrides_allowed)
- for (message, source) in modifier_errors:
- self._add_error(lineno, message, source)
+ if not self._full_test_list:
+ tests = [test_list_path]
+ else:
+ tests = self._expand_tests(test_list_path)
- return True
+ modifiers = [o for o in options if o in self.MODIFIERS]
+ # FIXME: Eliminate this awful error plumbing
+ modifier_errors = self._model.add_tests(tests, expectations, test_list_path, lineno, modifiers, num_matches, options, overrides_allowed)
+ for (message, source) in modifier_errors:
+ self._add_error(lineno, message, source)
+
# FIXME: Remove, no longer used anywhere?
def _get_options_list(self, listString):
return [part.strip().lower() for part in listString.strip().split(' ')]
@@ -833,7 +823,7 @@
"""Reports an error that will prevent running the tests. Does not
immediately raise an exception because we'd like to aggregate all the
errors so they can all be printed out."""
- self._errors.append('Line:%s %s %s' % (lineno, msg, path))
+ self._errors.append("Line:%s %s %s" % (lineno, msg, path))
def _log_non_fatal_error(self, lineno, msg, path):
"""Reports an error that will not prevent running the tests. These are
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py (91070 => 91071)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py 2011-07-15 15:34:28 UTC (rev 91070)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py 2011-07-15 15:51:37 UTC (rev 91071)
@@ -195,7 +195,7 @@
except ParseError, e:
self.assertTrue(e.fatal)
exp_errors = [u"Line:1 Unrecognized option 'foo' failures/expected/text.html",
- u"Line:2 Missing expectations in 'SKIP : failures/expected/image.html'"]
+ u"Line:2 Missing expectations SKIP : failures/expected/image.html"]
self.assertEqual(str(e), '\n'.join(map(str, exp_errors)))
self.assertEqual(e.errors, exp_errors)
@@ -463,102 +463,79 @@
class TestExpectationParserTests(unittest.TestCase):
def test_blank(self):
- (expectation, errors) = TestExpectationParser.parse('')
- self.assertEqual(expectation.malformed, False)
- self.assertEqual(expectation.valid, True)
+ expectation = TestExpectationParser.parse('')
+ self.assertEqual(expectation.is_malformed(), False)
self.assertEqual(expectation.comment, None)
- self.assertEqual(len(errors), 0)
+ self.assertEqual(len(expectation.errors), 0)
def test_missing_colon(self):
- (expectation, errors) = TestExpectationParser.parse('Qux.')
- self.assertEqual(expectation.malformed, True)
- self.assertEqual(expectation.valid, False)
+ expectation = TestExpectationParser.parse('Qux.')
+ self.assertEqual(expectation.is_malformed(), True)
self.assertEqual(expectation.comment, 'Qux.')
- self.assertEqual(str(errors), '[("Missing a \':\' in", "\'Qux.\'")]')
+ self.assertEqual(str(expectation.errors), '["Missing a \':\'"]')
def test_extra_colon(self):
- (expectation, errors) = TestExpectationParser.parse('FOO : : bar')
- self.assertEqual(expectation.malformed, True)
- self.assertEqual(expectation.valid, False)
+ expectation = TestExpectationParser.parse('FOO : : bar')
+ self.assertEqual(expectation.is_malformed(), True)
self.assertEqual(expectation.comment, 'FOO : : bar')
- self.assertEqual(str(errors), '[("Extraneous \':\' in", "\'FOO : : bar\'")]')
+ self.assertEqual(str(expectation.errors), '["Extraneous \':\'"]')
def test_empty_comment(self):
- (expectation, errors) = TestExpectationParser.parse('//')
- self.assertEqual(expectation.malformed, False)
- self.assertEqual(expectation.valid, True)
+ expectation = TestExpectationParser.parse('//')
+ self.assertEqual(expectation.is_malformed(), False)
self.assertEqual(expectation.comment, '')
- self.assertEqual(len(errors), 0)
+ self.assertEqual(len(expectation.errors), 0)
def test_comment(self):
- (expectation, errors) = TestExpectationParser.parse('//Qux.')
- self.assertEqual(expectation.malformed, False)
- self.assertEqual(expectation.valid, True)
+ expectation = TestExpectationParser.parse('//Qux.')
+ self.assertEqual(expectation.is_malformed(), False)
self.assertEqual(expectation.comment, 'Qux.')
- self.assertEqual(len(errors), 0)
+ self.assertEqual(len(expectation.errors), 0)
def test_missing_equal(self):
- (expectation, errors) = TestExpectationParser.parse('FOO : bar')
- self.assertEqual(expectation.malformed, True)
- self.assertEqual(expectation.valid, False)
+ expectation = TestExpectationParser.parse('FOO : bar')
+ self.assertEqual(expectation.is_malformed(), True)
self.assertEqual(expectation.comment, 'FOO : bar')
- self.assertEqual(str(errors), '[(\'Missing expectations in\', "\'FOO : bar\'")]')
+ self.assertEqual(str(expectation.errors), "['Missing expectations\']")
def test_extra_equal(self):
- (expectation, errors) = TestExpectationParser.parse('FOO : bar = BAZ = Qux.')
- self.assertEqual(expectation.malformed, True)
- self.assertEqual(expectation.valid, False)
+ expectation = TestExpectationParser.parse('FOO : bar = BAZ = Qux.')
+ self.assertEqual(expectation.is_malformed(), True)
self.assertEqual(expectation.comment, 'FOO : bar = BAZ = Qux.')
- self.assertEqual(str(errors), '[("Extraneous \'=\' in", "\'FOO : bar = BAZ = Qux.\'")]')
+ self.assertEqual(str(expectation.errors), '["Extraneous \'=\'"]')
def test_valid(self):
- (expectation, errors) = TestExpectationParser.parse('FOO : bar = BAZ')
- self.assertEqual(expectation.malformed, False)
- self.assertEqual(expectation.valid, True)
+ expectation = TestExpectationParser.parse('FOO : bar = BAZ')
+ self.assertEqual(expectation.is_malformed(), False)
self.assertEqual(expectation.comment, None)
- self.assertEqual(len(errors), 0)
+ self.assertEqual(len(expectation.errors), 0)
def test_valid_with_comment(self):
- (expectation, errors) = TestExpectationParser.parse('FOO : bar = BAZ //Qux.')
- self.assertEqual(expectation.malformed, False)
- self.assertEqual(expectation.valid, True)
+ expectation = TestExpectationParser.parse('FOO : bar = BAZ //Qux.')
+ self.assertEqual(expectation.is_malformed(), False)
self.assertEqual(expectation.comment, 'Qux.')
self.assertEqual(str(expectation.modifiers), '[\'foo\']')
self.assertEqual(str(expectation.expectations), '[\'baz\']')
- self.assertEqual(len(errors), 0)
+ self.assertEqual(len(expectation.errors), 0)
def test_valid_with_multiple_modifiers(self):
- (expectation, errors) = TestExpectationParser.parse('FOO1 FOO2 : bar = BAZ //Qux.')
- self.assertEqual(expectation.malformed, False)
- self.assertEqual(expectation.valid, True)
+ expectation = TestExpectationParser.parse('FOO1 FOO2 : bar = BAZ //Qux.')
+ self.assertEqual(expectation.is_malformed(), False)
self.assertEqual(expectation.comment, 'Qux.')
self.assertEqual(str(expectation.modifiers), '[\'foo1\', \'foo2\']')
self.assertEqual(str(expectation.expectations), '[\'baz\']')
- self.assertEqual(len(errors), 0)
+ self.assertEqual(len(expectation.errors), 0)
- def test_line_number_increment(self):
- validator = TestValidator()
- expectations = TestExpectationParser.parse_list('// Bar\nFOO : bar = BAZ\n\n', validator)
- self.assertEqual(str(validator.line_numbers), '[1, 2, 3, 4]')
- def test_validator_feedback(self):
- validator = TestValidator()
- expectations = TestExpectationParser.parse_list('FOO : bar1 = BAZ\nFOO : bar2 = BAZ\nFOO : bar3 = BAZ', validator)
- line_number = 0
- for expectation in expectations:
- line_number += 1
- self.assertEqual(line_number % 2 == 0, expectation.valid)
-
-
class TestExpectationSerializerTests(unittest.TestCase):
def assert_round_trip(self, in_string, expected_string=None):
- (expectation, _) = TestExpectationParser.parse(in_string)
+ expectation = TestExpectationParser.parse(in_string)
if expected_string is None:
expected_string = in_string
self.assertEqual(expected_string, TestExpectationSerializer.to_string(expectation))
def assert_list_round_trip(self, in_string, expected_string=None):
- expectations = TestExpectationParser.parse_list(in_string, TestValidator())
+ expectations = TestExpectationParser.parse_list(in_string)
if expected_string is None:
expected_string = in_string
self.assertEqual(expected_string, TestExpectationSerializer.list_to_string(expectations))
@@ -581,7 +558,7 @@
self.assert_to_string(expectation, 'FOO : bar = BAZ1 BAZ2 //Qux.')
expectation.modifiers = ['foo1', 'foO2']
self.assert_to_string(expectation, 'FOO1 FOO2 : bar = BAZ1 BAZ2 //Qux.')
- expectation.malformed = True
+ expectation.errors.append('Oh the horror.')
self.assert_to_string(expectation, 'Qux.')
def test_string_roundtrip(self):
@@ -625,14 +602,5 @@
self.assert_round_trip('FOO : bar = BAZ // Qux.', 'FOO : bar = BAZ // Qux.')
self.assert_round_trip('FOO : bar = BAZ // Qux.', 'FOO : bar = BAZ // Qux.')
-
-class TestValidator:
- def __init__(self):
- self.line_numbers = []
-
- def validate(self, line_number, expectation, errors):
- self.line_numbers.append(line_number)
- return line_number % 2 == 0
-
if __name__ == '__main__':
unittest.main()
Modified: trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py (91070 => 91071)
--- trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py 2011-07-15 15:34:28 UTC (rev 91070)
+++ trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py 2011-07-15 15:51:37 UTC (rev 91071)
@@ -122,7 +122,7 @@
def test_expectation_errors(self):
self.assert_lines_lint(
["missing expectations"],
- "Missing a ':' in 'missing expectations' [test/expectations] [5]")
+ "Missing a ':' missing expectations [test/expectations] [5]")
self.assert_lines_lint(
["SLOW : passes/text.html = TIMEOUT"],
"A test can not be both SLOW and TIMEOUT. "
