check-webkit-style of the chromium test_expectations.txt file takes too long
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Jan 2012 20:10:27 +0000 (20:10 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Jan 2012 20:10:27 +0000 (20:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=76745

Reviewed by Dimitri Glazkov.

When in lint mode, have TestExpectations test all configurations instead
of looping over each configuration. This also has the benefit of making
the error output considerably more concise.

Also, got rid of the double-printing of errors when linting through check-webkit-style.

* Scripts/webkitpy/layout_tests/controllers/manager.py:
(Manager.lint):
* Scripts/webkitpy/layout_tests/models/test_expectations.py:
(TestExpectations._report_errors):
(TestExpectations._add_expectations):
* Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:
(test_parse_error_nonfatal):
(test_error_on_different_platform):
* Scripts/webkitpy/style/checkers/test_expectations.py:
(TestExpectationsChecker.check_test_expectations):
(TestExpectationsChecker.check):
* Scripts/webkitpy/style/checkers/test_expectations_unittest.py:
(TestExpectationsTestCase.test_determine_port_from_exepectations_path):
(TestExpectationsTestCase.assert_lines_lint):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@105770 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py
Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py
Tools/Scripts/webkitpy/style/checkers/test_expectations.py
Tools/Scripts/webkitpy/style/checkers/test_expectations_unittest.py

index 28295e9..fc9ed8c 100644 (file)
@@ -1,3 +1,31 @@
+2012-01-20  Ojan Vafai  <ojan@chromium.org>
+
+        check-webkit-style of the chromium test_expectations.txt file takes too long
+        https://bugs.webkit.org/show_bug.cgi?id=76745
+
+        Reviewed by Dimitri Glazkov.
+
+        When in lint mode, have TestExpectations test all configurations instead
+        of looping over each configuration. This also has the benefit of making
+        the error output considerably more concise.
+
+        Also, got rid of the double-printing of errors when linting through check-webkit-style.
+
+        * Scripts/webkitpy/layout_tests/controllers/manager.py:
+        (Manager.lint):
+        * Scripts/webkitpy/layout_tests/models/test_expectations.py:
+        (TestExpectations._report_errors):
+        (TestExpectations._add_expectations):
+        * Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:
+        (test_parse_error_nonfatal):
+        (test_error_on_different_platform):
+        * Scripts/webkitpy/style/checkers/test_expectations.py:
+        (TestExpectationsChecker.check_test_expectations):
+        (TestExpectationsChecker.check):
+        * Scripts/webkitpy/style/checkers/test_expectations_unittest.py:
+        (TestExpectationsTestCase.test_determine_port_from_exepectations_path):
+        (TestExpectationsTestCase.assert_lines_lint):
+
 2012-01-24  Ryosuke Niwa  <rniwa@webkit.org>
 
         Another build fix attempt after r105543.
index 24d1b73..cf35f24 100644 (file)
@@ -348,31 +348,24 @@ class Manager(object):
         return path
 
     def lint(self):
-        lint_failed = False
-        for test_configuration in self._port.all_test_configurations():
-            try:
-                self.lint_expectations(test_configuration)
-            except test_expectations.ParseError:
-                lint_failed = True
-                self._printer.write("")
-
-        if lint_failed:
+        try:
+            test_expectations.TestExpectations(
+                self._port,
+                None,
+                self._port.test_expectations(),
+                self._port.test_configuration(),
+                self._options.lint_test_files,
+                self._port.test_expectations_overrides())
+        except test_expectations.ParseError, err:
+            for error in err.errors:
+                self._printer.write(error)
+            self._printer.write("")
             _log.error("Lint failed.")
             return -1
 
         _log.info("Lint succeeded.")
         return 0
 
-    def lint_expectations(self, config):
-        port = self._port
-        test_expectations.TestExpectations(
-            port,
-            None,
-            port.test_expectations(),
-            config,
-            self._options.lint_test_files,
-            port.test_expectations_overrides())
-
     def _is_http_test(self, test):
         return self.HTTP_SUBDIR in test or self.WEBSOCKET_SUBDIR in test
 
index f9e0ff7..cfa2cce 100644 (file)
@@ -792,19 +792,13 @@ class TestExpectations(object):
         for warning in self._skipped_tests_warnings:
             warnings.append(warning)
 
-        if len(errors) or len(warnings):
+        if errors or warnings:
             test_expectation_path = self._port.path_to_test_expectations_file()
-            failure_title = "FAILURES FOR %s in %s" % (str(self._test_config), test_expectation_path)
-            _log.error(failure_title)
+            failure_title = "FAILURES IN %s" % test_expectation_path
 
-            for error in errors:
-                _log.error(error)
-            for warning in warnings:
-                _log.error(warning)
-
-            if len(errors):
+            if errors:
                 raise ParseError(fatal=True, errors=[failure_title] + errors)
-            if len(warnings):
+            if warnings:
                 self._has_warnings = True
                 if self._is_lint_mode:
                     raise ParseError(fatal=False, errors=[failure_title] + warnings)
@@ -830,7 +824,7 @@ class TestExpectations(object):
             if not expectation_line.expectations:
                 continue
 
-            if self._test_config in expectation_line.matching_configurations:
+            if self._is_lint_mode or self._test_config in expectation_line.matching_configurations:
                 self._model.add_expectation_line(expectation_line, overrides_allowed)
 
     def _add_skipped_tests(self, tests_to_skip):
index ba687ab..e9d6620 100644 (file)
@@ -199,7 +199,7 @@ SKIP : failures/expected/image.html""")
             self.assertFalse(True, "ParseError wasn't raised")
         except ParseError, e:
             self.assertTrue(e.fatal)
-            exp_errors = [u"FAILURES FOR %s in %s" % (self._port.test_configuration(), self._port.path_to_test_expectations_file()),
+            exp_errors = [u"FAILURES IN %s" % self._port.path_to_test_expectations_file(),
                           u"Line:1 Unrecognized modifier 'foo' failures/expected/text.html",
                           u"Line:2 Missing expectations SKIP : failures/expected/image.html"]
             self.assertEqual(str(e), '\n'.join(map(str, exp_errors)))
@@ -207,16 +207,33 @@ SKIP : failures/expected/image.html""")
 
     def test_parse_error_nonfatal(self):
         try:
-            self.parse_exp('SKIP : failures/expected/text.html = TEXT',
-                           is_lint_mode=True)
+            self.parse_exp('SKIP : failures/expected/text.html = TEXT', is_lint_mode=True)
             self.assertFalse(True, "ParseError wasn't raised")
         except ParseError, e:
             self.assertFalse(e.fatal)
-            exp_errors = [u'FAILURES FOR %s in %s' % (self._port.test_configuration(), self._port.path_to_test_expectations_file()),
+            exp_errors = [u'FAILURES IN %s' % self._port.path_to_test_expectations_file(),
                           u'Line:1 Test lacks BUG modifier. failures/expected/text.html']
             self.assertEqual(str(e), '\n'.join(map(str, exp_errors)))
             self.assertEqual(e.errors, exp_errors)
 
+    def test_error_on_different_platform(self):
+        # parse_exp uses a Windows port. Assert errors on Mac show up in lint mode.
+        self.assertRaises(ParseError, self.parse_exp,
+            'BUG_TEST MAC : failures/expected/text.html = TEXT\nBUG_TEST MAC : failures/expected/text.html = TEXT',
+            is_lint_mode=True)
+
+    def test_error_on_different_build_type(self):
+        # parse_exp uses a Release port. Assert errors on DEBUG show up in lint mode.
+        self.assertRaises(ParseError, self.parse_exp,
+            'BUG_TEST DEBUG : failures/expected/text.html = TEXT\nBUG_TEST DEBUG : failures/expected/text.html = TEXT',
+            is_lint_mode=True)
+
+    def test_error_on_different_graphics_type(self):
+        # parse_exp uses a CPU port. Assert errors on GPU show up in lint mode.
+        self.assertRaises(ParseError, self.parse_exp,
+            'BUG_TEST GPU : failures/expected/text.html = TEXT\nBUG_TEST GPU : failures/expected/text.html = TEXT',
+            is_lint_mode=True)
+
     def test_overrides(self):
         self.parse_exp("BUG_EXP: failures/expected/text.html = TEXT",
                        "BUG_OVERRIDE : failures/expected/text.html = IMAGE")
index a141415..9d1941d 100644 (file)
@@ -90,13 +90,13 @@ class TestExpectationsChecker(object):
     def _handle_error_message(self, lineno, message, confidence):
         pass
 
-    def check_test_expectations(self, expectations_str, test_configuration, tests=None, overrides=None):
+    def check_test_expectations(self, expectations_str, tests=None, overrides=None):
         err = None
         expectations = None
         try:
             expectations = test_expectations.TestExpectations(
                 port=self._port_obj, expectations=expectations_str, tests=tests,
-                test_config=test_configuration,
+                test_config=self._port_obj.test_configuration(),
                 is_lint_mode=True, overrides=overrides)
         except test_expectations.ParseError, error:
             err = error
@@ -118,10 +118,8 @@ class TestExpectationsChecker(object):
     def check(self, lines):
         overrides = self._port_obj.test_expectations_overrides()
         expectations = '\n'.join(lines)
-        for test_configuration in self._port_obj.all_test_configurations():
-            self.check_test_expectations(expectations_str=expectations,
-                                         test_configuration=test_configuration,
-                                         tests=None,
-                                         overrides=overrides)
+        self.check_test_expectations(expectations_str=expectations,
+                                     tests=None,
+                                     overrides=overrides)
         # Warn tabs in lines as well
         self.check_tabs(lines)
index d7178b6..92d1fe6 100644 (file)
@@ -76,52 +76,11 @@ class TestExpectationsTestCase(unittest.TestCase):
         self._expect_port_for_expectations_path(None, "/")
         self._expect_port_for_expectations_path("ChromiumMacPort", "/mock-checkout/LayoutTests/chromium-mac/test_expectations.txt")
 
-    def test_check_covers_all_configurations(self):
-        checker = TestExpectationsChecker('test/test_expectations.txt', self._error_collector, host=MockHost())
-        output = []
-
-        def mock_check_test_expectations(expectations_str, test_configuration, tests, overrides, output=output):
-            output.append(str(test_configuration))
-        checker.check_test_expectations = mock_check_test_expectations
-        checker.check(lines="")
-
-        expected_output = """<leopard, x86, debug, cpu>
-<leopard, x86, debug, gpu>
-<leopard, x86, release, cpu>
-<leopard, x86, release, gpu>
-<snowleopard, x86, debug, cpu>
-<snowleopard, x86, debug, gpu>
-<snowleopard, x86, release, cpu>
-<snowleopard, x86, release, gpu>
-<xp, x86, debug, cpu>
-<xp, x86, debug, gpu>
-<xp, x86, release, cpu>
-<xp, x86, release, gpu>
-<vista, x86, debug, cpu>
-<vista, x86, debug, gpu>
-<vista, x86, release, cpu>
-<vista, x86, release, gpu>
-<win7, x86, debug, cpu>
-<win7, x86, debug, gpu>
-<win7, x86, release, cpu>
-<win7, x86, release, gpu>
-<lucid, x86, debug, cpu>
-<lucid, x86, debug, gpu>
-<lucid, x86, release, cpu>
-<lucid, x86, release, gpu>
-<lucid, x86_64, debug, cpu>
-<lucid, x86_64, debug, gpu>
-<lucid, x86_64, release, cpu>
-<lucid, x86_64, release, gpu>"""
-
-        self.assertEqual("\n".join(output), expected_output)
-
     def assert_lines_lint(self, lines, expected):
         self._error_collector.reset_errors()
         checker = TestExpectationsChecker('test/test_expectations.txt',
                                           self._error_collector, host=MockHost())
         checker.check_test_expectations(expectations_str='\n'.join(lines),
-                                        test_configuration=checker._port_obj.test_configuration(),
                                         tests=[self._test_file],
                                         overrides=None)
         checker.check_tabs(lines)