Title: [137355] trunk/Tools
- Revision
- 137355
- Author
- [email protected]
- Date
- 2012-12-11 13:42:05 -0800 (Tue, 11 Dec 2012)
Log Message
check-webkit-style complaining about TestExpectations lines a patch didn't modify
https://bugs.webkit.org/show_bug.cgi?id=104687
Reviewed by Ojan Vafai.
Way back in bug 60466 / r86165, we changed the style checker
for TestExpectations to ensure that the entire file linted, not
just the lines modified by a patch, in order to ensure that a
new line didn't cause errors elsewhere in the file (e.g., by
introducing duplicate lines). At the time, errors in the
expectations files were fatal and we didn't have a separate step
on the bots that would lint the files to ensure things were still
okay, so this was probably the right thing to do, but it could
cause some patches to fail the style check even though the patches
themselves were fine.
However, now errors aren't always fatal, and we have a separate build
step to lint the files. So, we should probably not punish the
patches and the style checker should filter out "unrelated" errors.
In theory we should be comparing the output pre- and post-patch
to filter out pre-existing errors, but I don't think the style
checker has that ability.
* Scripts/webkitpy/style/checkers/test_expectations.py:
(TestExpectationsChecker.__init__):
* Scripts/webkitpy/style/checkers/test_expectations_unittest.py:
(TestExpectationsTestCase.assert_lines_lint):
Modified Paths
Diff
Modified: trunk/Tools/ChangeLog (137354 => 137355)
--- trunk/Tools/ChangeLog 2012-12-11 21:40:13 UTC (rev 137354)
+++ trunk/Tools/ChangeLog 2012-12-11 21:42:05 UTC (rev 137355)
@@ -1,5 +1,36 @@
2012-12-11 Dirk Pranke <[email protected]>
+ check-webkit-style complaining about TestExpectations lines a patch didn't modify
+ https://bugs.webkit.org/show_bug.cgi?id=104687
+
+ Reviewed by Ojan Vafai.
+
+ Way back in bug 60466 / r86165, we changed the style checker
+ for TestExpectations to ensure that the entire file linted, not
+ just the lines modified by a patch, in order to ensure that a
+ new line didn't cause errors elsewhere in the file (e.g., by
+ introducing duplicate lines). At the time, errors in the
+ expectations files were fatal and we didn't have a separate step
+ on the bots that would lint the files to ensure things were still
+ okay, so this was probably the right thing to do, but it could
+ cause some patches to fail the style check even though the patches
+ themselves were fine.
+
+ However, now errors aren't always fatal, and we have a separate build
+ step to lint the files. So, we should probably not punish the
+ patches and the style checker should filter out "unrelated" errors.
+
+ In theory we should be comparing the output pre- and post-patch
+ to filter out pre-existing errors, but I don't think the style
+ checker has that ability.
+
+ * Scripts/webkitpy/style/checkers/test_expectations.py:
+ (TestExpectationsChecker.__init__):
+ * Scripts/webkitpy/style/checkers/test_expectations_unittest.py:
+ (TestExpectationsTestCase.assert_lines_lint):
+
+2012-12-11 Dirk Pranke <[email protected]>
+
nrwt isn't rejecting unrecognized expectations
https://bugs.webkit.org/show_bug.cgi?id=104701
Modified: trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations.py (137354 => 137355)
--- trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations.py 2012-12-11 21:40:13 UTC (rev 137354)
+++ trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations.py 2012-12-11 21:42:05 UTC (rev 137355)
@@ -62,7 +62,6 @@
def __init__(self, file_path, handle_style_error, host=None):
self._file_path = file_path
self._handle_style_error = handle_style_error
- self._handle_style_error.turn_off_line_filtering()
self._tab_checker = TabChecker(file_path, handle_style_error)
# FIXME: host should be a required parameter, not an optional one.
Modified: trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py (137354 => 137355)
--- trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py 2012-12-11 21:40:13 UTC (rev 137354)
+++ trunk/Tools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py 2012-12-11 21:42:05 UTC (rev 137355)
@@ -107,8 +107,13 @@
self.assertEqual(expected_output, self._error_collector.get_errors())
else:
self.assertNotEquals('', self._error_collector.get_errors())
- self.assertTrue(self._error_collector.turned_off_filtering)
+ # Note that a patch might change a line that introduces errors elsewhere, but we
+ # don't want to lint the whole file (it can unfairly punish patches for pre-existing errors).
+ # We rely on a separate lint-webkitpy step on the bots to keep the whole file okay.
+ # FIXME: See https://bugs.webkit.org/show_bug.cgi?id=104712 .
+ self.assertFalse(self._error_collector.turned_off_filtering)
+
def test_valid_expectations(self):
self.assert_lines_lint(["crbug.com/1234 [ Mac ] passes/text.html [ Pass Failure ]"], should_pass=True)
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo/webkit-changes
