In webkit-patch rebaseline-cl, abort if results are missing.

third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py

Index: third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
diff --git a/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
index 1fd61c3519183b1518c43a500f9224872603fe47..c50789690062dbdb787b1933db8b4c2600389eb7 100644
--- a/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
+++ b/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
@@ -57,6 +57,7 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
             return
 
         builds = self.rietveld.latest_try_jobs(issue_number, self._try_bots())
+
         if options.trigger_jobs:
             if self.trigger_jobs_for_missing_builds(builds):
                 _log.info('Please re-run webkit-patch rebaseline-cl once all pending try jobs have finished.')
@@ -64,13 +65,22 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
         if not builds:
             _log.info('No builds to download baselines from.')
 
+        try:
+            _log.debug('Getting results for Rietveld issue %d.', issue_number)

[wkorman, 2016/12/05 05:58:35]

Q for whomever may know: when are we moving to Gerrit, and are there
ramifications for this and other tools?

[qyearsley, 2016/12/05 17:55:21]

There are ramifications for this tool, but not so much for others. Currently,
this tool makes direct requests to Rietveld to:
 - List of try jobs for the latest patch for a CL.
 - List changed files for a CL.

The reason why requests are made to Rietveld is so that arbitrary CLs including
other people's CLs can be supported -- but, if rebaseline-cl was changed only
support CLs associated with the current local branch, then this information can
also be gotten via `git diff` and `git cl try-results`.

For other things, including status of try jobs and starting try jobs,
rebaseline-cl uses `git cl try` and `git cl try-results`.

[wkorman, 2016/12/05 18:56:25]

Sounds reasonable, perhaps worth checking in with agable@ who I think may be
involved in the gerrit move to make sure this is considered as part of the
migration. And/or open a bug to track your note re: arbitrary CLs. Is there a
way to find out how many CLs would be impacted if we shifted to only use local
branches? We could do a small survey.

[qyearsley, 2016/12/07 00:42:27]

Aye - just filed a bug for this: http://crbug.com/671684.

+            builds_to_results = self._fetch_results(builds)
+        except _MissingResults as error:
+            _log.error(error.message)
+            return
+
+        test_prefix_list = {}
         if args:
-            test_prefix_list = {}
             for test in args:
                 test_prefix_list[test] = {b: BASELINE_SUFFIX_LIST for b in builds}
         else:
             test_prefix_list = self._test_prefix_list(
-                issue_number, only_changed_tests=options.only_changed_tests)
+                issue_number,
+                builds_to_results,
+                only_changed_tests=options.only_changed_tests)
 
         self._log_test_prefix_list(test_prefix_list)
 
@@ -124,11 +134,42 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
 
         return bool(builders_with_pending_builds or builders_without_builds)
 
-    def _test_prefix_list(self, issue_number, only_changed_tests):
-        """Returns a collection of test, builder and file extensions to get new baselines for.
+    def _try_bots(self):
+        """Returns a collection of try bot builders to fetch results for."""
+        return self._tool.builders.all_try_builder_names()
+
+    def _fetch_results(self, builds):
+        """Fetch results for each build.
+
+        There should be a one-to-one correspondence between Builds, supported
+        platforms, and try bots. If not all of the builds can be fetched, then
+        continuing with rebaselining may yield incorrect results, when the new
+        baselines are deduped, an old baseline may be kept for the platform
+        that's missing results.
+
+        Returns:
+            A dict mapping Build to LayoutTestResults.
+        """
+        buildbot = self._tool.buildbot
+        results = {}
+        for build in builds:
+            results_url = buildbot.results_url(build.builder_name, build.build_number)
+            layout_test_results = buildbot.fetch_results(build)
+            if layout_test_results is None:
+                raise _MissingResults(

[wkorman, 2016/12/05 05:58:35]

Worth documenting exceptions a method may raise in the method doc comment?

[qyearsley, 2016/12/05 17:55:21]

Added a "Raises:" part to the docstring.

+                    'Failed to fetch results from "%s".\n'
+                    'Try starting a new job for %s by running :\n'
+                    '  git cl try -b %s' %
+                    (results_url, build.builder_name, build.builder_name))
+            results[build] = layout_test_results
+        return results
+
+    def _test_prefix_list(self, issue_number, builds_to_results, only_changed_tests):
+        """Returns a collection of tests, builders and file extensions to get new baselines for.
 
         Args:
             issue_number: The CL number of the change which needs new baselines.
+            builds_to_results: A dict mapping Builds to LayoutTestResults.
             only_changed_tests: Whether to only include baselines for tests that
                are changed in this CL. If False, all new baselines for failing
                tests will be downloaded, even for tests that were not modified.
@@ -136,7 +177,9 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
         Returns:
             A dict containing information about which new baselines to download.
         """
-        builds_to_tests = self._builds_to_tests(issue_number)
+        builds_to_tests = {}
+        for build, results in builds_to_results.iteritems():
+            builds_to_tests[build] = self._tests_to_rebaseline(build, results)
         if only_changed_tests:
             files_in_cl = self.rietveld.changed_files(issue_number)
             # Note, in the changed files list from Rietveld, paths always
@@ -154,28 +197,8 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
                 result[test][build] = BASELINE_SUFFIX_LIST
         return result
 
-    def _builds_to_tests(self, issue_number):
-        """Fetches a list of try bots, and for each, fetches tests with new baselines."""
-        _log.debug('Getting results for Rietveld issue %d.', issue_number)
-        builds = self.rietveld.latest_try_jobs(issue_number, self._try_bots())
-        if not builds:
-            _log.debug('No try job results for builders in: %r.', self._try_bots())
-        return {build: self._tests_to_rebaseline(build) for build in builds}
-
-    def _try_bots(self):
-        """Returns a collection of try bot builders to fetch results for."""
-        return self._tool.builders.all_try_builder_names()
-
-    def _tests_to_rebaseline(self, build):
-        """Fetches a list of tests that should be rebaselined."""
-        buildbot = self._tool.buildbot
-        results_url = buildbot.results_url(build.builder_name, build.build_number)
-
-        layout_test_results = buildbot.fetch_layout_test_results(results_url)
-        if layout_test_results is None:
-            _log.warning('Failed to request layout test results from "%s".', results_url)
-            return []
-
+    def _tests_to_rebaseline(self, build, layout_test_results):
+        """Fetches a list of tests that should be rebaselined for some build ."""
         unexpected_results = layout_test_results.didnt_run_as_expected_results()
         tests = sorted(r.test_name() for r in unexpected_results
                        if r.is_missing_baseline() or r.has_mismatch_result())
@@ -197,7 +220,7 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
             retry_summary = json.loads(content)
             return retry_summary['failures']
         except (ValueError, KeyError):
-            _log.warning('Unexepected retry summary content:\n%s', content)
+            _log.warning('Unexpected retry summary content:\n%s', content)
             return None
 
     @staticmethod
@@ -211,3 +234,7 @@ class RebaselineCL(AbstractParallelRebaselineCommand):
             _log.debug('  %s:', test)
             for build in sorted(builds):
                 _log.debug('    %s', build)
+
+
+class _MissingResults(Exception):

[wkorman, 2016/12/05 05:58:35]

Probably worth a doc comment or something in name to make it clearer what the
missing results are, since results is very general.

[qyearsley, 2016/12/05 17:55:21]

Changed name to _MissingLayoutTestResults.

My rationale for using an exception was originally so that I could raise it in
_tests_to_rebaseline and catch it in execute() and not have to worry about
passing a special value up the call stack to indicate that the whole process
should be aborted.

But now, it's raised directly in _fetch_results and it would probably be just as
simple now to print an error and return None from _fetch_results instead of
adding this exception here. Does that sound better?

[wkorman, 2016/12/05 18:56:25]

Yes, that sounds preferable.

[qyearsley, 2016/12/07 00:42:27]

Now done :-)

+    pass