Added update_test_expectations script to remove non-flaky test expectations.

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py

+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import unittest
+from collections import OrderedDict
+from webkitpy.common.host_mock import MockHost
+from webkitpy.layout_tests.models.test_expectations import TestExpectations
+from webkitpy.layout_tests.port.test import LAYOUT_TEST_DIR
+from webkitpy.layout_tests.update_test_expectations import RemoveFlakesOMatic
+from webkitpy.layout_tests.port import builders
+
+
+class FakeBotTestExpectations(object):
+
+    def __init__(self, results_by_path):
+        self._results = results_by_path
+
+    def all_results_by_path(self):
+        return self._results
+
+
+class FakeBotTestExpectationsFactory(object):
+
+    def __init__(self):
+        self._all_results_by_builder = {}

[Dirk Pranke, 2016/03/29 22:39:01]

Just make these public instance variables and get rid of the set() method.

[bokan, 2016/04/05 12:28:57]

Done.

+
+    def set_results(self, results_by_path_by_builder):
+        """Sets the results that will be returned by expectationts_for_builder.
+
+        Args:
+            results_by_path_by_builder (dict): The distinct results seen in
+                at least one run of the test. E.g. if the bot results for
+                mytest.html are:
+                    PASS PASS FAIL PASS TIMEOUT
+                then results_by_path_by_builder should be:
+                    {
+                        'WebKit Linux' : {
+                            'mytest.html': ['FAIL', 'PASS', 'TIMEOUT']
+                        }
+                    }
+        """
+        self._all_results_by_builder = results_by_path_by_builder
+
+    def expectations_for_builder(self, builder):
+        if builder not in self._all_results_by_builder:
+            return None
+
+        return FakeBotTestExpectations(self._all_results_by_builder[builder])
+
+
+class UpdateTestExpectationsTest(unittest.TestCase):
+
+    def __init__(self, testFunc):

[qyearsley, 2016/03/21 21:45:25]

What is testFunc here?

Also, would it make sense to use setUpClass and tearDownClass to do this?

[Dirk Pranke, 2016/03/29 22:39:01]

Don't override __init__ or use setUpClass or tearDownClass(), just use setUp()
and tearDown(). There's no reason to try and optimize this and using things that
aren't reset per method can lead to weird results.

[bokan, 2016/04/05 12:28:57]

It's part of the Python unittest framework. testFunc is a string, the name of
the method to run for the test. I cargo-culted this from some other test. The
test harness will pass the different test method names in here when running
tests.

[bokan, 2016/04/05 12:28:57]

Done.

+        self._host = MockHost()
+        self._port = self._host.port_factory.get('test', None)
+        self._expectation_factory = FakeBotTestExpectationsFactory()
+        self._flake_remover = RemoveFlakesOMatic(self._host,
+                                                 self._port,
+                                                 self._expectation_factory)
+        unittest.TestCase.__init__(self, testFunc)
+
+        filesystem = self._host.filesystem
+        for test in self._get_test_list():
+            filesystem.write_binary_file(filesystem.join(LAYOUT_TEST_DIR, test), '')
+
+    def setUp(self):
+        self._old_builders = builders._exact_matches

[qyearsley, 2016/03/21 21:45:26]

Maybe this attribute would be clearer if it were called
`self._old_builders_exact_matches`, or `self._original_builders_exact_matches`?

[bokan, 2016/04/05 12:28:57]

Done.

+        self._port.set_configuration_specifier_macros({
+            'mac': ['mac10.10'],
+            'win': ['win7'],
+            'linux': ['precise']
+        })
+
+    def tearDown(self):
+        builders._exact_matches = self._old_builders

[Dirk Pranke, 2016/03/29 22:39:01]

It is really ugly to be mutating module-level statics here, and it's lame that
builders.py is just a bunch of free functions in the first place.

Any idea how hard it would be to refactor that a bit to make builders.py contain
a class instead?

[bokan, 2016/04/05 12:28:57]

Yah, I agree. I'll look into doing this as a separate patch before landing this.

+
+    def _get_test_list(self):
+        return ['test/a.html',
+                'test/b.html',
+                'test/c.html',
+                'test/d.html',
+                'test/e.html',
+                'test/f.html',
+                'test/g.html']
+
+    def _assert_expectations_match(self, expectations, expected_string):
+        self.assertIsNotNone(expectations)
+        stringified_expectations = "\n".join(x.to_string(None) for x in expectations)
+        expected_string = "\n".join(x.strip() for x in expected_string.split("\n"))
+        self.assertEqual(stringified_expectations, expected_string)
+
+    def _parse_expectations(self, expectations):
+        """Parses a TestExpectatoin file given as string.

[Dirk Pranke, 2016/03/29 22:39:01]

nit: s/TestExpectatoin/TestExpectation/.

[bokan, 2016/04/05 12:28:57]

Done.

+
+        This function takes a string representing the contents of the
+        TestExpectations file and parses it, producing the TestExpectations
+        object and sets it on the Port object where the script will read it
+        from.
+
+        Args:
+            expectations: A string containing the contents of the
+            TestExpectations file to use.
+        """
+        expectations_dict = OrderedDict()
+        expectations_dict['expectations'] = expectations
+        self._port.expectations_dict = lambda: expectations_dict
+
+    def test_dont_remove_non_flakes(self):
+        """Tests that lines that aren't flaky are not touched.
+
+        Lines are flaky if they contain a PASS as well as at least one other
+        failing result.
+        """
+        test_expectations_before = """
+            # Even though the results show all passing, none of the
+            # expectations are flaky so we shouldn't remove any.
+            Bug(test) test/a.html [ Pass ]
+            Bug(test) test/b.html [ Timeout ]
+            Bug(test) test/c.html [ Failure Timeout ]
+            Bug(test) test/d.html [ Rebaseline ]
+            Bug(test) test/e.html [ NeedsManualRebaseline ]
+            Bug(test) test/f.html [ NeedsRebaseline ]"""
+
+        builders._exact_matches = {
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+        }
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('precise', 'x86_64'),))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({'WebKit Linux': {
+            "test/a.html": ["PASS", "PASS"],
+            "test/b.html": ["PASS", "PASS"],
+            "test/c.html": ["PASS", "PASS"],
+            "test/d.html": ["PASS", "PASS"],
+            "test/e.html": ["PASS", "PASS"],
+            "test/f.html": ["PASS", "PASS"],
+        }})
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, test_expectations_before)
+
+    def test_dont_remove_rebaselines(self):
+        """Tests that lines with rebaseline expectations are untouched.
+        """
+        test_expectations_before = """
+            # Even though the results show all passing, none of the
+            # expectations are flaky so we shouldn't remove any.
+            Bug(test) test/a.html [ Failure NeedsRebaseline Pass ]
+            Bug(test) test/b.html [ Failure Pass Rebaseline ]
+            Bug(test) test/c.html [ Failure NeedsManualRebaseline Pass ]"""
+
+        builders._exact_matches = {
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+        }
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('precise', 'x86_64'),))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({'WebKit Linux': {
+            "test/a.html": ["PASS", "PASS"],
+            "test/b.html": ["PASS", "PASS"],
+            "test/c.html": ["PASS", "PASS"]
+        }})
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, test_expectations_before)
+
+    def test_all_failure_types(self):
+        """Tests that all failure types are treated as failure."""
+        test_expectations_before = (
+            """Bug(test) test/a.html [ Failure Pass ]
+            Bug(test) test/b.html [ Failure Pass ]
+            Bug(test) test/c.html [ Failure Pass ]
+            Bug(test) test/d.html [ Failure Pass ]
+            # Remove these two since CRASH and TIMEOUT aren't considered Failure.
+            Bug(test) test/e.html [ Failure Pass ]
+            Bug(test) test/f.html [ Failure Pass ]""")
+
+        builders._exact_matches = {
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+        }
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('precise', 'x86_64'),))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({'WebKit Linux': {
+            "test/a.html": ["PASS", "IMAGE"],
+            "test/b.html": ["PASS", "TEXT"],
+            "test/c.html": ["PASS", "IMAGE+TEXT"],
+            "test/d.html": ["PASS", "AUDIO"],
+            "test/e.html": ["PASS", "CRASH"],
+            "test/f.html": ["PASS", "TIMEOUT"],
+        }})
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, (
+            """Bug(test) test/a.html [ Failure Pass ]
+            Bug(test) test/b.html [ Failure Pass ]
+            Bug(test) test/c.html [ Failure Pass ]
+            Bug(test) test/d.html [ Failure Pass ]"""))
+
+    def test_basic_one_builder(self):
+        """Tests basic functionality with a single builder.
+
+        Test that flaky expectations with results from a single bot showing the
+        expected failure isn't occuring should be removed. Results with failures
+        of the expected type shouldn't be removed but other kinds of failures
+        allow removal.
+        """
+        test_expectations_before = (
+            """# Remove this since it's passing all runs.
+            Bug(test) test/a.html [ Failure Pass ]
+            # Remove this since, although there's a failure, it's not a timeout.
+            Bug(test) test/b.html [ Pass Timeout ]
+            # Keep since we have both crashes and passes.
+            Bug(test) test/c.html [ Crash Pass ]""")

[qyearsley, 2016/03/21 21:45:25]

Formatting side note: If it's OK to have a newline at the the string, you could
remove the parentheses; it's also valid to write:

        test_expectations_before = """
            # Remove this since it's passing all runs.
            ...
            Bug(test) test/c.html [ Crash Pass ]"""

But the current way with parentheses is good too.

[bokan, 2016/04/05 12:28:57]

Yah, but that includes the newline in the string at the beginning (I had to deal
with lots of \n headaches here :). 

+
+        builders._exact_matches = {
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+        }
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('precise', 'x86_64'),))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({'WebKit Linux': {
+            "test/a.html": ["PASS", "PASS", "PASS"],
+            "test/b.html": ["PASS", "IMAGE", "PASS"],
+            "test/c.html": ["PASS", "CRASH", "PASS"],
+        }})
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, (
+            """# Keep since we have both crashes and passes.
+            Bug(test) test/c.html [ Crash Pass ]"""))
+
+    def test_all_failure_case(self):
+        """Tests that results with all failures are not treated as non-flaky."""
+        test_expectations_before = (
+            """# Keep since it's all failures.
+            Bug(test) test/a.html [ Failure Pass ]""")
+
+        builders._exact_matches = {
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+        }
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('precise', 'x86_64'),))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({'WebKit Linux': {
+            "test/a.html": ["IMAGE", "IMAGE", "IMAGE"],
+        }})
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, (
+            """# Keep since it's all failures.
+            Bug(test) test/a.html [ Failure Pass ]"""))
+
+    def test_empty_test_expectations(self):
+        """Tests that results with all failures are not treated as non-flaky."""
+        test_expectations_before = ""
+
+        builders._exact_matches = {
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+        }
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('precise', 'x86_64'),))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({'WebKit Linux': {
+            "test/a.html": ["PASS", "PASS", "PASS"],
+        }})
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, "")
+
+    def test_basic_multiple_builders(self):
+        """Tests basic functionality with multiple builders."""
+        test_expectations_before = (
+            """# Remove since it's passing on both builders.
+            Bug(test) test/a.html [ Failure Pass ]
+            # Keep since it's failing on the Mac builder.
+            Bug(test) test/b.html [ Failure Pass ]
+            # Keep since it's failing on the Linux builder.
+            Bug(test) test/c.html [ Failure Pass ]""")
+
+        builders._exact_matches = {
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+            "WebKit Mac10.10": {"port_name": "mac-mac10.10", "specifiers": ['Mac10.10', 'Release']},
+        }
+
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('mac10.10', 'x86'),
+                                   ('precise', 'x86_64')))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({
+            'WebKit Linux': {
+                "test/a.html": ["PASS", "PASS", "PASS"],
+                "test/b.html": ["PASS", "PASS", "PASS"],
+                "test/c.html": ["AUDIO", "AUDIO", "AUDIO"],
+            },
+            'WebKit Mac10.10': {
+                "test/a.html": ["PASS", "PASS", "PASS"],
+                "test/b.html": ["PASS", "PASS", "IMAGE"],
+                "test/c.html": ["PASS", "PASS", "PASS"],
+            },
+        })
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, (
+            """# Keep since it's failing on the Mac builder.
+            Bug(test) test/b.html [ Failure Pass ]
+            # Keep since it's failing on the Linux builder.
+            Bug(test) test/c.html [ Failure Pass ]"""))
+
+    def test_multiple_builders_and_platform_specifiers(self):
+        """Tests correct operation with platform specifiers."""
+        test_expectations_before = (
+            """# Keep since it's failing on Mac results.
+            Bug(test) [ Mac ] test/a.html [ Failure Pass ]
+            # Keep since it's failing on the Windows builder.
+            Bug(test) [ Linux Win ] test/b.html [ Failure Pass ]
+            # Remove since it's passing on both Linux and Windows builders.
+            Bug(test) [ Linux Win ] test/c.html [ Failure Pass ]
+            # Remove since it's passing on Mac results
+            Bug(test) [ Mac ] test/d.html [ Failure Pass ]""")
+
+        builders._exact_matches = {
+            "WebKit Win7": {"port_name": "win-win7", "specifiers": ['Win7', 'Release']},
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+            "WebKit Mac10.10": {"port_name": "mac-mac10.10", "specifiers": ['Mac10.10', 'Release']},
+        }
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('mac10.10', 'x86'),
+                                   ('win7', 'x86'),
+                                   ('precise', 'x86_64')))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({
+            'WebKit Linux': {
+                "test/a.html": ["PASS", "PASS", "PASS"],
+                "test/b.html": ["PASS", "PASS", "PASS"],
+                "test/c.html": ["PASS", "PASS", "PASS"],
+                "test/d.html": ["IMAGE", "PASS", "PASS"],
+            },
+            'WebKit Mac10.10': {
+                "test/a.html": ["PASS", "PASS", "IMAGE"],
+                "test/b.html": ["PASS", "IMAGE", "PASS"],
+                "test/c.html": ["PASS", "IMAGE", "PASS"],
+                "test/d.html": ["PASS", "PASS", "PASS"],
+            },
+            'WebKit Win7': {
+                "test/a.html": ["PASS", "PASS", "PASS"],
+                "test/b.html": ["IMAGE", "PASS", "PASS"],
+                "test/c.html": ["PASS", "PASS", "PASS"],
+                "test/d.html": ["IMAGE", "PASS", "PASS"],
+            },
+        })
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, (
+            """# Keep since it's failing on Mac results.
+            Bug(test) [ Mac ] test/a.html [ Failure Pass ]
+            # Keep since it's failing on the Windows builder.
+            Bug(test) [ Linux Win ] test/b.html [ Failure Pass ]"""))
+
+    def test_debug_release_specifiers(self):
+        """Tests correct operation of Debug/Release specifiers."""
+        test_expectations_before = (
+            """# Keep since it fails in debug.
+            Bug(test) [ Linux ] test/a.html [ Failure Pass ]
+            # Remove since the failure is in Release, Debug is all PASS.
+            Bug(test) [ Debug ] test/b.html [ Failure Pass ]
+            # Keep since there's a failure in Linux Release.
+            Bug(test) [ Release ] test/c.html [ Failure Pass ]
+            # Remove since the Release Linux builder is all passing.
+            Bug(test) [ Release Linux ] test/d.html [ Failure Pass ]
+            # Remove since all the Linux builders PASS.
+            Bug(test) [ Linux ] test/e.html [ Failure Pass ]""")
+
+        builders._exact_matches = {
+            "WebKit Win7": {"port_name": "win-win7", "specifiers": ['Win7', 'Release']},
+            "WebKit Win7 (dbg)": {"port_name": "win-win7", "specifiers": ['Win7', 'Debug']},
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+            "WebKit Linux (dbg)": {"port_name": "linux-precise", "specifiers": ['Precise', 'Debug']},
+        }
+        self._port.set_all_build_types(('release', 'debug'))
+        self._port.set_all_systems((('win7', 'x86'),
+                                    ('precise', 'x86_64')))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({
+            'WebKit Linux': {
+                "test/a.html": ["PASS", "PASS", "PASS"],
+                "test/b.html": ["PASS", "IMAGE", "PASS"],
+                "test/c.html": ["PASS", "IMAGE", "PASS"],
+                "test/d.html": ["PASS", "PASS", "PASS"],
+                "test/e.html": ["PASS", "PASS", "PASS"],
+            },
+            'WebKit Linux (dbg)': {
+                "test/a.html": ["PASS", "IMAGE", "PASS"],
+                "test/b.html": ["PASS", "PASS", "PASS"],
+                "test/c.html": ["PASS", "PASS", "PASS"],
+                "test/d.html": ["IMAGE", "PASS", "PASS"],
+                "test/e.html": ["PASS", "PASS", "PASS"],
+            },
+            'WebKit Win7 (dbg)': {
+                "test/a.html": ["PASS", "PASS", "PASS"],
+                "test/b.html": ["PASS", "PASS", "PASS"],
+                "test/c.html": ["PASS", "PASS", "PASS"],
+                "test/d.html": ["PASS", "IMAGE", "PASS"],
+                "test/e.html": ["PASS", "PASS", "PASS"],
+            },
+            'WebKit Win7': {
+                "test/a.html": ["PASS", "PASS", "PASS"],
+                "test/b.html": ["PASS", "PASS", "IMAGE"],
+                "test/c.html": ["PASS", "PASS", "PASS"],
+                "test/d.html": ["PASS", "IMAGE", "PASS"],
+                "test/e.html": ["PASS", "PASS", "PASS"],
+            },
+        })
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, (
+            """# Keep since it fails in debug.
+            Bug(test) [ Linux ] test/a.html [ Failure Pass ]
+            # Keep since there's a failure in Linux Release.
+            Bug(test) [ Release ] test/c.html [ Failure Pass ]"""))
+
+    def test_preserve_comments_and_whitespace(self):
+        """Tests that comments and whitespace are preserved appropriately.
+
+        Comments and whitespace should be kept unless all the tests grouped
+        below a comment are removed. In that case the comment block should also
+        be removed.
+
+        Ex:
+            # This comment applies to the below tests.
+            Bug(test) test/a.html [ Failure Pass ]
+            Bug(test) test/b.html [ Failure Pass ]
+
+            # <some prose>
+
+            # This is another comment.
+            Bug(test) test/c.html [ Failure Pass ]
+
+        Assuming we removed a.html and c.html we get:
+            # This comment applies to the below tests.
+            Bug(test) test/b.html [ Failure Pass ]
+
+            # <some prose>
+        """
+        test_expectations_before = """
+            # Comment A - Keep since these aren't part of any test.
+            # Comment B - Keep since these aren't part of any test.
+
+            # Comment C - Remove since it's a block belonging to a
+            # Comment D - and a is removed.
+            Bug(test) test/a.html [ Failure Pass ]
+            # Comment E - Keep since it's below a.
+
+
+            # Comment F - Keep since only b is removed
+            Bug(test) test/b.html [ Failure Pass ]
+            Bug(test) test/c.html [ Failure Pass ]
+
+            # Comment G - Should be removed since both d and e will be removed.
+            Bug(test) test/d.html [ Failure Pass ]
+            Bug(test) test/e.html [ Failure Pass ]"""
+
+        builders._exact_matches = {
+            "WebKit Linux": {"port_name": "linux-precise", "specifiers": ['Precise', 'Release']},
+        }
+        self._port.set_all_build_types(('release',))
+        self._port.set_all_systems((('precise', 'x86_64'),))
+
+        self._parse_expectations(test_expectations_before)
+        self._expectation_factory.set_results({
+            'WebKit Linux': {
+                "test/a.html": ["PASS", "PASS", "PASS"],
+                "test/b.html": ["PASS", "PASS", "PASS"],
+                "test/c.html": ["PASS", "IMAGE", "PASS"],
+                "test/d.html": ["PASS", "PASS", "PASS"],
+                "test/e.html": ["PASS", "PASS", "PASS"],
+            }
+        })
+        updated_expectations = self._flake_remover.get_updated_test_expectations()
+        self._assert_expectations_match(updated_expectations, (
+            """
+            # Comment A - Keep since these aren't part of any test.
+            # Comment B - Keep since these aren't part of any test.
+            # Comment E - Keep since it's below a.
+
+
+            # Comment F - Keep since only b is removed
+            Bug(test) test/c.html [ Failure Pass ]"""))