In rebaseline, include build number information with "test_prefix_list" dicts.

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 31 matching lines @@
 from webkitpy.common.system.executive import ScriptError
 from webkitpy.layout_tests.controllers.test_result_writer import baseline_name
 from webkitpy.layout_tests.models.test_expectations import TestExpectations, BASELINE_SUFFIX_LIST, SKIP
 from webkitpy.layout_tests.port import factory
 from webkitpy.tool.commands.command import Command
 
 
 _log = logging.getLogger(__name__)
 
 
+class Build(object):
+    """Represents a combination of builder and build number.
+
+    If build number is None, this represents the latest build
+    for a given builder.
+
+    TODO(qyearsley): Move this somewhere else; note it's very similar to TryJob,

[wkorman, 2016/07/15 00:12:14]

There is also already a Build class:

https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

Maybe that's fine, I considered alternate names and nothing great came to mind.

[qyearsley, 2016/07/15 16:28:47]

Yesterday after looking at the buildbot module, I was thinking about how it 
could be refactored, and the result was http://crrev.com/2152663003.

One possible way to refactor this would be:
  - Move this Build class to buildbot.
  - In Rietveld, use buildbot.Build instead of TryJob.

I'd like to do this after deciding whether to commit
http://crrev.com/2152663003.

+    and it seems like it might belong in the buildbot module.
+    """
+    def __init__(self, builder_name, build_number=None):
+        self.builder_name = builder_name
+        self.build_number = build_number
+
+    # Having __hash__ and __eq__ allow instances of this class to be used
+    # as keys in dictionaries.
+    def __hash__(self):
+        return hash((self.builder_name, self.build_number))
+
+    def __eq__(self, other):
+        return (self.builder_name, self.build_number) == (other.builder_name, other.build_number)
+
+
 class AbstractRebaseliningCommand(Command):
     """Base class for rebaseline-related commands."""
     # Not overriding execute() - pylint: disable=abstract-method
 
     no_optimize_option = optparse.make_option('--no-optimize', dest='optimize', action='store_false', default=True,
                                               help=('Do not optimize/de-dup the expectations after rebaselining (default is to de-dup automatically). '
                                                     'You can use "webkit-patch optimize-baselines" to optimize separately.'))
 
     platform_options = factory.platform_options(use_globs=True)
 
@@ skipping 218 matching lines @@
 
 
 class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
     """Base class for rebaseline commands that do some tasks in parallel."""
     # Not overriding execute() - pylint: disable=abstract-method
 
     def __init__(self, options=None):
         super(AbstractParallelRebaselineCommand, self).__init__(options=options)
 
     @memoized
-    def builder_data(self):
-        builder_to_results = {}
+    def build_data(self):
+        """Returns a map of Build objects to LayoutTestResult objects.
+
+        The Build objects are the latest builds for the release builders,
+        and LayoutTestResult objects for results fetched from archived layout
+        test results.
+        """
+        build_to_results = {}
         for builder_name in self._release_builders():
             builder = self._tool.buildbot.builder_with_name(builder_name)
             builder_results = builder.latest_layout_test_results()
             if builder_results:
-                builder_to_results[builder_name] = builder_results
+                build_to_results[Build(builder_name)] = builder_results
             else:
                 raise Exception("No result for builder %s." % builder_name)
-        return builder_to_results
+        return build_to_results
 
     # The release builders cycle much faster than the debug ones and cover all the platforms.
     def _release_builders(self):
         release_builders = []
         for builder_name in self._tool.builders.all_continuous_builder_names():
             if 'ASAN' in builder_name:
                 continue
             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.scm().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.
+        """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.
+            builders_to_check: List of builder names.
         """
         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)
         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()
 
     def _rebaseline_commands(self, test_prefix_list, options, skip_checking_actual_results=False):
         path_to_webkit_patch = self._tool.path()
         cwd = self._tool.scm().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]):
-                for builder in self._builders_to_fetch_from(test_prefix_list[test_prefix]):
+                builders_to_fetch_from = self._builders_to_fetch_from([b.builder_name for b in test_prefix_list[test_prefix]])
+                for build in test_prefix_list[test_prefix]:
                     # TODO(qyearsley): Remove the parameter skip_checking_actual_results
                     # and instead refactor the existing code so that this is not necessary.
+                    builder, build_number = build.builder_name, build.build_number
+
+                    if builder not in builders_to_fetch_from:
+                        break
                     if skip_checking_actual_results:
-                        actual_failures_suffixes = test_prefix_list[test_prefix][builder]
+                        actual_failures_suffixes = test_prefix_list[test_prefix][build]
                     else:
                         actual_failures_suffixes = self._suffixes_for_actual_failures(
-                            test, builder, test_prefix_list[test_prefix][builder])
+                            test, build, test_prefix_list[test_prefix][build])
                     if not actual_failures_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)
                     cmd_line = ['--suffixes', suffixes, '--builder', builder, '--test', test]
+                    if build_number:
+                        cmd_line.extend(['--build-number', build_number])
                     if options.results_directory:
                         cmd_line.extend(['--results-directory', options.results_directory])
                     if options.verbose:
                         cmd_line.append('--verbose')
                     copy_baseline_commands.append(
                         tuple([[self._tool.executable, path_to_webkit_patch, 'copy-existing-baselines-internal'] + cmd_line, cwd]))
                     rebaseline_commands.append(
                         tuple([[self._tool.executable, path_to_webkit_patch, 'rebaseline-test-internal'] + cmd_line, cwd]))
         return copy_baseline_commands, rebaseline_commands, lines_to_remove
 
@@ skipping 24 matching lines @@
 
             if not file_added:
                 _log.debug('Could not add file based off output "%s"' % output)
 
         return list(files_to_add), list(files_to_delete), lines_to_remove
 
     def _optimize_baselines(self, test_prefix_list, verbose=False):
         optimize_commands = []
         for test in test_prefix_list:
             all_suffixes = set()
-            for builder in self._builders_to_fetch_from(test_prefix_list[test]):
-                all_suffixes.update(self._suffixes_for_actual_failures(test, builder, test_prefix_list[test][builder]))
+            builders_to_fetch_from = self._builders_to_fetch_from([b.builder_name for b in test_prefix_list[test]])

[wkorman, 2016/07/15 00:12:14]

Could be worth a helper method for the list comprehension used here and
elsewhere:

[b.builder_name for b in test_prefix_list[test]]

[qyearsley, 2016/07/15 16:28:47]

Extracted a little helper method -- but now, after seeing that there are these
"test_prefix_list" dicts used all over this module, which are a little unwieldy,
it seems like it might be nicer to add another class to represent "a set of
baselines to download across different builders", which is what test_prefix_list
represents now.

If that were done, then this could be perhaps be replaced by a method on that
class.

Added a TODO note for a follow-up change.

+            for build in 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]))
 
             # 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 = ['--no-modify-scm', '--suffixes', ','.join(all_suffixes), test]
             if verbose:
                 cmd_line.append('--verbose')
 
@@ skipping 62 matching lines @@
         if files_to_add:
             self._tool.scm().add_list(files_to_add)
         return lines_to_remove
 
     def _rebaseline(self, options, test_prefix_list, skip_checking_actual_results=False):
         """Downloads new baselines in parallel, then updates expectations files
         and optimizes baselines.
 
         Args:
             options: An object with the options passed to the current command.
-            test_prefix_list: A map of test names to builder names to baseline
-                suffixes to rebaseline. For example:
+            test_prefix_list: A map of test names to Build objects to file suffixes
+                for new baseilnes. For example:
                 {
-                    "some/test.html": {"builder-1": ["txt"], "builder-2": ["txt"]},
-                    "some/other.html": {"builder-1": ["txt"]}
+                    "some/test.html": {Build("builder-1", 412): ["txt"], Build("builder-2", 100): ["txt"]},
+                    "some/other.html": {Build("builder-1", 412): ["txt"]}
                 }
                 This would mean that new text baselines should be downloaded for
-                "some/test.html" on both builder-1 and builder-2, and new text
-                baselines should be downloaded for "some/other.html" but only
-                from builder-1.
+                "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.
             skip_checking_actual_results: If True, then the lists of suffixes
                 to rebaseline from |test_prefix_list| will be used directly;
                 if False, then the list of suffixes will filtered to include
                 suffixes with mismatches in actual results.
         """
-        for test, builders_to_check in sorted(test_prefix_list.items()):
+        for test, builds_to_check in sorted(test_prefix_list.items()):
             _log.info("Rebaselining %s" % test)
-            for builder, suffixes in sorted(builders_to_check.items()):
-                _log.debug("  %s: %s" % (builder, ",".join(suffixes)))
+            for build, suffixes in sorted(builds_to_check.items()):
+                _log.debug("  %s: %s" % (build, ",".join(suffixes)))
 
         copy_baseline_commands, rebaseline_commands, extra_lines_to_remove = self._rebaseline_commands(
             test_prefix_list, options, skip_checking_actual_results)
         lines_to_remove = {}
 
         self._run_in_parallel_and_update_scm(copy_baseline_commands)
         lines_to_remove = self._run_in_parallel_and_update_scm(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_and_update_scm(self._optimize_baselines(test_prefix_list, options.verbose))
 
-    def _suffixes_for_actual_failures(self, test, builder_name, existing_suffixes):
-        if builder_name not in self.builder_data():
+    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 pair (builder_name, build_number).

[wkorman, 2016/07/15 00:12:14]

Is the description here up to date re: Build class?

[qyearsley, 2016/07/15 16:28:47]

It was not; now updated.

+            existing_suffixes: A collection of all suffixes to consider.
+
+        Returns:
+            A set of file suffix strings.
+        """
+        if build not in self.build_data():
             return set()
-        test_result = self.builder_data()[builder_name].result_for_test(test)
+        test_result = self.build_data()[build].result_for_test(test)
         if not test_result:
             return set()
         return set(existing_suffixes) & 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,):
@@ skipping 32 matching lines @@
             return
         tests = self._tests_to_rebaseline(self._tool.port_factory.get(port_name)).items()
 
         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)))
             if test_name not in self._test_prefix_list:
                 self._test_prefix_list[test_name] = {}
-            self._test_prefix_list[test_name][builder_name] = suffixes
+            self._test_prefix_list[test_name][Build(builder_name)] = suffixes
 
     def execute(self, options, args, 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_for_port(port_name)
         if not self._test_prefix_list:
             _log.warning("Did not find any tests marked Rebaseline.")
             return
@@ skipping 37 matching lines @@
         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] = {}
-                test_prefix_list[test][builder.name()] = suffixes_to_update
+                build = Build(builder.name())
+                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)
 
 
 class AutoRebaseline(AbstractParallelRebaselineCommand):
     name = "auto-rebaseline"
     help_text = "Rebaselines any NeedsRebaseline lines in TestExpectations that have cycled through all the bots."
@@ skipping 26 matching lines @@
                     @
                     [^@>]+  # Domain terminated by @ or >, some lines have an additional @ fragment after the email.
                 )
                 .*?([^ ]*)  # Test file name
                 \ \[        # Single space followed by opening [ for expectation specifier
                 [^[]*$      # Prevents matching previous [ for version specifiers instead of expectation specifiers
             """, re.VERBOSE)
 
     def bot_revision_data(self, scm):
         revisions = []
-        for result in self.builder_data().values():
+        for result in self.build_data().values():
             if result.run_was_interrupted():
                 _log.error("Can't rebaseline because the latest run on %s exited early." % result.builder_name())
                 return []
             revisions.append({
                 "builder": result.builder_name(),
                 "revision": result.chromium_revision(scm),
             })
         return revisions
 
     def _strip_comments(self, line):
@@ skipping 79 matching lines @@
             port = self._tool.port_factory.get(port_name)
             expectations = TestExpectations(port, include_overrides=True)
             for test in expectations.get_needs_rebaseline_failures():
                 if test not in tests:
                     continue
 
                 if test not in test_prefix_list:
                     lines_to_remove[test] = []
                     test_prefix_list[test] = {}
                 lines_to_remove[test].append(builder_name)
-                test_prefix_list[test][builder_name] = BASELINE_SUFFIX_LIST
+                test_prefix_list[test][Build(builder_name)] = BASELINE_SUFFIX_LIST
 
         return test_prefix_list, lines_to_remove
 
     def _run_git_cl_command(self, options, command):
         subprocess_command = ['git', 'cl'] + command
         if options.verbose:
             subprocess_command.append('--verbose')
         if options.auth_refresh_token_json:
             subprocess_command.append('--auth-refresh-token-json')
             subprocess_command.append(options.auth_refresh_token_json)
@@ skipping 106 matching lines @@
                     issue_already_closed = tool.executive.run_command(
                         ['git', 'config', 'branch.%s.rietveldissue' % rebaseline_branch_name],
                         return_exit_code=True)
                     if not issue_already_closed:
                         self._run_git_cl_command(options, ['set_close'])
 
                 tool.scm().ensure_cleanly_tracking_remote_master()
                 if old_branch_name_or_ref:
                     tool.scm().checkout_branch(old_branch_name_or_ref)
                 tool.scm().delete_branch(rebaseline_branch_name)