Simplification: Remove support for specifying suffix lists when 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 347 matching lines @@
         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]):
+

[Xianzhu, 2017/03/18 00:07:36]

Nit: The blank line just looks weird. Feel free to keep it if this is
intentional though.

                     builder, build_number = build.builder_name, build.build_number
+
                     if builder not in builders_to_fetch_from:
                         break
-                    else:
-                        actual_failures_suffixes = self._suffixes_for_actual_failures(
-                            test, build, test_prefix_list[test_prefix][build])
-                    if not actual_failures_suffixes:
+
+                    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 = ','.join(actual_failures_suffixes)
-                    args = ['--suffixes', suffixes, '--builder', builder, '--test', test]
+                    args = ['--suffixes', ','.join(suffixes), '--builder', builder, '--test', test]
 
                     if options.verbose:
                         args.append('--verbose')
 
                     copy_baseline_commands.append(
                         tuple([[self._tool.executable, path_to_webkit_patch, 'copy-existing-baselines-internal'] + args, cwd]))
 
                     if build_number:
                         args.extend(['--build-number', str(build_number)])
                     if options.results_directory:
@@ skipping 22 matching lines @@
         return change_set
 
     def _optimize_baselines(self, test_prefix_list, verbose=False):
         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, test_prefix_list[test][build]))
+                all_suffixes.update(self._suffixes_for_actual_failures(test, build))
 
             # No need to optimize baselines for a test with no failures.
             if not all_suffixes:
                 continue
 
             # FIXME: We should propagate the platform options as well.
             cmd_line = ['--suffixes', ','.join(all_suffixes), test]
             if verbose:
                 cmd_line.append('--verbose')
 
@@ skipping 43 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):
-        """Downloads new baselines in parallel, then updates expectations files
-        and optimizes baselines.
+        """Fetches new baselines and removes related test expectation lines.
 
         Args:
-            options: An object with the options passed to the current command.
-            test_prefix_list: A map of test names to Build objects to file suffixes
-                for new baselines. For example:
+            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): ["txt"], Build("builder-2", 100): ["txt"]},
-                    "some/other.html": {Build("builder-1", 412): ["txt"]}
+                    "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.
+                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.
         """
         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()):
             _log.info('Rebaselining %s', test)
-            for build, suffixes in sorted(builds_to_check.items()):
-                _log.debug('  %s: %s', build, ','.join(suffixes))
+            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)
         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:
@@ skipping 32 matching lines @@
         for path in baseline_paths:
             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):
-        """Return file paths for all baselines for the given tests and builders.
+        """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 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.
+                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.
 
         Returns:
             A list of absolute paths to possible baseline files,
             which may or may not exist on the local filesystem.
         """
         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)
+            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 suffixes
+                    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 all_suffixes
+                for test in tests for suffix in BASELINE_SUFFIX_LIST
             ])
 
         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, existing_suffixes):
+    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.
             build: A Build object.
-            existing_suffixes: A collection of all suffixes to consider.
 
         Returns:
             A set of file suffix strings.
         """
         results = self._tool.buildbot.fetch_results(build)
         if not results:
             _log.debug('No results found for build %s', build)
             return set()
         test_result = results.result_for_test(test)
         if not test_result:
             _log.debug('No test result for test %s in build %s', test, build)
             return set()
-        return set(existing_suffixes) & TestExpectations.suffixes_for_test_result(test_result)
+        return TestExpectations.suffixes_for_test_result(test_result)
 
 
 class RebaselineJson(AbstractParallelRebaselineCommand):
     name = 'rebaseline-json'
     help_text = 'Rebaseline based off JSON passed to stdin. Intended to only be called from other scripts.'
 
     def __init__(self,):
         super(RebaselineJson, self).__init__(options=[
             self.no_optimize_option,
             self.results_directory_option,
@@ skipping 10 matching lines @@
     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
 
     @staticmethod
     def _tests_to_rebaseline(port):
-        tests_to_rebaseline = {}
+        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():
-                suffixes = TestExpectations.suffixes_for_expectations(expectations.get_expectations(test))
-                tests_to_rebaseline[test] = suffixes or BASELINE_SUFFIX_LIST
+                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)).items()
+        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, suffixes in tests:
-            _log.info('    %s (%s)', test_name, ','.join(suffixes))
+        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][Build(builder_name)] = suffixes
+                self._test_prefix_list[test_name] = []
+            self._test_prefix_list[test_name].append(Build(builder_name))
 
     def execute(self, options, args, tool):
         self._tool = 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:
             _log.warning('Did not find any tests marked Rebaseline.')
             return
 
         self.rebaseline(options, self._test_prefix_list)
 
 
 class Rebaseline(AbstractParallelRebaselineCommand):
     name = 'rebaseline'
     help_text = 'Rebaseline tests with results from the build bots.'
     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.suffixes_option,
             self.results_directory_option,
             optparse.make_option('--builders', default=None, action='append',
                                  help=('Comma-separated-list of builders to pull new baselines from '
                                        '(can also be provided multiple times).')),
         ])
 
     def _builders_to_pull_from(self):
         return self._tool.user.prompt_with_list(
             'Which builder to pull results from:', self._release_builders(), can_choose_multiple=True)
 
     def execute(self, options, args, tool):
         self._tool = tool
         if not args:
             _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 = {}
-        suffixes_to_update = options.suffixes.split(',')
 
         for builder in builders_to_check:
             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
+                    test_prefix_list[test] = []
+                test_prefix_list[test].append(Build(builder))
 
         if options.verbose:
             _log.debug('rebaseline-json: ' + str(test_prefix_list))
 
         self.rebaseline(options, test_prefix_list)