Change rebaseline-test-internal to take port name option.

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 46 matching lines @@
               '(default is to de-dupe automatically). You can use "webkit-patch '
               'optimize-baselines" to optimize separately.'))
     platform_options = factory.platform_options(use_globs=True)
     results_directory_option = optparse.make_option(
         '--results-directory', help='Local results directory to use.')
     suffixes_option = optparse.make_option(
         '--suffixes', default=','.join(BASELINE_SUFFIX_LIST), action='store',
         help='Comma-separated-list of file types to rebaseline.')
     builder_option = optparse.make_option(
         '--builder', help='Builder to pull new baselines from.')
+    port_name_option = optparse.make_option(
+        '--port-name',
+        help=('Fully-qualified name of the port that new baselines belong to. '
+              'If not given, this is determined based on --builder.'))

[wkorman, 2017/03/24 18:56:05]

Maybe include an example value for people unfamiliar with builder names, port
names, etc.

[qyearsley, 2017/03/24 21:10:59]

Sounds good. In this case, --port-name is only used for "internal" commands that
are not intended to be directly invoked, but still things are generally clearer
with examples. Updated in latest patchset.

     test_option = optparse.make_option('--test', help='Test to rebaseline.')
     build_number_option = optparse.make_option(
         '--build-number', default=None, type='int',
         help='Optional build number; if not given, the latest build is used.')
 
     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
@@ skipping 18 matching lines @@
 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 {}
 
-    def remove_line(self, test, builder):
+    def remove_line(self, test, port_name):
         if test not in self.lines_to_remove:
             self.lines_to_remove[test] = []
-        self.lines_to_remove[test].append(builder)
+        self.lines_to_remove[test].append(port_name)
 
     def to_dict(self):
         remove_lines = []
         for test in self.lines_to_remove:
-            for builder in self.lines_to_remove[test]:
-                remove_lines.append({'test': test, 'builder': builder})
+            for port_name in self.lines_to_remove[test]:
+                remove_lines.append({'test': test, 'port_name': port_name})
         return {'remove-lines': remove_lines}
 
     @staticmethod
     def from_dict(change_dict):
         lines_to_remove = {}
         if 'remove-lines' in change_dict:
             for line_to_remove in change_dict['remove-lines']:
                 test = line_to_remove['test']
-                builder = line_to_remove['builder']
+                port_name = line_to_remove['port_name']
                 if test not in lines_to_remove:
                     lines_to_remove[test] = []
-                lines_to_remove[test].append(builder)
+                lines_to_remove[test].append(port_name)
         return ChangeSet(lines_to_remove=lines_to_remove)
 
     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])
 
@@ skipping 44 matching lines @@
         return self._builder_names
 
 
 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.test_option,
+            self.suffixes_option,
+            self.port_name_option,
             self.results_directory_option,
-            self.suffixes_option,
-            self.builder_option,
-            self.test_option,
         ])
 
     @memoized
     def _immediate_predecessors_in_fallback(self, path_to_rebaseline):
         """Returns the predecessor directories in the baseline fall-back graph.
 
-        The platform-specific fall-back baseline directories form a tree; the
-        "immediate predecessors" are the children nodes For example, if the
-        baseline fall-back graph includes:
+        The platform-specific fall-back baseline directories form a tree, where
+        when we search for baselines we normally fall back to parents nodes in
+        the tree. The "immediate predecessors" are the children nodes of the
+        given node.
+
+        For example, if the baseline fall-back graph includes:
             "mac10.9" -> "mac10.10/"
             "mac10.10/" -> "mac/"
             "retina/" -> "mac/"
         Then, the "immediate predecessors" are:
             "mac/": ["mac10.10/", "retina/"]
             "mac10.10/": ["mac10.9/"]
             "mac10.9/", "retina/": []
+
+        Args:
+            path_to_rebaseline: The absolute path to a baseline directory.
+
+        Returns:
+            A list of directory names (not full paths) of directories that are
+            "immediate predecessors" of the given baseline path.
         """
         port_names = self._tool.port_factory.all_port_names()
         immediate_predecessors = []
         for port_name in port_names:
             port = self._tool.port_factory.get(port_name)
             baseline_search_path = port.baseline_search_path()
             try:
                 index = baseline_search_path.index(path_to_rebaseline)
                 if index:
                     immediate_predecessors.append(self._tool.filesystem.basename(baseline_search_path[index - 1]))
             except ValueError:
                 # baseline_search_path.index() throws a ValueError if the item isn't in the list.
                 pass
         return immediate_predecessors
 
     def _port_for_primary_baseline(self, baseline):
         """Returns a Port object for the given baseline directory base name."""
         for port in [self._tool.port_factory.get(port_name) for port_name in self._tool.port_factory.all_port_names()]:
             if self._tool.filesystem.basename(port.baseline_version_dir()) == baseline:
                 return port
         raise Exception('Failed to find port for primary baseline %s.' % baseline)
 
-    def _copy_existing_baseline(self, builder_name, test_name, suffix):
+    def _copy_existing_baseline(self, port_name, test_name, suffix):
         """Copies the baseline for the given builder to all "predecessor" directories."""
-        baseline_directory = self._baseline_directory(builder_name)
+        baseline_directory = self._tool.port_factory.get(port_name).baseline_version_dir()
         ports = [self._port_for_primary_baseline(baseline)
                  for baseline in self._immediate_predecessors_in_fallback(baseline_directory)]
 
         old_baselines = []
         new_baselines = []
 
         # Need to gather all the baseline paths before modifying the filesystem since
         # the modifications can affect the results of port.expected_filename.
         for port in ports:
             old_baseline = port.expected_filename(test_name, '.' + suffix)
@@ skipping 24 matching lines @@
         for i in range(len(old_baselines)):
             old_baseline = old_baselines[i]
             new_baseline = new_baselines[i]
 
             _log.debug('Copying baseline from %s to %s.', old_baseline, new_baseline)
             self._tool.filesystem.maybe_make_directory(self._tool.filesystem.dirname(new_baseline))
             self._tool.filesystem.copyfile(old_baseline, new_baseline)
 
     def execute(self, options, args, tool):
         self._tool = tool
+        port_name = options.port_name
         for suffix in options.suffixes.split(','):
-            self._copy_existing_baseline(options.builder, options.test, suffix)
+            self._copy_existing_baseline(port_name, options.test, suffix)
 
 
 class RebaselineTest(AbstractRebaseliningCommand):
     name = 'rebaseline-test-internal'
     help_text = 'Rebaseline a single test from a buildbot. Only intended for use by other webkit-patch commands.'
 
     def __init__(self):
         super(RebaselineTest, self).__init__(options=[
+            self.test_option,
+            self.suffixes_option,
+            self.port_name_option,
+            self.builder_option,
+            self.build_number_option,
             self.results_directory_option,
-            self.suffixes_option,
-            self.builder_option,
-            self.test_option,
-            self.build_number_option,
         ])
 
     def _save_baseline(self, data, target_baseline):
         if not data:
             _log.debug('No baseline data to save.')
             return
 
         filesystem = self._tool.filesystem
         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)
+    def _rebaseline_test(self, port_name, test_name, suffix, results_url):
+        """Downloads a baseline file and saves it to the filesystem.
+
+        Args:
+            port_name: The port that the baseline is for. This determines
+                the directory that the baseline is saved to.
+            test_name: The name of the test being rebaselined.
+            suffix: The baseline file extension (e.g. png); together with the
+                test name and results_url this determines what file to download.
+            results_url: Base URL to download the actual result from.
+        """
+        baseline_directory = self._tool.port_factory.get(port_name).baseline_version_dir()
 
         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)
         self._save_baseline(self._tool.web.get_binary(source_baseline, return_none_on_404=True),
                             target_baseline)
 
     def _rebaseline_test_and_update_expectations(self, options):
         self._baseline_suffix_list = options.suffixes.split(',')
 
-        port = self._tool.port_factory.get_from_builder_name(options.builder)
+        port_name = options.port_name or self._tool.builders.port_name_for_builder_name(options.builder)
+        port = self._tool.port_factory.get(port_name)
+
         if port.reference_files(options.test):
             if 'png' in self._baseline_suffix_list:
                 _log.warning('Cannot rebaseline image result for reftest: %s', options.test)
                 return
             assert self._baseline_suffix_list == ['txt']
 
         if options.results_directory:
             results_url = 'file://' + options.results_directory
         else:
             results_url = self._tool.buildbot.results_url(options.builder, build_number=options.build_number)
 
         for suffix in self._baseline_suffix_list:
-            self._rebaseline_test(options.builder, options.test, suffix, results_url)
-        self.expectation_line_changes.remove_line(test=options.test, builder=options.builder)
+            self._rebaseline_test(port_name, options.test, suffix, results_url)
+        self.expectation_line_changes.remove_line(test=options.test, port_name=port_name)
 
     def execute(self, options, args, tool):
         self._tool = tool
         self._rebaseline_test_and_update_expectations(options)
         self._print_expectation_line_changes()
 
 
 class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
     """Base class for rebaseline commands that do some tasks in parallel."""
     # Not overriding execute() - pylint: disable=abstract-method
@@ skipping 63 matching lines @@
         cwd = self._tool.git().checkout_root
         copy_baseline_commands = []
         rebaseline_commands = []
         lines_to_remove = {}
 
         builders_to_fetch_from = self._builders_to_fetch_from(test_baseline_set.all_builders())
         for test, build in test_baseline_set:
             if build.builder_name not in builders_to_fetch_from:
                 continue
 
+            port_name = self._tool.builders.port_name_for_builder_name(build.builder_name)
+
             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)
+                lines_to_remove[test].append(port_name)
                 continue
 
-            args = ['--suffixes', ','.join(suffixes), '--builder', build.builder_name, '--test', test]
+            args = []
             if options.verbose:
                 args.append('--verbose')
+            args.extend([
+                '--test', test,
+                '--suffixes', ','.join(suffixes),
+                '--port-name', port_name,
+            ])
 
             copy_command = [self._tool.executable, path_to_webkit_patch, 'copy-existing-baselines-internal'] + args
             copy_baseline_commands.append(tuple([copy_command, cwd]))
 
+            args.extend(['--builder', build.builder_name])
             if build.build_number:
                 args.extend(['--build-number', str(build.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]))
 
         return copy_baseline_commands, rebaseline_commands, lines_to_remove
 
@@ skipping 23 matching lines @@
             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, suffixes in tests_to_suffixes.iteritems():
             # No need to optimize baselines for a test with no failures.
             if not suffixes:
                 continue
             # FIXME: We should propagate the platform options as well.
-            args = ['--suffixes', ','.join(suffixes), test]
+            args = []
             if verbose:
                 args.append('--verbose')
+            args.extend(['--suffixes', ','.join(suffixes), test])
             path_to_webkit_patch = self._tool.path()
             cwd = self._tool.git().checkout_root
             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..
         # This is only necessary for "webkit-patch rebaseline" and for rebaselining expected
         # failures from garden-o-matic. rebaseline-expectations and auto-rebaseline will always
         # pass the exact set of ports to rebaseline.
         for port_name in self._tool.port_factory.all_port_names():
             port = self._tool.port_factory.get(port_name)
             generic_expectations = TestExpectations(port, tests=tests, include_overrides=False)
             full_expectations = TestExpectations(port, tests=tests, include_overrides=True)
             for test in tests:
                 if port.skips_test(test, generic_expectations, full_expectations):
                     for test_configuration in port.all_test_configurations():
                         if test_configuration.version == port.test_configuration().version:
                             to_remove.append((test, test_configuration))
 
         for test in lines_to_remove:
-            for builder in lines_to_remove[test]:
-                port = self._tool.port_factory.get_from_builder_name(builder)
+            for port_name in lines_to_remove[test]:
+                port = self._tool.port_factory.get(port_name)
                 for test_configuration in port.all_test_configurations():
                     if test_configuration.version == port.test_configuration().version:
                         to_remove.append((test, test_configuration))
 
         port = self._tool.port_factory.get()
         expectations = TestExpectations(port, include_overrides=False)
         expectations_string = expectations.remove_configurations(to_remove)
         path = port.path_to_generic_test_expectations_file()
         self._tool.filesystem.write_text_file(path, expectations_string)
 
@@ skipping 198 matching lines @@
 
         test_baseline_set = TestBaselineSet(tool)
 
         for builder in builders_to_check:
             for test_prefix in args:
                 test_baseline_set.add(test_prefix, Build(builder))
 
         _log.debug('Rebaselining: %s', test_baseline_set)
 
         self.rebaseline(options, test_baseline_set)