2010-12-09 Dirk Pranke <dpranke@chromium.org>
authordpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Dec 2010 23:22:15 +0000 (23:22 +0000)
committerdpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Dec 2010 23:22:15 +0000 (23:22 +0000)
        Reviewed by Ojan Vafai.

        Make new-run-webkit-tests --lint-test-files log slightly better
        messages and return -1 if lint fails. Remove the
        'suppress_errors' keyword param to the TestExpectationsFile
        class, and clean up logging and exception raising for error
        handling. Also add more unit tests and clean up the unit test code a bit.

        https://bugs.webkit.org/show_bug.cgi?id=50205

        * Scripts/webkitpy/layout_tests/run_webkit_tests.py:
        * Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:
        * Scripts/webkitpy/layout_tests/layout_package/test_expectations.py
        * Scripts/webkitpy/layout_tests/layout_package/test_expectations_unittest.py
        * Scripts/webkitpy/layout_tests/port/chromium.py:
        * Scripts/webkitpy/style/checkers/test_expectations.py:
        * Scripts/webkitpy/style/checkers/test_expectations_unittest.py:

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

WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_expectations_unittest.py
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py
WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py
WebKitTools/Scripts/webkitpy/style/checkers/test_expectations.py

index cf5546d46553f7ba08ab85d36701dacf32422b96..79ff508bdedec774810955ca12eaeb8623d1a9bb 100644 (file)
@@ -1,3 +1,23 @@
+2010-12-09  Dirk Pranke  <dpranke@chromium.org>
+
+        Reviewed by Ojan Vafai.
+
+        Make new-run-webkit-tests --lint-test-files log slightly better
+        messages and return -1 if lint fails. Remove the
+        'suppress_errors' keyword param to the TestExpectationsFile
+        class, and clean up logging and exception raising for error
+        handling. Also add more unit tests and clean up the unit test code a bit.
+
+        https://bugs.webkit.org/show_bug.cgi?id=50205
+
+        * Scripts/webkitpy/layout_tests/run_webkit_tests.py:
+        * Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:
+        * Scripts/webkitpy/layout_tests/layout_package/test_expectations.py
+        * Scripts/webkitpy/layout_tests/layout_package/test_expectations_unittest.py
+        * Scripts/webkitpy/layout_tests/port/chromium.py:
+        * Scripts/webkitpy/style/checkers/test_expectations.py:
+        * Scripts/webkitpy/style/checkers/test_expectations_unittest.py:
+
 2010-12-09  Tony Chang  <tony@chromium.org>
 
         Unreviewed, fix for windows code.  We were catching the wrong
index 67873a8cc0ab3179c7de35d5c9816ce340d34cb9..7e1e53acde9c6a4f4955d7ba3950100842a99d0e 100644 (file)
@@ -190,6 +190,18 @@ def strip_comments(line):
         return line
 
 
+class ParseError(Exception):
+    def __init__(self, fatal, errors):
+        self.fatal = fatal
+        self.errors = errors
+
+    def __str__(self):
+        return '\n'.join(map(str, self.errors))
+
+    def __repr__(self):
+        return 'ParseError(fatal=%s, errors=%s)' % (fatal, errors)
+
+
 class ModifiersAndExpectations:
     """A holder for modifiers and expectations on a test that serializes to
     JSON."""
@@ -293,7 +305,7 @@ class TestExpectationsFile:
                     'flaky': FLAKY}
 
     def __init__(self, port, expectations, full_test_list, test_platform_name,
-        is_debug_mode, is_lint_mode, suppress_errors=False, overrides=None):
+        is_debug_mode, is_lint_mode, overrides=None):
         """
         expectations: Contents of the expectations file
         full_test_list: The list of all tests to be run pending processing of
@@ -303,7 +315,6 @@ class TestExpectationsFile:
             port.test_platform_name() when is_lint_mode is True.
         is_debug_mode: Whether we testing a test_shell built debug mode.
         is_lint_mode: Whether this is just linting test_expecatations.txt.
-        suppress_errors: Whether to suppress lint errors.
         overrides: test expectations that are allowed to override any
             entries in |expectations|. This is used by callers
             that need to manage two sets of expectations (e.g., upstream
@@ -317,7 +328,6 @@ class TestExpectationsFile:
         self._is_debug_mode = is_debug_mode
         self._is_lint_mode = is_lint_mode
         self._overrides = overrides
-        self._suppress_errors = suppress_errors
         self._errors = []
         self._non_fatal_errors = []
 
@@ -362,8 +372,7 @@ class TestExpectationsFile:
         self._process_tests_without_expectations()
 
     def _handle_any_read_errors(self):
-        if not self._suppress_errors and (
-            len(self._errors) or len(self._non_fatal_errors)):
+        if len(self._errors) or len(self._non_fatal_errors):
             if self._is_debug_mode:
                 build_type = 'DEBUG'
             else:
@@ -372,12 +381,15 @@ class TestExpectationsFile:
             _log.error("FAILURES FOR PLATFORM: %s, BUILD_TYPE: %s" %
                        (self._test_platform_name.upper(), build_type))
 
+            for error in self._errors:
+                _log.error(error)
             for error in self._non_fatal_errors:
                 _log.error(error)
-            _log.error('')
 
             if len(self._errors):
-                raise SyntaxError('\n'.join(map(str, self._errors)))
+                raise ParseError(fatal=True, errors=self._errors)
+            if len(self._non_fatal_errors) and self._is_lint_mode:
+                raise ParseError(fatal=False, errors=self._non_fatal_errors)
 
     def _process_tests_without_expectations(self):
         expectations = set([PASS])
@@ -835,7 +847,7 @@ class TestExpectationsFile:
         """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('\nLine:%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
index 55eaf994b8d257c48b994c30096db1072e26c90d..e04ac3161e2a9ee18984bb4a2769f7f3d6a3fb3e 100644 (file)
@@ -196,19 +196,42 @@ BUGX WONTFIX : failures/expected = IMAGE
                                                       include_skips=False)
         self.assertEqual(s, set([]))
 
+    def test_parse_error_fatal(self):
+        try:
+            self.parse_exp("""FOO : failures/expected/text.html = TEXT
+SKIP : failures/expected/image.html""")
+            self.assertFalse(True, "ParseError wasn't raised")
+        except ParseError, e:
+            self.assertTrue(e.fatal)
+            exp_errors = [u'Line:1 Invalid modifier for test: foo failures/expected/text.html',
+                          u"Line:2 Missing expectations. [' failures/expected/image.html']"]
+            self.assertEqual(str(e), '\n'.join(map(str, exp_errors)))
+            self.assertEqual(e.errors, exp_errors)
+
+    def test_parse_error_nonfatal(self):
+        try:
+            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'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_syntax_missing_expectation(self):
         # This is missing the expectation.
-        self.assertRaises(SyntaxError, self.parse_exp,
+        self.assertRaises(ParseError, self.parse_exp,
                           'BUG_TEST: failures/expected/text.html',
                           is_debug_mode=True)
 
     def test_syntax_invalid_option(self):
-        self.assertRaises(SyntaxError, self.parse_exp,
+        self.assertRaises(ParseError, self.parse_exp,
                           'BUG_TEST FOO: failures/expected/text.html = PASS')
 
     def test_syntax_invalid_expectation(self):
         # This is missing the expectation.
-        self.assertRaises(SyntaxError, self.parse_exp,
+        self.assertRaises(ParseError, self.parse_exp,
                           'BUG_TEST: failures/expected/text.html = FOO')
 
     def test_syntax_missing_bugid(self):
@@ -219,21 +242,21 @@ BUGX WONTFIX : failures/expected = IMAGE
 
     def test_semantic_slow_and_timeout(self):
         # A test cannot be SLOW and expected to TIMEOUT.
-        self.assertRaises(SyntaxError, self.parse_exp,
+        self.assertRaises(ParseError, self.parse_exp,
             'BUG_TEST SLOW : failures/expected/timeout.html = TIMEOUT')
 
     def test_semantic_rebaseline(self):
         # Can't lint a file w/ 'REBASELINE' in it.
-        self.assertRaises(SyntaxError, self.parse_exp,
+        self.assertRaises(ParseError, self.parse_exp,
             'BUG_TEST REBASELINE : failures/expected/text.html = TEXT',
             is_lint_mode=True)
 
     def test_semantic_duplicates(self):
-        self.assertRaises(SyntaxError, self.parse_exp, """
+        self.assertRaises(ParseError, self.parse_exp, """
 BUG_TEST : failures/expected/text.html = TEXT
 BUG_TEST : failures/expected/text.html = IMAGE""")
 
-        self.assertRaises(SyntaxError, self.parse_exp,
+        self.assertRaises(ParseError, self.parse_exp,
             self.get_basic_expectations(), """
 BUG_TEST : failures/expected/text.html = TEXT
 BUG_TEST : failures/expected/text.html = IMAGE""")
index 8fe685af0669eb34afe8f94c56fe3ff3cd9536c7..4523700331524563be84c7fa845cc5c0e9aaefe2 100644 (file)
@@ -285,7 +285,7 @@ class ChromiumPort(base.Port):
 
         expectations = test_expectations.TestExpectations(
             self, all_test_files, expectations_str, test_platform_name,
-            is_debug_mode, is_lint_mode=True, overrides=overrides_str)
+            is_debug_mode, is_lint_mode=False, overrides=overrides_str)
         tests_dir = self.layout_tests_dir()
         return [self.relative_test_filename(test)
                 for test in expectations.get_tests_with_result_type(test_expectations.SKIP)]
index f3b1da0646d57f8691cb06ae6507841e6fbce54e..ba55f16991f462060dd17ec13c34f39ca51bf1ce 100755 (executable)
@@ -284,15 +284,27 @@ class TestRunner:
         return path
 
     def lint(self):
+        lint_failed = False
+
         # Creating the expecations for each platform/configuration pair does
         # all the test list parsing and ensures it's correct syntax (e.g. no
         # dupes).
         for platform_name in self._port.test_platform_names():
-            self.parse_expectations(platform_name, is_debug_mode=True)
-            self.parse_expectations(platform_name, is_debug_mode=False)
+            try:
+                self.parse_expectations(platform_name, is_debug_mode=True)
+            except test_expectations.ParseError:
+                lint_failed = True
+            try:
+                self.parse_expectations(platform_name, is_debug_mode=False)
+            except test_expectations.ParseError:
+                lint_failed = True
+
         self._printer.write("")
-        _log.info("If there are no fail messages, errors or exceptions, "
-                  "then the lint succeeded.")
+        if lint_failed:
+            _log.error("Lint failed.")
+            return -1
+
+        _log.info("Lint succeeded.")
         return 0
 
     def parse_expectations(self, test_platform_name, is_debug_mode):
@@ -304,19 +316,14 @@ class TestRunner:
         else:
             test_files = self._test_files
 
-        try:
-            expectations_str = self._port.test_expectations()
-            overrides_str = self._port.test_expectations_overrides()
-            self._expectations = test_expectations.TestExpectations(
-                self._port, test_files, expectations_str, test_platform_name,
-                is_debug_mode, self._options.lint_test_files,
-                overrides=overrides_str)
-            return self._expectations
-        except SyntaxError, err:
-            if self._options.lint_test_files:
-                print str(err)
-            else:
-                raise err
+        expectations_str = self._port.test_expectations()
+        overrides_str = self._port.test_expectations_overrides()
+        self._expectations = test_expectations.TestExpectations(
+            self._port, test_files, expectations_str, test_platform_name,
+            is_debug_mode, self._options.lint_test_files,
+            overrides=overrides_str)
+        return self._expectations
+
 
     def prepare_lists_and_print_output(self):
         """Create appropriate subsets of test lists and returns a
index 6bb741a57c91c0b0e264cfde7d6410cbf583966a..810f787d6f6bfcafc777694172f158b420a70df6 100644 (file)
@@ -64,10 +64,13 @@ class MockUser():
         self.url = url
 
 
-def passing_run(extra_args=None, port_obj=None, record_results=False,
-                tests_included=False):
+def parse_args(extra_args=None, record_results=False, tests_included=False,
+               print_nothing=True):
     extra_args = extra_args or []
-    args = ['--print', 'nothing']
+    if print_nothing:
+        args = ['--print', 'nothing']
+    else:
+        args = []
     if not '--platform' in extra_args:
         args.extend(['--platform', 'test'])
     if not record_results:
@@ -81,7 +84,13 @@ def passing_run(extra_args=None, port_obj=None, record_results=False,
                      'http/tests',
                      'websocket/tests',
                      'failures/expected/*'])
-    options, parsed_args = run_webkit_tests.parse_args(args)
+    return run_webkit_tests.parse_args(args)
+
+
+def passing_run(extra_args=None, port_obj=None, record_results=False,
+                tests_included=False):
+    options, parsed_args = parse_args(extra_args, record_results,
+                                      tests_included)
     if not port_obj:
         port_obj = port.get(port_name=options.platform, options=options,
                             user=MockUser())
@@ -90,27 +99,24 @@ def passing_run(extra_args=None, port_obj=None, record_results=False,
 
 
 def logging_run(extra_args=None, port_obj=None, tests_included=False):
-    extra_args = extra_args or []
-    args = ['--no-record-results']
-    if not '--platform' in extra_args:
-        args.extend(['--platform', 'test'])
-    if not '--child-processes' in extra_args:
-        args.extend(['--worker-model', 'inline'])
-    args.extend(extra_args)
-    if not tests_included:
-        args.extend(['passes',
-                     'http/tests',
-                     'websocket/tests',
-                     'failures/expected/*'])
+    options, parsed_args = parse_args(extra_args=extra_args,
+                                      record_results=False,
+                                      tests_included=tests_included,
+                                      print_nothing=False)
+    user = MockUser()
+    if not port_obj:
+        port_obj = port.get(port_name=options.platform, options=options,
+                            user=user)
+
+    res, buildbot_output, regular_output = run_and_capture(port_obj, options,
+                                                           parsed_args)
+    return (res, buildbot_output, regular_output, user)
 
+
+def run_and_capture(port_obj, options, parsed_args):
     oc = outputcapture.OutputCapture()
     try:
         oc.capture_output()
-        options, parsed_args = run_webkit_tests.parse_args(args)
-        user = MockUser()
-        if not port_obj:
-            port_obj = port.get(port_name=options.platform, options=options,
-                                user=user)
         buildbot_output = array_stream.ArrayStream()
         regular_output = array_stream.ArrayStream()
         res = run_webkit_tests.run(port_obj, options, parsed_args,
@@ -118,22 +124,17 @@ def logging_run(extra_args=None, port_obj=None, tests_included=False):
                                    regular_output=regular_output)
     finally:
         oc.restore_output()
-    return (res, buildbot_output, regular_output, user)
+    return (res, buildbot_output, regular_output)
 
 
 def get_tests_run(extra_args=None, tests_included=False, flatten_batches=False):
     extra_args = extra_args or []
-    args = [
-        '--print', 'nothing',
-        '--platform', 'test',
-        '--no-record-results',
-        '--worker-model', 'inline']
-    args.extend(extra_args)
     if not tests_included:
         # Not including http tests since they get run out of order (that
         # behavior has its own test, see test_get_test_file_queue)
-        args.extend(['passes', 'failures'])
-    options, parsed_args = run_webkit_tests.parse_args(args)
+        extra_args = ['passes', 'failures'] + extra_args
+    options, parsed_args = parse_args(extra_args, tests_included=True)
+
     user = MockUser()
 
     test_batches = []
@@ -163,13 +164,14 @@ def get_tests_run(extra_args=None, tests_included=False, flatten_batches=False):
             return RecordingTestDriver(self, worker_number)
 
     recording_port = RecordingTestPort(options=options, user=user)
-    logging_run(extra_args=args, port_obj=recording_port, tests_included=True)
+    run_and_capture(recording_port, options, parsed_args)
 
     if flatten_batches:
         return list(itertools.chain(*test_batches))
 
     return test_batches
 
+
 class MainTest(unittest.TestCase):
     def test_accelerated_compositing(self):
         # This just tests that we recognize the command line args
@@ -244,12 +246,21 @@ class MainTest(unittest.TestCase):
         self.assertEqual(buildbot_output.get(), [])
 
     def test_lint_test_files(self):
-        # FIXME:  add errors?
-        res, out, err, user = logging_run(['--lint-test-files'],
-                                          tests_included=True)
+        res, out, err, user = logging_run(['--lint-test-files'])
         self.assertEqual(res, 0)
         self.assertTrue(out.empty())
-        self.assertTrue(any(['lint succeeded' in msg for msg in err.get()]))
+        self.assertTrue(any(['Lint succeeded' in msg for msg in err.get()]))
+
+    def test_lint_test_files__errors(self):
+        options, parsed_args = parse_args(['--lint-test-files'])
+        user = MockUser()
+        port_obj = port.get(options.platform, options=options, user=user)
+        port_obj.test_expectations = lambda: "# syntax error"
+        res, out, err = run_and_capture(port_obj, options, parsed_args)
+
+        self.assertEqual(res, -1)
+        self.assertTrue(out.empty())
+        self.assertTrue(any(['Lint failed' in msg for msg in err.get()]))
 
     def test_no_tests_found(self):
         res, out, err, user = logging_run(['resources'], tests_included=True)
index d2d67f35755dcd42c86109871b342ff63bd499d9..2f1057ac2fb502b7db0a3f3c14adfa837d64fa26 100644 (file)
@@ -87,28 +87,26 @@ class TestExpectationsChecker(object):
         pass
 
     def check_test_expectations(self, expectations_str, tests=None, overrides=None):
-        errors = []
+        err = None
         expectations = None
         try:
             expectations = test_expectations.TestExpectationsFile(
                 port=self._port_obj, expectations=expectations_str, full_test_list=tests,
                 test_platform_name=self._port_to_check, is_debug_mode=False,
-                is_lint_mode=True, suppress_errors=False, overrides=overrides)
-        except SyntaxError, error:
-            errors = str(error).splitlines()
-
-        for error in errors:
-            matched = self._output_regex.match(error)
-            if matched:
-                lineno, message = matched.group('line', 'message')
-                self._handle_style_error(int(lineno), 'test/expectations', 5, message)
-
-        if expectations:
-            for error in expectations.get_non_fatal_errors():
+                is_lint_mode=True, overrides=overrides)
+        except test_expectations.ParseError, error:
+            err = error
+
+        if err:
+            level = 2
+            if err.fatal:
+                level = 5
+            for error in err.errors:
                 matched = self._output_regex.match(error)
                 if matched:
                     lineno, message = matched.group('line', 'message')
-                    self._handle_style_error(int(lineno), 'test/expectations', 2, message)
+                    self._handle_style_error(int(lineno), 'test/expectations', level, message)
+
 
     def check_tabs(self, lines):
         self._tab_checker.check(lines)