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

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 de915219f7561c3c7f5c4a48abeb355af75f5704..24d9c4dda835c379e49a243a2f590152540d2d57 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline.py
@@ -287,16 +287,16 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
         super(AbstractParallelRebaselineCommand, self).__init__(options=options)
 
     @memoized
-    def builder_data(self):

[wkorman, 2016/07/14 01:01:45]

Helpful if we document what this returns key/value wise.

In general at the point where we're passing around a dict whose key is a tuple,
we could consider using a separate object to store things so that we can just
use a method on that object that takes the two params, and the 'None' one can
just be the default value.

[qyearsley, 2016/07/14 23:53:29]

Done -- now added a docstring here, and now instead of a pair, this code uses
"Build" objects, which have two attributes, with a default value for
build_number. How does that seem?

-        builder_to_results = {}
+    def build_data(self):
+        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[(builder_name, None)] = 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):
@@ -320,15 +320,15 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
             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()
@@ -356,14 +356,18 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
 
         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 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
+                    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.
@@ -374,6 +378,8 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
 
                     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:
@@ -418,8 +424,12 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
         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 for b, _ in test_prefix_list[test]])
+            for build in test_prefix_list[test]:
+                builder, _ = build
+                if builder 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:
@@ -502,25 +512,25 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
 
         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 (builder name, build number)
+                pairs to baseline suffixes to rebaseline. For example:
                 {
-                    "some/test.html": {"builder-1": ["txt"], "builder-2": ["txt"]},
-                    "some/other.html": {"builder-1": ["txt"]}
+                    "some/test.html": {("builder-1", 412): ["txt"], ("builder-2", 100): ["txt"]},
+                    "some/other.html": {("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)
@@ -544,10 +554,20 @@ class AbstractParallelRebaselineCommand(AbstractRebaseliningCommand):
             # 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).
+            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)
@@ -600,7 +620,7 @@ class RebaselineExpectations(AbstractParallelRebaselineCommand):
             _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][(builder_name, None)] = suffixes
 
     def execute(self, options, args, tool):
         options.results_directory = None
@@ -658,11 +678,12 @@ class Rebaseline(AbstractParallelRebaselineCommand):
             for test in args:
                 if test not in test_prefix_list:
                     test_prefix_list[test] = {}
-                test_prefix_list[test][builder.name()] = suffixes_to_update
+                test_prefix_list[test][(builder.name(), None)] = suffixes_to_update
 
         if options.verbose:
             _log.debug("rebaseline-json: " + str(test_prefix_list))
 
+        print(test_prefix_list)

[wkorman, 2016/07/14 01:01:46]

Debugging

[qyearsley, 2016/07/14 23:53:29]

Now removed

         self._rebaseline(options, test_prefix_list)
 
 
@@ -705,7 +726,7 @@ class AutoRebaseline(AbstractParallelRebaselineCommand):
 
     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 []
@@ -805,7 +826,7 @@ class AutoRebaseline(AbstractParallelRebaselineCommand):
                     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][(builder_name, None)] = BASELINE_SUFFIX_LIST
 
         return test_prefix_list, lines_to_remove