Added update_test_expectations script to remove non-flaky test expectations.

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

Index: third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py
new file mode 100644
index 0000000000000000000000000000000000000000..4ce2a75d199a07863ea12f6bb43c85f295689756
--- /dev/null
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations_unittest.py
@@ -0,0 +1,404 @@
+# 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 webkitpy.layout_tests.update_test_expectations import RemoveFlakesOMatic
+
+from webkitpy.common.host_mock import MockHost
+
+from webkitpy.layout_tests.models.test_configuration import *
+from webkitpy.layout_tests.models.test_expectations import *

[qyearsley, 2016/03/10 19:13:59]

If you make a separate line for each of the names imported, then it makes it a
little clearer where things come from when reading the code:

from webkitpy.layout_tests.models.test_expectations import TestExpectations

Is anything else used from test_configuration and test_expectations below?

[bokan, 2016/03/16 20:24:53]

They were cargo-culted over here. Removed test_configuration, TestExpectations
was all that was needed in the other.

+from webkitpy.layout_tests.port.test import LAYOUT_TEST_DIR
+
+

[qyearsley, 2016/03/10 19:13:59]

This blank line could be removed.

[bokan, 2016/03/16 20:24:53]

Removed all the blanks in import statements.

+from collections import OrderedDict
+
+
+class FakeBotTestExpectations(object):
+    def __init__(self, results_by_path):

[qyearsley, 2016/03/10 19:13:58]

One blank line is added above the first method in the classes below; optionally
it could also be added here.

[bokan, 2016/03/16 20:24:53]

Done.

+        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 = {}
+
+    def set_results(self, results_by_path_by_builder):
+        """
+            Sets the results that will be returned by expectationts_for_builder.

[qyearsley, 2016/03/10 19:13:59]

Method summary can go on the first line: """Sets the results...

[bokan, 2016/03/16 20:24:53]

Done.

+
+            Args:
+                results_by_path_by_builder(Dict): The distinct results seen in

[qyearsley, 2016/03/10 19:13:59]

results_by_path_by_builder(Dict) -> results_by_path_by_builder (dict)

[bokan, 2016/03/16 20:24:53]

Done.

+                    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']
+                            }
+                        }
+

[qyearsley, 2016/03/10 19:13:59]

Blank lines at the end of docs could be removed.

[bokan, 2016/03/16 20:24:53]

Done.

+        """
+        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):
+        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 _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):
+        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_exp(self, expectations):

[qyearsley, 2016/03/10 19:13:59]

Could rename _exp and _parse_exp to _expectations and _parse_expectations, to
lessen ambiguity (exp can also stand for expression sometimes). This function
could also use a docstring.

[bokan, 2016/03/16 20:24:54]

Seems self._exp was unneeded. Renamed and docstring'd the rest.

+        expectations_dict = OrderedDict()
+        expectations_dict['expectations'] = expectations
+        self._port.expectations_dict = lambda: expectations_dict
+        self._exp = TestExpectations(self._port, self._get_test_list(), expectations_dict=None, is_lint_mode=False)
+
+    def test_dont_remove_non_flakes(self):
+        """ Test 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 ]"""
+
+        self._parse_exp(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):
+        """ Test 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 ]"""
+
+        self._parse_exp(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):
+        """ Test 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 ]""")
+
+        self._parse_exp(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):
+        """ Basic test 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 ]""")
+
+        self._parse_exp(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):
+        """ Test 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 ]""")
+
+        self._parse_exp(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):
+        """ Test that results with all failures are not treated as non-flaky.
+        """
+        test_expectations_before = ""
+
+        self._parse_exp(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):
+        """ Basic test with a 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 ]""")
+
+        self._parse_exp(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):
+        """ Test 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 ]""")
+
+        self._parse_exp(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):
+        """ Test correct operation of Debug/Release specifiers

[qyearsley, 2016/03/10 19:13:59]

- Space after """ is unnecessary
- In other function summaries, third-person singular form is used (i.e. "Tests"
rather than "Test").
- Docstring one-liner generally ends with a period.
- Since this is one line, the closing """ can go on the same line.

[bokan, 2016/03/16 20:24:53]

Done.

+
+        """
+        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 ]""")
+
+        self._parse_exp(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"],
+            },
+            'Win 7': {
+                "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):
+        """ Test 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 ]"""
+
+        self._parse_exp(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 ]"""))