Fill out implementation of UpdateTestExpectations script

third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/update_test_expectations.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.
 
 """Updates TestExpectations based on results in builder bots.
 
 Scans the TestExpectations file and uses results from actual builder bots runs
 to remove tests that are marked as flaky but don't fail in the specified way.
 
 E.g. If a test has this expectation:
@@ skipping 44 matching lines @@
                                                   expectations_file)
 
 
 class RemoveFlakesOMatic(object):
     def __init__(self, host, port, bot_test_expectations_factory):
         self._host = host
         self._port = port
         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.
 
         Reads the current TestExpectatoins file and, using results from the
         build bots, removes lines that are passing. That is, removes lines that
         were not needed to keep the bots green.
 
         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):
         """Writes the given TestExpectations object to the filesystem.
 
         Args:
             test_expectatoins: The TestExpectations object to write.
             test_expectations_file: The full file path of the Blink
                 TestExpectations file. This file will be overwritten.
         """
         self._host.filesystem.write_text_file(
             test_expectations_file,
             TestExpectations.list_to_string(test_expectations, reconstitute_only_these=[]))