Attachment 348435 Details for Bug 189088 – Patch

[patch] Patch

bug-189088-20180829151156.patch (text/plain), 21.39 KB, created by Simon Fraser (smfr) on 2018-08-29 15:11:57 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2018-08-29 15:11:57 PDT

Size: 21.39 KB

patch

obsolete

>Subversion Revision: 235475
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 472bc3e9d6d4f8c0c0b2092c427c7931e85a3088..28a474b43c303006e3264d3c4d9bcb30f9043f93 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,28 @@
>+2018-08-29  Simon Fraser  <simon.fraser@apple.com>
>+
>+        Make it possible to mark tests as leaks in TestExpectations
>+        https://bugs.webkit.org/show_bug.cgi?id=189088
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        Have webkitpy parse out "Leak" expectations in TestExpectations, and do the right
>+        thing if the test run did not use --world-leaks. Add unit tests for leaks combined
>+        with various other result combinations.
>+
>+        * Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:
>+        (LayoutTestRunner._update_summary_with_result):
>+        (LayoutTestRunner._annotate_results_with_additional_failures):
>+        * Scripts/webkitpy/layout_tests/models/test_expectations.py:
>+        (TestExpectationParser):
>+        (TestExpectations): The 'Leak' line was duplicated here, so remove a copy.
>+        (TestExpectations.remove_leak_failures):
>+        (TestExpectations.matches_an_expected_result):
>+        * Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:
>+        (Base.get_basic_tests):
>+        * Scripts/webkitpy/port/test.py:
>+        (TestList.add_reftest):
>+        (unit_test_list):
>+
> 2018-08-29  Jer Noble  <jer.noble@apple.com>
> 
>         Muted elements do not have their Now Playing status updated when unmuted.
>diff --git a/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py b/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
>index 076547d4ed78550a80068afcd838b5ac0f8b1b64..4b19974d9874dd826d3ab5d6db2b80eed99d1412 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py
>@@ -182,7 +182,7 @@ class LayoutTestRunner(object):
>             exp_str = got_str = 'SKIP'
>             expected = True
>         else:
>-            expected = self._expectations.matches_an_expected_result(result.test_name, result.type, self._options.pixel_tests or result.reftest_type)
>+            expected = self._expectations.matches_an_expected_result(result.test_name, result.type, self._options.pixel_tests or result.reftest_type, self._options.world_leaks)
>             exp_str = self._expectations.model().get_expectations_string(result.test_name)
>             got_str = self._expectations.model().expectation_to_string(result.type)
> 
>@@ -198,8 +198,8 @@ class LayoutTestRunner(object):
>             # When running a chunk (--run-chunk), results_by_name contains all the tests, but (confusingly) all_tests only contains those in the chunk that was run,
>             # and we don't want to modify the results of a test that didn't run. existing_result.test_number is only non-None for tests that ran.
>             if existing_result and existing_result.test_number is not None:
>-                was_expected = self._expectations.matches_an_expected_result(new_result.test_name, existing_result.type, self._options.pixel_tests or existing_result.reftest_type)
>-                now_expected = self._expectations.matches_an_expected_result(new_result.test_name, new_result.type, self._options.pixel_tests or new_result.reftest_type)
>+                was_expected = self._expectations.matches_an_expected_result(new_result.test_name, existing_result.type, self._options.pixel_tests or existing_result.reftest_type, self._options.world_leaks)
>+                now_expected = self._expectations.matches_an_expected_result(new_result.test_name, new_result.type, self._options.pixel_tests or new_result.reftest_type, self._options.world_leaks)
>                 run_results.change_result_to_failure(existing_result, new_result, was_expected, now_expected)
> 
>     def start_servers(self):
>diff --git a/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py b/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py
>index af4d1be7f50c811a999133f1cfb870af205d7901..f8cd7d009fc42181636051e1d5670447dee4196c 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py
>@@ -245,7 +245,6 @@ class TestExpectationParser(object):
>         'Failure': 'FAIL',
>         'ImageOnlyFailure': 'IMAGE',
>         'Missing': 'MISSING',
>-        'Leak': 'LEAK',
>         'Pass': 'PASS',
>         'Rebaseline': 'REBASELINE',
>         'Skip': 'SKIP',
>@@ -404,6 +403,9 @@ class TestExpectationLine(object):
>         self.related_files = {}  # Dictionary of files to lines number in that file which may have caused the list of warnings.
>         self.not_applicable_to_current_platform = False
> 
>+    def __str__(self):
>+        return self.to_string(None)
>+
>     def is_invalid(self):
>         return self.warnings and self.warnings != [TestExpectationParser.MISSING_BUG_WARNING]
> 
>@@ -866,6 +868,19 @@ class TestExpectations(object):
>             expected_results.add(PASS)
>         return expected_results
> 
>+    @staticmethod
>+    def remove_leak_failures(expected_results):
>+        """Returns a copy of the expected results for a test, except that we
>+        drop any leak failures and return the remaining expectations. For example,
>+        if we're not running with --world-leaks, then tests expected to fail as LEAK
>+        will PASS."""
>+        expected_results = expected_results.copy()
>+        if LEAK in expected_results:
>+            expected_results.remove(LEAK)
>+            if not expected_results:
>+                expected_results.add(PASS)
>+        return expected_results
>+
>     @staticmethod
>     def has_pixel_failures(actual_results):
>         return IMAGE in actual_results or FAIL in actual_results
>@@ -950,10 +965,14 @@ class TestExpectations(object):
>     def get_rebaselining_failures(self):
>         return self._model.get_test_set(REBASELINE)
> 
>-    def matches_an_expected_result(self, test, result, pixel_tests_are_enabled):
>+    def matches_an_expected_result(self, test, result, pixel_tests_are_enabled, world_leaks_are_enabled):
>         expected_results = self._model.get_expectations(test)
>         if not pixel_tests_are_enabled:
>             expected_results = self.remove_pixel_failures(expected_results)
>+
>+        if not world_leaks_are_enabled:
>+            expected_results = self.remove_leak_failures(expected_results)
>+
>         return self.result_was_expected(result,
>                                    expected_results,
>                                    self.is_rebaselining(test),
>diff --git a/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py b/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py
>index 6bda9732cd897287bad0dfd3aa1ea1a8a92e2780..4d3f0b6676ba30336d2f33dd2c310d8987e4dcee 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py
>@@ -54,18 +54,24 @@ class Base(unittest.TestCase):
>                 'failures/expected/image_checksum.html',
>                 'failures/expected/crash.html',
>                 'failures/expected/leak.html',
>+                'failures/expected/flaky-leak.html',
>                 'failures/expected/missing_text.html',
>                 'failures/expected/image.html',
>+                'failures/expected/reftest.html',
>+                'failures/expected/leaky-reftest.html',
>                 'passes/text.html']
> 
>     def get_basic_expectations(self):
>         return """
> Bug(test) failures/expected/text.html [ Failure ]
> Bug(test) failures/expected/crash.html [ WontFix ]
>-Bug(test) failures/expected/crash.html [ Leak ]
>+Bug(test) failures/expected/leak.html [ Leak ]
>+Bug(test) failures/expected/flaky-leak.html [ Failure Leak ]
> Bug(test) failures/expected/missing_image.html [ Rebaseline Missing ]
> Bug(test) failures/expected/image_checksum.html [ WontFix ]
> Bug(test) failures/expected/image.html [ WontFix Mac ]
>+Bug(test) failures/expected/reftest.html [ ImageOnlyFailure ]
>+Bug(test) failures/expected/leaky-reftest.html [ ImageOnlyFailure Leak ]
> """
> 
>     def parse_exp(self, expectations, overrides=None, is_lint_mode=False):
>@@ -79,8 +85,10 @@ Bug(test) failures/expected/image.html [ WontFix Mac ]
>         self._exp.parse_all_expectations()
> 
>     def assert_exp(self, test, result):
>-        self.assertEqual(self._exp.model().get_expectations(test),
>-                          set([result]))
>+        self.assertEqual(self._exp.model().get_expectations(test), set([result]))
>+
>+    def assert_exp_set(self, test, result_set):
>+        self.assertEqual(self._exp.model().get_expectations(test), result_set)
> 
>     def assert_bad_expectations(self, expectations, overrides=None):
>         self.assertRaises(ParseError, self.parse_exp, expectations, is_lint_mode=True, overrides=overrides)
>@@ -91,8 +99,12 @@ class BasicTests(Base):
>         self.parse_exp(self.get_basic_expectations())
>         self.assert_exp('failures/expected/text.html', FAIL)
>         self.assert_exp('failures/expected/image_checksum.html', PASS)
>-        self.assert_exp('failures/expected/leak.html', PASS)
>+        self.assert_exp('failures/expected/reftest.html', IMAGE)
>+        self.assert_exp_set('failures/expected/leaky-reftest.html', set([IMAGE, LEAK]))
>+        self.assert_exp('failures/expected/leak.html', LEAK)
>+        self.assert_exp_set('failures/expected/flaky-leak.html', set([FAIL, LEAK]))
>         self.assert_exp('passes/text.html', PASS)
>+        # self.assert_exp_set('passes/flaky-leak.html', set([PASS, LEAK]))
>         self.assert_exp('failures/expected/image.html', PASS)
> 
> 
>@@ -123,6 +135,14 @@ class MiscTests(Base):
>         self.assertEqual(TestExpectations.remove_pixel_failures(set([FAIL])), set([FAIL]))
>         self.assertEqual(TestExpectations.remove_pixel_failures(set([PASS, IMAGE, CRASH])), set([PASS, CRASH]))
> 
>+    def test_remove_leak_failures(self):
>+        self.assertEqual(TestExpectations.remove_leak_failures(set([FAIL])), set([FAIL]))
>+        self.assertEqual(TestExpectations.remove_leak_failures(set([PASS])), set([PASS]))
>+        self.assertEqual(TestExpectations.remove_leak_failures(set([LEAK])), set([PASS]))
>+        self.assertEqual(TestExpectations.remove_leak_failures(set([PASS, LEAK])), set([PASS]))
>+        self.assertEqual(TestExpectations.remove_leak_failures(set([FAIL, LEAK])), set([FAIL]))
>+        self.assertEqual(TestExpectations.remove_leak_failures(set([PASS, IMAGE, LEAK, CRASH])), set([PASS, IMAGE, CRASH]))
>+
>     def test_suffixes_for_expectations(self):
>         self.assertEqual(TestExpectations.suffixes_for_expectations(set([FAIL])), set(['txt', 'png', 'wav']))
>         self.assertEqual(TestExpectations.suffixes_for_expectations(set([IMAGE])), set(['png']))
>@@ -152,8 +172,7 @@ class MiscTests(Base):
>     def test_expectation_to_string(self):
>         # Normal cases are handled by other tests.
>         self.parse_exp(self.get_basic_expectations())
>-        self.assertRaises(ValueError, self._exp.model().expectation_to_string,
>-                          -1)
>+        self.assertRaises(ValueError, self._exp.model().expectation_to_string, -1)
> 
>     def test_get_test_set(self):
>         # Handle some corner cases for this routine not covered by other tests.
>@@ -215,20 +234,44 @@ class MiscTests(Base):
> 
>     def test_pixel_tests_flag(self):
>         def match(test, result, pixel_tests_enabled):
>-            return self._exp.matches_an_expected_result(
>-                test, result, pixel_tests_enabled)
>+            return self._exp.matches_an_expected_result(test, result, pixel_tests_enabled, False)
>+
>+        self.parse_exp(self.get_basic_expectations())
>+        pixel_tests_enabled = True
>+        pixel_tests_disabled = False
>+        self.assertTrue(match('failures/expected/text.html', FAIL, pixel_tests_enabled))
>+        self.assertTrue(match('failures/expected/text.html', FAIL, pixel_tests_disabled))
>+        self.assertFalse(match('failures/expected/text.html', CRASH, pixel_tests_enabled))
>+        self.assertFalse(match('failures/expected/text.html', CRASH, pixel_tests_disabled))
>+        self.assertTrue(match('failures/expected/image_checksum.html', PASS, pixel_tests_enabled))
>+        self.assertTrue(match('failures/expected/image_checksum.html', PASS, pixel_tests_disabled))
>+        self.assertTrue(match('failures/expected/crash.html', PASS, pixel_tests_disabled))
>+        self.assertTrue(match('passes/text.html', PASS, pixel_tests_disabled))
>+
>+    def test_world_leaks_flag(self):
>+        def match(test, result, pixel_tests_enabled, world_leaks_enabled):
>+            return self._exp.matches_an_expected_result(test, result, pixel_tests_enabled, world_leaks_enabled)
>+
>+        pixel_tests_enabled = True
>+        pixel_tests_disabled = False
>+        world_leaks_enabled = True
>+        world_leaks_disabled = False
> 
>         self.parse_exp(self.get_basic_expectations())
>-        self.assertTrue(match('failures/expected/text.html', FAIL, True))
>-        self.assertTrue(match('failures/expected/text.html', FAIL, False))
>-        self.assertFalse(match('failures/expected/text.html', CRASH, True))
>-        self.assertFalse(match('failures/expected/text.html', CRASH, False))
>-        self.assertTrue(match('failures/expected/image_checksum.html', PASS,
>-                              True))
>-        self.assertTrue(match('failures/expected/image_checksum.html', PASS,
>-                              False))
>-        self.assertTrue(match('failures/expected/crash.html', PASS, False))
>-        self.assertTrue(match('passes/text.html', PASS, False))
>+        self.assertTrue(match('failures/expected/leak.html', LEAK, pixel_tests_enabled, world_leaks_enabled))
>+        self.assertTrue(match('failures/expected/leak.html', PASS, pixel_tests_enabled, world_leaks_disabled))
>+        self.assertTrue(match('failures/expected/flaky-leak.html', FAIL, pixel_tests_enabled, world_leaks_disabled))
>+
>+        self.assertTrue(match('failures/expected/leaky-reftest.html', LEAK, pixel_tests_disabled, world_leaks_enabled))
>+        self.assertTrue(match('failures/expected/leaky-reftest.html', PASS, pixel_tests_disabled, world_leaks_disabled))
>+
>+        self.assertTrue(match('failures/expected/leaky-reftest.html', IMAGE, pixel_tests_enabled, world_leaks_enabled))
>+        self.assertTrue(match('failures/expected/leaky-reftest.html', LEAK, pixel_tests_enabled, world_leaks_enabled))
>+        self.assertTrue(match('failures/expected/leaky-reftest.html', IMAGE, pixel_tests_enabled, world_leaks_disabled))
>+
>+        self.assertFalse(match('failures/expected/text.html', PASS, pixel_tests_enabled, world_leaks_enabled))
>+        self.assertFalse(match('failures/expected/text.html', CRASH, pixel_tests_enabled, world_leaks_disabled))
>+        self.assertTrue(match('passes/text.html', PASS, pixel_tests_enabled, world_leaks_disabled))
> 
>     def test_more_specific_override_resets_skip(self):
>         self.parse_exp("Bug(x) failures/expected [ Skip ]\n"
>@@ -348,6 +391,9 @@ class ExpectationSyntaxTests(Base):
>     def test_slow(self):
>         self.assert_tokenize_exp('foo.html [ Slow ]', modifiers=['SLOW'], expectations=['PASS'])
> 
>+    def test_leak(self):
>+        self.assert_tokenize_exp('foo.html [ Leak ]', modifiers=[], expectations=['LEAK'])
>+
>     def test_wontfix(self):
>         self.assert_tokenize_exp('foo.html [ WontFix ]', modifiers=['WONTFIX', 'SKIP'], expectations=['PASS'])
>         self.assert_tokenize_exp('foo.html [ WontFix ImageOnlyFailure ]', modifiers=['WONTFIX'], expectations=['IMAGE'])
>diff --git a/Tools/Scripts/webkitpy/layout_tests/models/test_results.py b/Tools/Scripts/webkitpy/layout_tests/models/test_results.py
>index 922a96e33da842a69c0c3289de62002121be0b4e..947f707d656e79bbdd8df356c6d23b2e346bd0cd 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/models/test_results.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/models/test_results.py
>@@ -68,6 +68,14 @@ class TestResult(object):
>     def __hash__(self):
>         return self.test_name.__hash__()
> 
>+    def __str__(self):
>+        result = self.test_name
>+        if len(self.failures):
>+            result += ' failures: ' + ','.join([failure.message() for failure in self.failures])
>+        if len(self.reftest_type):
>+            result += ' reftest_type: ' + ','.join(self.reftest_type)
>+        return result
>+
>     def convert_to_failure(self, failure_result):
>         if self.type is failure_result.type:
>             return
>diff --git a/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py b/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
>index 197520d031f8976b3fe420c51495e7b074898e19..4370fddc6709c9cf14be97ba18792e6681b1d00e 100644
>--- a/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
>+++ b/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
>@@ -118,7 +118,7 @@ class TestRunResults(object):
>             self.unexpected -= 1
>             if had_failures:
>                 self.unexpected_failures -= 1
>-        else:
>+        elif existing_expected and not new_expected:
>             # test changed from expected to unexpected
>             self.expected -= 1
>             self.unexpected_results_by_name[existing_result.test_name] = existing_result
>diff --git a/Tools/Scripts/webkitpy/port/test.py b/Tools/Scripts/webkitpy/port/test.py
>index f2d6c6ffcc34319c44b04e686136297da26c0cf6..96d72ddf47ccb479f8dd5bde7610621abd954fbc 100644
>--- a/Tools/Scripts/webkitpy/port/test.py
>+++ b/Tools/Scripts/webkitpy/port/test.py
>@@ -78,13 +78,18 @@ class TestList(object):
>             test.__dict__[key] = value
>         self.tests[name] = test
> 
>-    def add_reftest(self, name, reference_name, same_image):
>+    def add_reftest(self, name, reference_name, same_image, **kwargs):
>         self.add(name, actual_checksum='xxx', actual_image='XXX', is_reftest=True)
>         if same_image:
>             self.add(reference_name, actual_checksum='xxx', actual_image='XXX', is_reftest=True)
>         else:
>             self.add(reference_name, actual_checksum='yyy', actual_image='YYY', is_reftest=True)
> 
>+        if kwargs:
>+            test = self.tests[name]
>+            for key, value in kwargs.items():
>+                test.__dict__[key] = value
>+
>     def keys(self):
>         return self.tests.keys()
> 
>@@ -97,7 +102,7 @@ class TestList(object):
> #
> # These numbers may need to be updated whenever we add or delete tests.
> #
>-TOTAL_TESTS = 74
>+TOTAL_TESTS = 76
> TOTAL_SKIPS = 9
> TOTAL_RETRIES = 15
> 
>@@ -116,6 +121,7 @@ def unit_test_list():
>     tests.add('failures/expected/hang.html', hang=True)
>     tests.add('failures/expected/missing_text.html', expected_text=None)
>     tests.add('failures/expected/leak.html', leak=True)
>+    tests.add('failures/expected/flaky-leak.html', leak=True)
>     tests.add('failures/expected/image.html',
>               actual_image='image_fail-pngtEXtchecksum\x00checksum_fail',
>               expected_image='image-pngtEXtchecksum\x00checksum-png')
>@@ -215,6 +221,7 @@ layer at (0,0) size 800x34
>     tests.add_reftest('passes/xhtreftest.xht', 'passes/xhtreftest-expected.html', same_image=True)
>     tests.add_reftest('passes/phpreftest.php', 'passes/phpreftest-expected-mismatch.svg', same_image=False)
>     tests.add_reftest('failures/expected/reftest.html', 'failures/expected/reftest-expected.html', same_image=False)
>+    tests.add_reftest('failures/expected/leaky-reftest.html', 'failures/expected/leaky-reftest-expected.html', same_image=False, leak=True)
>     tests.add_reftest('failures/expected/mismatch.html', 'failures/expected/mismatch-expected-mismatch.html', same_image=True)
>     tests.add_reftest('failures/unexpected/reftest.html', 'failures/unexpected/reftest-expected.html', same_image=False)
>     tests.add_reftest('failures/unexpected/mismatch.html', 'failures/unexpected/mismatch-expected-mismatch.html', same_image=True)
>@@ -283,6 +290,8 @@ def add_unit_tests_to_mock_filesystem(filesystem):
>         filesystem.write_text_file(LAYOUT_TEST_DIR + '/platform/test/TestExpectations', """
> Bug(test) failures/expected/crash.html [ Crash ]
> Bug(test) failures/expected/leak.html [ Leak ]
>+Bug(test) failures/expected/flaky-leak.html [ Failure Leak ]
>+Bug(test) failures/expected/leaky-reftest.html [ ImageOnlyFailure Leak ]
> Bug(test) failures/expected/image.html [ ImageOnlyFailure ]
> Bug(test) failures/expected/audio.html [ Failure ]
> Bug(test) failures/expected/image_checksum.html [ ImageOnlyFailure ]
>@@ -599,11 +608,15 @@ class TestDriver(Driver):
>             return None
> 
>         test_world_leaks_output = """TEST: file:///test.checkout/LayoutTests/failures/expected/leak.html
>-ABANDONED DOCUMENT: file:///test.checkout/LayoutTests//failures/expected/leak.html
>+ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/leak.html
> TEST: file:///test.checkout/LayoutTests/failures/unexpected/leak.html
>-ABANDONED DOCUMENT: file:///test.checkout/LayoutTests//failures/expected/leak.html
>+ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/flaky-leak.html
>+TEST: file:///test.checkout/LayoutTests/failures/unexpected/flaky-leak.html
>+ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/leak.html
> TEST: file:///test.checkout/LayoutTests/failures/unexpected/leak.html
>-ABANDONED DOCUMENT: file:///test.checkout/LayoutTests//failures/expected/leak-subframe.html"""
>+ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/leak-subframe.html
>+TEST: file:///test.checkout/LayoutTests/failures/expected/leaky-reftest.html
>+ABANDONED DOCUMENT: file:///test.checkout/LayoutTests/failures/expected/leaky-reftest.html"""
>         return self._parse_world_leaks_output(test_world_leaks_output)
> 
>     def stop(self):

Flags:

jonlee: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 189088: 348411 | 348424 | 348435