Refactoring: Replace test_prefix_list variables with TestBaselineSet objects.

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
 # in the documentation and/or other materials provided with the
 # distribution.
 #     * Neither the name of Google Inc. nor the names of its
 # contributors may be used to endorse or promote products derived from
 # this software without specific prior written permission.
 #
 # THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
 # "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
 # LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
 # A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
 # OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
 # SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
 # LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
 # DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
 # THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 # (INCLUDING NEGLIGENCE OR/ OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 from __future__ import print_function
+import collections
 import json
 import logging
 import optparse
 import re
 import sys
 import traceback
 
 from webkitpy.common.memoized import memoized
 from webkitpy.common.net.buildbot import Build
 from webkitpy.common.system.executive import ScriptError
@@ skipping 87 matching lines @@
 
     def update(self, other):
         assert isinstance(other, ChangeSet)
         assert isinstance(other.lines_to_remove, dict)
         for test in other.lines_to_remove:
             if test not in self.lines_to_remove:
                 self.lines_to_remove[test] = []
             self.lines_to_remove[test].extend(other.lines_to_remove[test])
 
 
+class TestBaselineSet(object):
+    """Represents a collection of tests and platforms that can be rebaselined.
+
+    A TestBaselineSet specifies tests to rebaseline along with information
+    about where to fetch the baselines from.
+    """
+
+    def __init__(self, host):
+        self._host = host
+        self._test_prefix_map = collections.defaultdict(list)
+
+    def __iter__(self):
+        return iter(self._iter_combinations())
+
+    def __bool__(self):
+        return bool(self._test_prefix_map)
+
+    def _iter_combinations(self):
+        """Iterates through (test, build) combinations."""
+        port = self._host.port_factory.get()
+        for test_prefix, builds in self._test_prefix_map.iteritems():
+            for build in builds:
+                for test in port.tests([test_prefix]):
+                    yield (test, build)
+
+    def __str__(self):
+        if not self._test_prefix_map:
+            return '<Empty TestBaselineSet>'
+        return '<TestBaselineSet with:\n  ' + '\n  '.join('%s: %s' % pair for pair in self._iter_combinations()) + '>'
+
+    def add(self, test_prefix, build):
+        """Adds an entry for baselines to download for some set of tests.
+
+        Args:
+            test_prefix: This can be a full test path, or directory of tests, or a path with globs.
+            build: A Build object. This specifies where to fetch baselines from.
+        """
+        self._test_prefix_map[test_prefix].append(build)
+
+    def all_builders(self):
+        """Returns all builder names in in this collection."""
+        return sorted({b.builder_name for _, b in self._iter_combinations()})
+
+
 class CopyExistingBaselinesInternal(AbstractRebaseliningCommand):
     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.")
 
     def __init__(self):
         super(CopyExistingBaselinesInternal, self).__init__(options=[
             self.results_directory_option,
             self.suffixes_option,
             self.builder_option,
@@ skipping 159 matching lines @@
         release_builders = []
         for builder_name in self._tool.builders.all_continuous_builder_names():
             port = self._tool.port_factory.get_from_builder_name(builder_name)
             if port.test_configuration().build_type == 'release':
                 release_builders.append(builder_name)
         return release_builders
 
     def _run_webkit_patch(self, args, verbose):
         try:
             verbose_args = ['--verbose'] if verbose else []
-            stderr = self._tool.executive.run_command([self._tool.path()] + verbose_args +
-                                                      args, cwd=self._tool.git().checkout_root, return_stderr=True)
+            stderr = self._tool.executive.run_command(
+                [self._tool.path()] + verbose_args + args,
+                cwd=self._tool.git().checkout_root,
+                return_stderr=True)
             for line in stderr.splitlines():
                 _log.warning(line)
         except ScriptError:
             traceback.print_exc(file=sys.stderr)
 
     def _builders_to_fetch_from(self, builders_to_check):
         """Returns the subset of builders that will cover all of the baseline
         search paths used in the input list.
 
         In particular, if the input list contains both Release and Debug
         versions of a configuration, we *only* return the Release version
         (since we don't save debug versions of baselines).
 
         Args:
             builders_to_check: List of builder names.
+
+        Returns:
+            A list of builders that we may fetch from.

[wkorman, 2017/03/21 19:02:13]

Document that it's returned sorted? Presuming that's important, which it seems
like it is since it looks like it's added in this change

[qyearsley, 2017/03/21 21:47:22]

It's not really important that it was sorted; the reason for changing that was
so that the return order would be consistent for the unit test. Since order
isn't important here, I just changed this to return a set instead.

         """
         release_builders = set()
         debug_builders = set()
-        builders_to_fallback_paths = {}
         for builder in builders_to_check:
             port = self._tool.port_factory.get_from_builder_name(builder)
             if port.test_configuration().build_type == 'release':
                 release_builders.add(builder)
             else:
                 debug_builders.add(builder)
+
+        builders_to_fallback_paths = {}
         for builder in list(release_builders) + list(debug_builders):
             port = self._tool.port_factory.get_from_builder_name(builder)
             fallback_path = port.baseline_search_path()
             if fallback_path not in builders_to_fallback_paths.values():
                 builders_to_fallback_paths[builder] = fallback_path
-        return builders_to_fallback_paths.keys()
 
-    @staticmethod
-    def _builder_names(builds):
-        # TODO(qyearsley): If test_prefix_list dicts are converted to instances
-        # of some class, then this could be replaced with  a method on that class.
-        return [b.builder_name for b in builds]
+        return sorted(builders_to_fallback_paths)
 
-    def _rebaseline_commands(self, test_prefix_list, options):
+    def _rebaseline_commands(self, test_baseline_set, options):
         path_to_webkit_patch = self._tool.path()
         cwd = self._tool.git().checkout_root
         copy_baseline_commands = []
         rebaseline_commands = []
         lines_to_remove = {}
-        port = self._tool.port_factory.get()
 
-        for test_prefix in test_prefix_list:
-            for test in port.tests([test_prefix]):
-                builders_to_fetch_from = self._builders_to_fetch_from(self._builder_names(test_prefix_list[test_prefix]))
-                for build in sorted(test_prefix_list[test_prefix]):
-                    builder, build_number = build.builder_name, build.build_number
-                    if builder not in builders_to_fetch_from:
-                        break
+        for test, build in test_baseline_set:
+            if build.builder_name not in self._builders_to_fetch_from(test_baseline_set.all_builders()):
+                continue
 
-                    suffixes = self._suffixes_for_actual_failures(test, build)
-                    if not suffixes:
-                        # If we're not going to rebaseline the test because it's passing on this
-                        # builder, we still want to remove the line from TestExpectations.
-                        if test not in lines_to_remove:
-                            lines_to_remove[test] = []
-                        lines_to_remove[test].append(builder)
-                        continue
+            suffixes = self._suffixes_for_actual_failures(test, build)
+            if not suffixes:
+                # If we're not going to rebaseline the test because it's passing on this
+                # builder, we still want to remove the line from TestExpectations.
+                if test not in lines_to_remove:
+                    lines_to_remove[test] = []
+                lines_to_remove[test].append(build.builder_name)
+                continue
 
-                    args = ['--suffixes', ','.join(suffixes), '--builder', builder, '--test', test]
+            args = ['--suffixes', ','.join(suffixes), '--builder', build.builder_name, '--test', test]
+            if options.verbose:
+                args.append('--verbose')
 
-                    if options.verbose:
-                        args.append('--verbose')
+            copy_command = [self._tool.executable, path_to_webkit_patch, 'copy-existing-baselines-internal'] + args
+            copy_baseline_commands.append(tuple([copy_command, cwd]))
 
-                    copy_baseline_commands.append(
-                        tuple([[self._tool.executable, path_to_webkit_patch, 'copy-existing-baselines-internal'] + args, cwd]))
+            if build.build_number:
+                args.extend(['--build-number', str(build.build_number)])
+            if options.results_directory:
+                args.extend(['--results-directory', options.results_directory])
 
-                    if build_number:
-                        args.extend(['--build-number', str(build_number)])
-                    if options.results_directory:
-                        args.extend(['--results-directory', options.results_directory])
+            rebaseline_command = [self._tool.executable, path_to_webkit_patch, 'rebaseline-test-internal'] + args
+            rebaseline_commands.append(tuple([rebaseline_command, cwd]))
 
-                    rebaseline_commands.append(
-                        tuple([[self._tool.executable, path_to_webkit_patch, 'rebaseline-test-internal'] + args, cwd]))
         return copy_baseline_commands, rebaseline_commands, lines_to_remove
 
     @staticmethod
     def _extract_expectation_line_changes(command_results):
         """Parses the JSON lines from sub-command output and returns the result as a ChangeSet."""
         change_set = ChangeSet()
         for _, stdout, _ in command_results:
             updated = False
             for line in filter(None, stdout.splitlines()):
                 try:
                     parsed_line = json.loads(line)
                     change_set.update(ChangeSet.from_dict(parsed_line))
                     updated = True
                 except ValueError:
                     _log.debug('"%s" is not a JSON object, ignoring', line)
             if not updated:
                 # TODO(qyearsley): This probably should be an error. See http://crbug.com/649412.
                 _log.debug('Could not add file based off output "%s"', stdout)
         return change_set
 
-    def _optimize_baselines(self, test_prefix_list, verbose=False):
+    def _optimize_baselines(self, test_baseline_set, verbose=False):
+        """Returns a list of commands to run in parallel to de-duplicate baselines."""
+        tests_to_suffixes = collections.defaultdict(set)
+        for test, build in test_baseline_set:
+            builders_to_fetch_from = self._builders_to_fetch_from(test_baseline_set.all_builders())
+            if build.builder_name not in builders_to_fetch_from:
+                continue
+            tests_to_suffixes[test].update(self._suffixes_for_actual_failures(test, build))
+
         optimize_commands = []
-        for test in test_prefix_list:
-            all_suffixes = set()
-            builders_to_fetch_from = self._builders_to_fetch_from(self._builder_names(test_prefix_list[test]))
-            for build in sorted(test_prefix_list[test]):
-                if build.builder_name not in builders_to_fetch_from:
-                    break
-                all_suffixes.update(self._suffixes_for_actual_failures(test, build))
-
+        for test, suffixes in tests_to_suffixes.iteritems():
             # No need to optimize baselines for a test with no failures.
-            if not all_suffixes:
+            if not suffixes:
                 continue
-
             # FIXME: We should propagate the platform options as well.
-            cmd_line = ['--suffixes', ','.join(all_suffixes), test]
+            args = ['--suffixes', ','.join(suffixes), test]
             if verbose:
-                cmd_line.append('--verbose')
-
+                args.append('--verbose')
             path_to_webkit_patch = self._tool.path()
             cwd = self._tool.git().checkout_root
-            optimize_commands.append(tuple([[self._tool.executable, path_to_webkit_patch, 'optimize-baselines'] + cmd_line, cwd]))
+            command = [self._tool.executable, path_to_webkit_patch, 'optimize-baselines'] + args
+            optimize_commands.append(tuple([command, cwd]))
+
         return optimize_commands
 
     def _update_expectations_files(self, lines_to_remove):
         # FIXME: This routine is way too expensive. We're creating O(n ports) TestExpectations objects.
         # This is slow and uses a lot of memory.
         tests = lines_to_remove.keys()
         to_remove = []
 
         # This is so we remove lines for builders that skip this test, e.g. Android skips most
         # tests and we don't want to leave stray [ Android ] lines in TestExpectations..
@@ skipping 29 matching lines @@
 
         command_results = self._tool.executive.run_in_parallel(commands)
         for _, _, stderr in command_results:
             if stderr:
                 _log.error(stderr)
 
         change_set = self._extract_expectation_line_changes(command_results)
 
         return change_set.lines_to_remove
 
-    def rebaseline(self, options, test_prefix_list):
+    def rebaseline(self, options, test_baseline_set):
         """Fetches new baselines and removes related test expectation lines.
 
         Args:
             options: An object with the command line options.
-            test_prefix_list: A dict of test names to Build objects for new
-                baselines. For example:
-                {
-                    "some/test.html": [Build("builder-1", 412),
-                                       Build("builder-2", 100)]},
-                    "some/other.html": [Build("builder-1", 412)}
-                }
-                This would mean that new text baselines should be
-                downloaded for "some/test.html" on both builder-1
-                (build 412) and builder-2 (build 100), and new text
-                baselines should be downloaded for "some/other.html"
-                but only from builder-1.
-                TODO(qyearsley): Replace test_prefix_list everywhere
-                with some sort of class that contains the same data.
+            test_baseline_set: A TestRebaselineSet instance, which represents
+                a set of tests/platform combinations to rebaseline.

[wkorman, 2017/03/21 19:02:13]

TestRebaselineSet -> TestBaselineSet

[qyearsley, 2017/03/21 21:47:22]

Fixed

         """
         if self._tool.git().has_working_directory_changes(pathspec=self._layout_tests_dir()):
             _log.error('There are uncommitted changes in the layout tests directory; aborting.')
             return
 
-        for test, builds_to_check in sorted(test_prefix_list.items()):
+        for test in sorted({t for t, _ in test_baseline_set}):
             _log.info('Rebaselining %s', test)
-            for build in sorted(builds_to_check):
-                _log.debug('  %s', build)
 
         copy_baseline_commands, rebaseline_commands, extra_lines_to_remove = self._rebaseline_commands(
-            test_prefix_list, options)
+            test_baseline_set, options)
         lines_to_remove = {}
 
         self._run_in_parallel(copy_baseline_commands)
         lines_to_remove = self._run_in_parallel(rebaseline_commands)
 
         for test in extra_lines_to_remove:
             if test in lines_to_remove:
                 lines_to_remove[test] = lines_to_remove[test] + extra_lines_to_remove[test]
             else:
                 lines_to_remove[test] = extra_lines_to_remove[test]
 
         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._run_in_parallel(self._optimize_baselines(test_baseline_set, options.verbose))
 
-        self._remove_all_pass_testharness_baselines(test_prefix_list)
+        self._remove_all_pass_testharness_baselines(test_baseline_set)
 
         self._tool.git().add_list(self.unstaged_baselines())
 
     def unstaged_baselines(self):
         """Returns absolute paths for unstaged (including untracked) baselines."""
         baseline_re = re.compile(r'.*[\\/]LayoutTests[\\/].*-expected\.(txt|png|wav)$')
         unstaged_changes = self._tool.git().unstaged_changes()
         return sorted(self._tool.git().absolute_path(path) for path in unstaged_changes if re.match(baseline_re, path))
 
-    def _remove_all_pass_testharness_baselines(self, test_prefix_list):
+    def _remove_all_pass_testharness_baselines(self, test_baseline_set):
         """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.
         """
         filesystem = self._tool.filesystem
-        baseline_paths = self._all_baseline_paths(test_prefix_list)
+        baseline_paths = self._possible_baseline_paths(test_baseline_set)
         for path in baseline_paths:
-            if not (filesystem.exists(path) and
-                    filesystem.splitext(path)[1] == '.txt'):
+            if not (filesystem.exists(path) and filesystem.splitext(path)[1] == '.txt'):
                 continue
             contents = filesystem.read_text_file(path)
             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):
+    def _possible_baseline_paths(self, test_baseline_set):
         """Returns 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.
-                TODO(qyearsley): If a class is added to replace
-                test_prefix_list, then this can be made a method on
-                that class.
+            test_baseline_set: A TestBaselineSet instance.
 
         Returns:
-            A list of absolute paths to possible baseline files,
-            which may or may not exist on the local filesystem.
+            A list of absolute paths where baselines could possibly 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])
-
-            for build in test_prefix_list[test_prefix]:
-                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 BASELINE_SUFFIX_LIST
-                ])
-
-            baseline_paths.extend([
-                filesystem.join(self._layout_tests_dir(), self._file_name_for_expected_result(test, suffix))
-                for test in tests for suffix in BASELINE_SUFFIX_LIST
-            ])
-
+        baseline_paths = set()
+        for test, build in test_baseline_set:
+            filenames = [self._file_name_for_expected_result(test, suffix) for suffix in BASELINE_SUFFIX_LIST]
+            port_baseline_dir = self._baseline_directory(build.builder_name)
+            baseline_paths.update({filesystem.join(port_baseline_dir, filename) for filename in filenames})
+            baseline_paths.update({filesystem.join(self._layout_tests_dir(), filename) for filename in filenames})
         return sorted(baseline_paths)
 
     def _layout_tests_dir(self):
         return self._tool.port_factory.get().layout_tests_dir()
 
     def _suffixes_for_actual_failures(self, test, build):
         """Gets the baseline suffixes for actual mismatch failures in some results.
 
         Args:
             test: A full test path string.
@@ skipping 30 matching lines @@
 
 class RebaselineExpectations(AbstractParallelRebaselineCommand):
     name = 'rebaseline-expectations'
     help_text = 'Rebaselines the tests indicated in TestExpectations.'
     show_in_main_help = True
 
     def __init__(self):
         super(RebaselineExpectations, self).__init__(options=[
             self.no_optimize_option,
         ] + self.platform_options)
-        self._test_prefix_list = None
+        self._test_baseline_set = None
 
     @staticmethod
     def _tests_to_rebaseline(port):
         tests_to_rebaseline = []
         for path, value in port.expectations_dict().items():
             expectations = TestExpectations(port, include_overrides=False, expectations_dict={path: value})
             for test in expectations.get_rebaselining_failures():
                 tests_to_rebaseline.append(test)
         return tests_to_rebaseline
 
     def _add_tests_to_rebaseline(self, port_name):
         builder_name = self._tool.builders.builder_name_for_port_name(port_name)
         if not builder_name:
             return
         tests = self._tests_to_rebaseline(self._tool.port_factory.get(port_name))
 
         if tests:
             _log.info('Retrieving results for %s from %s.', port_name, builder_name)
 
         for test_name in tests:
             _log.info('    %s', test_name)
-            if test_name not in self._test_prefix_list:
-                self._test_prefix_list[test_name] = []
-            self._test_prefix_list[test_name].append(Build(builder_name))
+            self._test_baseline_set.add(test_name, Build(builder_name))
 
     def execute(self, options, args, tool):
         self._tool = tool
+        self._test_baseline_set = TestBaselineSet(tool)
         options.results_directory = None
-        self._test_prefix_list = {}
         port_names = tool.port_factory.all_port_names(options.platform)
         for port_name in port_names:
             self._add_tests_to_rebaseline(port_name)
-        if not self._test_prefix_list:
+        if not self._test_baseline_set:
             _log.warning('Did not find any tests marked Rebaseline.')
             return
 
-        self.rebaseline(options, self._test_prefix_list)
+        self.rebaseline(options, self._test_baseline_set)
 
 
 class Rebaseline(AbstractParallelRebaselineCommand):
     name = 'rebaseline'
-    help_text = 'Rebaseline tests with results from the build bots.'
+    help_text = 'Rebaseline tests with results from the continuous builders.'
     show_in_main_help = True
     argument_names = '[TEST_NAMES]'
 
     def __init__(self):
         super(Rebaseline, self).__init__(options=[
             self.no_optimize_option,
             # FIXME: should we support the platform options in addition to (or instead of) --builders?
             self.results_directory_option,
             optparse.make_option('--builders', default=None, action='append',
                                  help=('Comma-separated-list of builders to pull new baselines from '
@@ skipping 10 matching lines @@
             _log.error('Must list tests to rebaseline.')
             return
 
         if options.builders:
             builders_to_check = []
             for builder_names in options.builders:
                 builders_to_check += builder_names.split(',')
         else:
             builders_to_check = self._builders_to_pull_from()
 
-        test_prefix_list = {}
+        test_baseline_set = TestBaselineSet(tool)
 
         for builder in builders_to_check:
-            for test in args:
-                if test not in test_prefix_list:
-                    test_prefix_list[test] = []
-                test_prefix_list[test].append(Build(builder))
+            for test_prefix in args:
+                test_baseline_set.add(test_prefix, Build(builder))
 
-        if options.verbose:
-            _log.debug('rebaseline-json: ' + str(test_prefix_list))
+        _log.debug('Rebaselining: %s', test_baseline_set)
 
-        self.rebaseline(options, test_prefix_list)
+        self.rebaseline(options, test_baseline_set)