Fill out implementation of UpdateTestExpectations script

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

Index: third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
index ed1be7297a62b05e1315ba677989d72c965d3e50..0fb450abee8a9f72fb010cd7a09bd1880e86e76c 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.py
@@ -62,6 +62,111 @@ class RemoveFlakesOMatic(object):
         self._expectations_factory = bot_test_expectations_factory
         self.builder_results_by_path = {}
 
+    def _can_delete_line(self, test_expectation_line):
+        """Returns whether a given line in the expectations can be removed.
+
+        Uses results from builder bots to determine if a given line is stale and
+        can safely be removed from the TestExpectations file. (i.e. remove if
+        the bots show that it's not flaky.) There's also some rules about when

[qyearsley, 2016/07/08 17:55:03]

There's also some rules -> There are also some rules

[bokan, 2016/07/08 21:13:00]

Done.

+        not to remove lines (e.g. never remove lines with Rebaseline
+        expectations, don't remove non-flaky expectations, etc.)
+
+        Args:
+            test_expectation_line (TestExpectationLine): A line in the test
+                expectation file to test for possible removal.
+
+        Returns: True if the line can be removed, False otherwise.

[qyearsley, 2016/07/08 17:55:04]

To be consistent with the docstrings below, this could be:
    Returns:
        True if the line can be removed, False otherwise.

[bokan, 2016/07/08 21:13:00]

Done.

+        """
+        expectations = test_expectation_line.expectations
+        if len(expectations) < 2:
+            return False
+
+        if self._has_unstrippable_expectations(expectations):
+            return False
+
+        if not self._has_pass_expectation(expectations):
+            return False

[qyearsley, 2016/07/08 17:55:03]

Is "PASS" an "unstrippable" expectation? Would it be possible to put "PASS" in
the list of unstrippable expectations and remove _has_pass_expectations?

[bokan, 2016/07/08 21:13:00]

No, it's a bit subtle but has_unstrippable_expectations is saying that that line
shouldn't even be looked at (e.g. it has a NeedsRebaseline).

The second condition here is making sure that the line is actually flaky. That
is, we don't want to look at lines that are known to be failing (that seems like
an obvious extension for the future but this script is scoped just for flakiness
for now).

I've added comments here to clarify.

If we made pass unstrippable this script wouldn't do anything :)

+
+        # The line can be deleted if the only expectation on the line that appears in the actual
+        # results is the PASS expectation.
+        for config in test_expectation_line.matching_configurations:
+            builder_name = self._host.builders.builder_name_for_specifiers(config.version, config.build_type)
+
+            if not builder_name:
+                _log.error('Failed to get builder for config [%s, %s, %s]' % (config.version, config.architecture, config.build_type))

[qyearsley, 2016/07/08 17:55:03]

For logging methods, if you give more than one argument it will use string
formatting to format the string for you, e.g.:

_log.error('Failed to get builder for config [%s, %s, %s]',
           config.version, config.architecture, config.build_type)

This is slightly nicer because it allows you to use one less set of parentheses.

[bokan, 2016/07/08 21:13:00]

Done.

+                # TODO(bokan): Matching configurations often give us bots that don't have a
+                # builder in builders.py's exact_matches. Should we ignore those or be conservative
+                # and assume we need these expectations to make a decision?
+                return False
+
+            if builder_name not in self.builder_results_by_path.keys():
+                _log.error('Failed to find results for builder "%s"' % builder_name)
+                return False
+
+            results_by_path = self.builder_results_by_path[builder_name]
+
+            # No results means the tests were all skipped or all results are passing.
+            if test_expectation_line.path not in results_by_path.keys():
+                continue
+
+            results_for_single_test = results_by_path[test_expectation_line.path]
+
+            if self._expectations_that_were_met(test_expectation_line, results_for_single_test) != set(['PASS']):
+                return False
+
+        return True
+
+    def _has_pass_expectation(self, expectations):
+        return 'PASS' in expectations
+
+    def _expectations_that_were_met(self, test_expectation_line, results_for_single_test):
+        """Returns the set of expectations that appear in the given results.
+
+        e.g. If the test expectations is:
+            bug(test) fast/test.html [Crash Failure Pass]
+
+        And the results are ['TEXT', 'PASS', 'PASS', 'TIMEOUT']
+
+        This method would return [Pass Failure]

[qyearsley, 2016/07/08 17:55:03]

Maybe the "flowiness" of this section could be improved if it were put in one
paragraph, and made more like prose:

    For example, if the test expectations line is
    "bug(test) fast/test.html [Crash Failure Pass]"
    and the results are ['TEXT', 'PASS', 'PASS', 'TIMEOUT'],
    then this method would return set(['PASS', 'FAIL']).

[bokan, 2016/07/08 21:13:00]

Done.

+
+        Args:
+            test_expectation_line: A TestExpectationLine object
+            results_for_single_test: A list of result strings.
+                e.g. ['IMAGE', 'IMAGE', 'PASS']
+
+        Returns:
+            A set containing expectations that occured in the results.
+        """
+        # TODO(bokan): Does this not exist in a more central place?

[qyearsley, 2016/07/08 17:55:03]

Good question, it seems like it should, so if it doesn't, then maybe this should
be put somewhere else in a later CL (somewhere in
layout_tests/models/test_expectations.py?)

[bokan, 2016/07/08 21:13:00]

Ok, I'll leave this TODO here for now and either find an existing method or add
one in a followup.

+        def replace_failing_with_fail(expectation):
+            if expectation in ('TEXT', 'IMAGE', 'IMAGE+TEXT', 'AUDIO'):
+                return 'FAIL'
+            else:
+                return expectation
+
+        actual_results = {replace_failing_with_fail(r) for r in results_for_single_test}
+
+        return set(test_expectation_line.expectations) & actual_results
+
+    def _has_unstrippable_expectations(self, expectations):
+        """ Returns whether any of the given expectations are considered unstrippable.

[qyearsley, 2016/07/08 17:55:04]

Extra space character can be removed.

[bokan, 2016/07/08 21:13:00]

Done.

+
+        Unstrippable expectations are those which should stop a line from being
+        removed regardless of builder bot results.
+
+        Args:
+            expectations: A list of string expectations.
+                E.g. ['PASS', 'FAIL' 'CRASH']
+
+        Returns:
+            True if at least one of the expectations is unstrippable. False
+            otherwise.
+        """
+        unstrippable_expectations = ('REBASELINE', 'NEEDSREBASELINE',
+                                     'NEEDSMANUALREBASELINE', 'SLOW',
+                                     'SKIP')
+        return any(s in expectations for s in unstrippable_expectations)
+
     def get_updated_test_expectations(self):
         """Filters out passing lines from TestExpectations file.
 
@@ -72,7 +177,53 @@ class RemoveFlakesOMatic(object):
         Returns: A TestExpectations object with the passing lines filtered out.
         """
         test_expectations = TestExpectations(self._port, include_overrides=False).expectations()
-        # TODO: Filter the expectations based on results.
+
+        self.builder_results_by_path = {}
+        for builder_name in self._host.builders.all_builder_names():
+            expectations_for_builder = (
+                self._expectations_factory.expectations_for_builder(builder_name)
+            )
+
+            if not expectations_for_builder:
+                # This is not fatal since we may not need to check these
+                # results. If we do need these results we'll log an error later
+                # when trying to check against them.
+                _log.warn('Downloaded results are missing results for builder "%s"' % builder_name)
+                continue
+
+            self.builder_results_by_path[builder_name] = (
+                expectations_for_builder.all_results_by_path()
+            )

[qyearsley, 2016/07/08 17:55:04]

This block of code (lines 181-196), which builds up
self.builder_results_by_path, could be extracted to a separate method.

[bokan, 2016/07/08 21:13:00]

Done.

+
+        expectations_to_remove = []
+
+        for expectation in test_expectations:
+            if self._can_delete_line(expectation):
+                expectations_to_remove.append(expectation)
+
+        for expectation in expectations_to_remove:
+            index = test_expectations.index(expectation)
+            test_expectations.remove(expectation)
+
+            # Remove associated comments and whitespace if we've removed the last expectation under
+            # a comment block. Only remove a comment block if it's not separated from the test
+            # expectation line by whitespace.
+            if index == len(test_expectations) or test_expectations[index].is_whitespace() or test_expectations[index].is_comment():
+                removed_whitespace = False
+                while index and test_expectations[index - 1].is_whitespace():
+                    index = index - 1

[qyearsley, 2016/07/08 17:55:03]

`index = index - 1` can also be written as `index -= 1` if you want.

[bokan, 2016/07/08 21:13:00]

Done.

+                    test_expectations.pop(index)
+                    removed_whitespace = True
+
+                if not removed_whitespace:
+                    while index and test_expectations[index - 1].is_comment():
+                        index = index - 1
+                        test_expectations.pop(index)
+
+                    while index and test_expectations[index - 1].is_whitespace():
+                        index = index - 1
+                        test_expectations.pop(index)

[qyearsley, 2016/07/08 17:55:03]

This block of code (removing associated comments and whitespace directly above
removed expectations takes a while for me to understand -- although I'm
convinced that it works since I've seen the test.

Possible optional changes that might make it easier to understand:

 1. extract to a separate method (which takes index and test_expectations and
modifies test_expectations)?
 2. add more comments?

What do you think about this possible change:

  
def _remove_associated_comments_and_whitespace(expectations, index):
    if not (index == len(expectations) or expectations[index].is_whitespace() or
expectations[index].is_comment()):
        # No whitespace or comments to remove.
        return

    removed_whitespace = False
    while index > 0 and expectations[index - 1].is_whitespace():
        index -= 1
        expectations.pop(index)
        removed_whitespace = True

    if removed_whitespace:
        # No associated comments; whitespace lines are now already removed.
        return
    
    # First remove all comment lines, then remove whitespace lines before those
comments.
    while index > 0 and expectations[index - 1].is_comment():
        index -= 1
        expectations.pop(index)
    while index > 0 and expectations[index - 1].is_whitespace():
        index -= 1
        expectations.pop(index)

[bokan, 2016/07/08 21:13:00]

Done.

+
         return test_expectations
 
     def write_test_expectations(self, test_expectations, test_expectations_file):