Added update_test_expectations script to remove non-flaky test expectations.

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

+import optparse
+
+from webkitpy.layout_tests.models.test_expectations import TestExpectations
+from webkitpy.layout_tests.layout_package.bot_test_expectations import BotTestExpectationsFactory
+from webkitpy.layout_tests.port import builders
+
+
+def main(host, argv):
+    parser = optparse.OptionParser(usage='%prog ')
+    # parser.add_option('-f', '--forward', action='store', type='int',
+    #                   help='group times by first N directories of test')
+    # parser.add_option('-b', '--backward', action='store', type='int',
+    #                  help='group times by last N directories of test')
+    # parser.add_option('--fastest', action='store', type='float',
+    #                   help='print a list of tests that will take N % of the time')
+
+    epilog = """
+       Test test test"""
+    parser.epilog = '\n'.join(s.lstrip() for s in epilog.splitlines())
+
+    options, args = parser.parse_args(argv)

[qyearsley, 2016/03/07 19:09:53]

This could be switched to argparse, the successor to optparse:

    parser = argparse.ArgumentParser(epilog='...')
    parser.add_argument('-f', '--forward', type=int
                        help='group times by first N directories of test')
    parser.add_argument('-b', '--backward', type=int,
                        help='group times by last N directories of test')
    parser.add_argument('--fastest', type=float,
                        help='print a list of tests that will take N % of the
time')
    args = parser.parse_args(argv)

[bokan, 2016/03/07 22:59:15]

Thanks, done.

+
+    remove_flakes_o_matic = RemoveFlakesOMatic()
+    remove_flakes_o_matic.execute(host)

[qyearsley, 2016/03/07 19:09:53]

Possible refactoring: Maybe RemoveFlakesOMatic.execute could be changed to a
top-level function called remove_flakes(expectations)?

    port = host.port_factory.get()
    test_expectations = TestExpectations(,
include_overrides=False)._expectations

    updated_test_expectations = remove_flakes(test_expectations)

    expectations_file = port.path_to_generic_test_expectations_file()
    host.filesystem.write_text_file(
        expectations_file,
        TestExpectations.list_to_string(updated_test_expectations,
reconstitute_only_these=[]))

Is there an advantage to using a RemoveFlakesOMatic class?

[bokan, 2016/03/07 22:59:15]

My patch is based on Ojan's CL which originally used webkit-patch so I moved it
over as a class for convenience. I suspect it might be easier to unittest by
keeping it as a class (allows feeding it mock objects, faking its state, etc).
I'm about to start writing tests so I'll find out how testing in these scripts
actually works. I'll keep in mind whether I can convert it to a standalone
function.

+
+
+class RemoveFlakesOMatic:

[qyearsley, 2016/03/07 19:09:53]

What do instances of this class represent? Could the methods here could be
changed to top-level functions or static methods?

[bokan, 2016/03/07 22:59:16]

Addressed above.

+    def __init__(self):
+        self.expectations_factory = BotTestExpectationsFactory()
+
+    def _can_delete_line(self, test_expectation_line):

[qyearsley, 2016/03/07 19:09:53]

test_expectation_line is an instance of test_expectations.TestExpectationLine,
right? (Could potentially add a docstring to explicitly state this.)

What is the class of the objects in test_expectations? Are they strings?

[bokan, 2016/03/07 22:59:15]

Done.
test_expectations is a list of TestExpectationLine instances.

+        expectations = test_expectation_line.expectations
+        if len(expectations) < 2:
+            return False

[qyearsley, 2016/03/07 19:09:53]

What does an expectation list of length 0 or 1 indicate? Why does it mean we
can't delete the line?

[bokan, 2016/03/07 22:59:15]

It means the test isn't flaky and this script (right now) only removes flaky
lines. e.g. test/mytest.html [ Failure ] doesn't indicate a flake. I can't think
of a reason why we shouldn't update these kinds of lines too.

I'm not sure if a 0 length expectation is possible.

+
+        if self._has_unstripable_expectations(expectations):
+            return False
+
+        if not self._has_pass_expectation(expectations):
+            return False
+
+        # 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 = builders.builder_name_for_specifiers(config.version, config.build_type)
+
+            if not builder_name:
+                # TODO(bokan): We probably want to report a problem if we can't get turn the
+                # specifiers into a bot.
+                # TODO(bokan): Matching configurations often give us bots that don't have a

[bokan, 2016/03/04 03:44:10]

There seems to be more matching_configurations that builders in builders.py, am
I using the right list or do the others not matter?

+                # builder in builders.py's exact_matches. Should we ignore those or be conservative
+                # and assume those need these expectations?
+                return False
+
+            if builder_name not in self.builder_results_by_path.keys():
+                # TODO(bokan): We probably want to report a problem if we can't get results for
+                # a bot.
+                return False
+
+            results_by_path = self.builder_results_by_path[builder_name]
+
+            # No results means the test was all skipped or all results are passing.
+            if test_expectation_line.path not in results_by_path.keys():
+                continue
+
+            results_for_test = results_by_path[test_expectation_line.path]
+
+            if self._met_expectations(test_expectation_line, results_for_test) != set(['PASS']):
+                return False
+
+        return True
+
+    def _has_pass_expectation(self, expectations):
+        return 'PASS' in expectations
+
+    def _met_expectations(self, test_expectation_line, results_for_test):
+        # TODO(bokan): Does this not exist in a more central place?

[bokan, 2016/03/04 03:44:10]

Ditto here, is there a function that turns "failure types" into "FAIL"?

+        def replace_failing_with_fail(expectation):
+            if expectation in ('TEXT', 'IMAGE', 'IMAGE+TEXT', 'AUDIO'):
+                return 'FAIL'
+            else:
+                return expectation
+
+        test_results = set(results_for_test)

[qyearsley, 2016/03/07 19:09:53]

This line could be removed without changing the output;

From the variable names, results_for_test and test_results sound like two names
for the same thing.

[bokan, 2016/03/07 22:59:15]

Yes, you're right, removed.

+
+        actual_results = set(map(replace_failing_with_fail, test_results))

[qyearsley, 2016/03/07 19:09:54]

Possible rephrasing:

    actual_results = {replace_failing_with_fail(r) for r in results_for_test}

[bokan, 2016/03/07 22:59:15]

Done.

+
+        return set(test_expectation_line.expectations) & actual_results
+
+    def _has_unstripable_expectations(self, expectations):

[qyearsley, 2016/03/07 19:09:53]

unstripable -> unstrippable

[bokan, 2016/03/07 22:59:15]

Done.

+        return ('REBASELINE' in expectations
+                or 'NEEDSREBASLINE' in expectations
+                or 'NEEDSMANUALREBASELINE' in expectations
+                or 'SLOW' in expectations)

[qyearsley, 2016/03/07 19:09:53]

Possible alternate phrasing:

    passing_statuses = ('PASS', 'SLOW', 'NEEDSREBASELINE',
                        'NEEDSMANUALREBASELINE')
    return any(s in expectations for s in passing_statuses)

[bokan, 2016/03/07 22:59:15]

Done.

+
+    def execute(self, host):
+        port = host.port_factory.get()
+        test_expectations = TestExpectations(port, include_overrides=False)._expectations

[qyearsley, 2016/03/07 19:09:53]

1. What is the class of the objects in test_expectations?
2. This is accessing a private property, maybe TestExpectations should have a
get_expectations method, or that property should be public?

[bokan, 2016/03/07 22:59:15]

test_expectations is a list of TestExpectationLine.
Done.

+
+        self.builder_results_by_path = {}
+        for builder_name in builders.all_builder_names():
+            self.builder_results_by_path[builder_name] = (
+                self.expectations_factory.expectations_for_builder(builder_name).all_results_by_path()
+            )
+            pass
+
+        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 if we've removed the last expectation under a comment block.
+            if index == len(test_expectations) or test_expectations[index].is_whitespace_or_comment():
+                while index and test_expectations[index - 1].is_whitespace_or_comment():
+                    index = index - 1
+                    test_expectations.pop(index)
+
+        expectations_file = port.path_to_generic_test_expectations_file()

[bokan, 2016/03/04 03:44:10]

Presumably we also want to update ASAN and other bots of that sort?

+        host.filesystem.write_text_file(
+            expectations_file,
+            TestExpectations.list_to_string(test_expectations, reconstitute_only_these=[]))