Remove all all-PASS testharness baselines after rebaselining.

third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py

 # Copyright (c) 2010 Google Inc. All rights reserved.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 # notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 # copyright notice, this list of conditions and the following disclaimer
@@ skipping 52 matching lines @@
 
     def __init__(self, options=None):
         super(AbstractRebaseliningCommand, self).__init__(options=options)
         self._baseline_suffix_list = BASELINE_SUFFIX_LIST
         self.expectation_line_changes = ChangeSet()
         self._tool = None
 
     def _print_expectation_line_changes(self):
         print(json.dumps(self.expectation_line_changes.to_dict()))
 
+    def _baseline_directory(self, builder_name):
+        port = self._tool.port_factory.get_from_builder_name(builder_name)
+        return port.baseline_version_dir()
+
+    def _test_root(self, test_name):
+        return self._tool.filesystem.splitext(test_name)[0]
+
+    def _file_name_for_actual_result(self, test_name, suffix):
+        return "%s-actual.%s" % (self._test_root(test_name), suffix)
+
+    def _file_name_for_expected_result(self, test_name, suffix):
+        return "%s-expected.%s" % (self._test_root(test_name), suffix)
+
 
 class ChangeSet(object):
     """A record of TestExpectation lines to remove.
 
     TODO(qyearsley): Remove this class, track list of lines to remove directly
     in an attribute of AbstractRebaseliningCommand.
     """
     def __init__(self, lines_to_remove=None):
         self.lines_to_remove = lines_to_remove or {}
 
@@ skipping 37 matching lines @@
     def __init__(self):
         super(BaseInternalRebaselineCommand, self).__init__(options=[
             self.results_directory_option,
             self.suffixes_option,
             optparse.make_option("--builder", help="Builder to pull new baselines from."),
             optparse.make_option("--test", help="Test to rebaseline."),
             optparse.make_option("--build-number", default=None, type="int",
                                  help="Optional build number; if not given, the latest build is used."),
         ])
 
-    def _baseline_directory(self, builder_name):
-        port = self._tool.port_factory.get_from_builder_name(builder_name)
-        return port.baseline_version_dir()
-
-    def _test_root(self, test_name):
-        return self._tool.filesystem.splitext(test_name)[0]
-
-    def _file_name_for_actual_result(self, test_name, suffix):
-        return "%s-actual.%s" % (self._test_root(test_name), suffix)
-
-    def _file_name_for_expected_result(self, test_name, suffix):
-        return "%s-expected.%s" % (self._test_root(test_name), suffix)
-
 
 class CopyExistingBaselinesInternal(BaseInternalRebaselineCommand):
     name = "copy-existing-baselines-internal"
     help_text = ("Copy existing baselines down one level in the baseline order to ensure "
                  "new baselines don't break existing passing platforms.")
 
     @memoized
     def _immediate_predecessors_in_fallback(self, path_to_rebaseline):
         port_names = self._tool.port_factory.all_port_names()
         immediate_predecessors = []
@@ skipping 63 matching lines @@
 class RebaselineTest(BaseInternalRebaselineCommand):
     name = "rebaseline-test-internal"
     help_text = "Rebaseline a single test from a buildbot. Only intended for use by other webkit-patch commands."
 
     def _save_baseline(self, data, target_baseline):
         if not data:
             _log.debug("No baseline data to save.")
             return
 
         filesystem = self._tool.filesystem
-        if is_all_pass_testharness_result(data):
-            _log.debug("The new baseline is a passing testharness result with "
-                       "no console warnings or errors, so it will not be saved.")
-            if filesystem.exists(target_baseline):
-                filesystem.remove(target_baseline)
-            return
-
         filesystem.maybe_make_directory(filesystem.dirname(target_baseline))
         filesystem.write_binary_file(target_baseline, data)
 
     def _rebaseline_test(self, builder_name, test_name, suffix, results_url):
         baseline_directory = self._baseline_directory(builder_name)
 
         source_baseline = "%s/%s" % (results_url, self._file_name_for_actual_result(test_name, suffix))
         target_baseline = self._tool.filesystem.join(baseline_directory, self._file_name_for_expected_result(test_name, suffix))
 
         _log.debug("Retrieving source %s for target %s.", source_baseline, target_baseline)
@@ skipping 274 matching lines @@
 
         if lines_to_remove:
             self._update_expectations_files(lines_to_remove)
 
         if options.optimize:
             # TODO(wkorman): Consider changing temporary branch to base off of HEAD rather than
             # origin/master to ensure we run baseline optimization processes with the same code as
             # auto-rebaseline itself.
             self._run_in_parallel(self._optimize_baselines(test_prefix_list, options.verbose))
 
+        self._remove_all_pass_testharness_baselines(test_prefix_list)
+
         self._tool.scm().add_all(pathspec=self._layout_tests_dir())
 
+    def _remove_all_pass_testharness_baselines(self, test_prefix_list):
+        """Removes all of the all-PASS baselines for the given builders and tests.
+
+        In general, for testharness.js tests, the absence of a baseline
+        indicates that the test is expected to pass. When rebaselining,
+        new all-PASS baselines may be downloaded, but they should not be kept.

[wkorman, 2016/10/25 00:14:20]

Is removing restoring the repo to the baselines-don't-exist state, or are we
relying on a subsequent 'git checkout' or otherwise to restore them?

[qyearsley, 2016/10/25 00:33:04]

I think it could be either removing an existing file or removing a newly-added
file.

If there were existing baselines with FAIL lines but now they're passing, then
rebaselining should remove the file, so in general we don't want to restore the
file (although the user could use git checkout to get the file back if they want
to).

+        """
+        filesystem = self._tool.filesystem
+        baseline_paths = self._all_baseline_paths(test_prefix_list)
+        for path in baseline_paths:
+            if not (filesystem.exists(path) and
+                    filesystem.splitext(path)[1] == '.txt'):
+                continue
+            contents = filesystem.read_text_file(path)

[wkorman, 2016/10/25 00:14:20]

How many of these do we expect there to be typically? I assume on the order of
tens that would show up in a change that actually affected some of these; and
that in a degenerate case there are maybe hundreds (thousands?) in LayoutTests
directory. In any case it's probably fast enough even in that degenerate case
but figured worth a checkpoint.

[qyearsley, 2016/10/25 00:33:04]

Right -- hypothetically there could be hundreds of tests tests that all have
platform-specific baselines for 10 platforms, but reading up to 10000 small-ish
text files should hopefully be alright...

Note: I just tried `time cat $(find . -name '*.html' | head -n 10000) | wc -l`,
which read 406854 lines from 10000 files in about 1-2 seconds on my workstation.

But in general, there would more likely be less than 100 tests with 1-5
baselines each.

+            if is_all_pass_testharness_result(contents):
+                _log.info('Removing all-PASS testharness baseline: %s', path)
+                filesystem.remove(path)
+
+    def _all_baseline_paths(self, test_prefix_list):
+        """Return file paths for all baselines for the given tests and builders.
+
+        Args:
+            test_prefix_list: A dict mapping test prefixes, which could be
+                directories or full test paths, to builds to baseline suffixes.
+                TODO(qyearsley): If a class is added to replace test_prefix_list,
+                then this can be made a method on that class.
+
+        Returns:
+            A list of absolute paths to baseline files.

[qyearsley, 2016/10/25 00:33:04]

Currently this actually returns potential places where there *could* be
baselines, not places where baseline files currently actually exist. If it makes
more sense, then maybe this file could be changed to only return paths for files
that exist?

[wkorman, 2016/10/25 00:37:17]

Seems reasonable, this method is only used in this file, are you talking about
changing the results returned from other methods? The only thing I could see as
issue is introducing file I/O ops where there are none today.

[qyearsley, 2016/10/25 20:13:34]

I was thinking of filtering out files that don't exist in this function rather
than in _remove_all_pass_testharness_baselines, but it's basically the same one
way or the other. I added a note in the docstring of this function to note that
it may return file paths for files that don't exist.

+        """
+        filesystem = self._tool.filesystem
+        baseline_paths = []
+        port = self._tool.port_factory.get()
+
+        for test_prefix in test_prefix_list:
+            tests = port.tests([test_prefix])
+            all_suffixes = set()
+
+            for build, suffixes in test_prefix_list[test_prefix].iteritems():
+                all_suffixes.update(suffixes)
+                port_baseline_dir = self._baseline_directory(build.builder_name)
+                baseline_paths.extend([
+                    filesystem.join(port_baseline_dir, self._file_name_for_expected_result(test, suffix))
+                    for test in tests for suffix in suffixes
+                ])
+
+            baseline_paths.extend([
+                filesystem.join(self._layout_tests_dir(), self._file_name_for_expected_result(test, suffix))
+                for test in tests for suffix in all_suffixes
+            ])
+
+        return baseline_paths
+
     def _layout_tests_dir(self):
         return self._tool.port_factory.get().layout_tests_dir()
 
     def _suffixes_for_actual_failures(self, test, build, existing_suffixes):
         """Gets the baseline suffixes for actual mismatch failures in some results.
 
         Args:
             test: A full test path string.
             build: A Build object.
             existing_suffixes: A collection of all suffixes to consider.
@@ skipping 118 matching lines @@
             for test in args:
                 if test not in test_prefix_list:
                     test_prefix_list[test] = {}
                 build = Build(builder)
                 test_prefix_list[test][build] = suffixes_to_update
 
         if options.verbose:
             _log.debug("rebaseline-json: " + str(test_prefix_list))
 
         self.rebaseline(options, test_prefix_list)