Remove all all-PASS testharness baselines after rebaselining.

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

Index: third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
index cd080acc1996324b05bbefe9d1e30c3a01c61694..f690f870917c146018d999473a5f9a8707e8bff8 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
@@ -70,6 +70,19 @@ class AbstractRebaseliningCommand(Command):
     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.
@@ -127,19 +140,6 @@ class BaseInternalRebaselineCommand(AbstractRebaseliningCommand):
                                  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"
@@ -223,13 +223,6 @@ class RebaselineTest(BaseInternalRebaselineCommand):
             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)
 
@@ -524,8 +517,63 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
             # 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()