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

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

 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """A command to fetch new baselines from try jobs for a Rietveld issue.
 
 This command interacts with the Rietveld API to get information about try jobs
 with layout test results.
 """
 
@@ skipping 39 matching lines @@
         ])
         self.rietveld = Rietveld(Web())
 
     def execute(self, options, args, tool):
         self._tool = tool
         issue_number = self._get_issue_number(options)
         if not issue_number:
             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.')
                 return
         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)
 
         if options.dry_run:
             return
         self.rebaseline(options, test_prefix_list)
 
     def _get_issue_number(self, options):
         """Gets the Rietveld CL number from either |options| or from the current local branch."""
         if options.issue:
@@ skipping 33 matching lines @@
         if builders_without_builds:
             _log.info('Triggering try jobs for:')
             command = ['try']
             for builder in sorted(builders_without_builds):
                 _log.info('  %s', builder)
                 command.extend(['-b', builder])
             self.git_cl().run(command)
 
         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.
 
         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
             # use / as the separator, and they're always relative to repo root.
             # TODO(qyearsley): Do this without using a hard-coded constant.
             test_base = 'third_party/WebKit/LayoutTests/'
             tests_in_cl = [f[len(test_base):] for f in files_in_cl if f.startswith(test_base)]
         result = {}
         for build, tests in builds_to_tests.iteritems():
             for test in tests:
                 if only_changed_tests and test not in tests_in_cl:
                     continue
                 if test not in result:
                     result[test] = {}
                 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())
 
         new_failures = self._fetch_tests_with_new_failures(build)
         if new_failures is None:
             _log.warning('No retry summary available for build %s.', build)
         else:
             tests = [t for t in tests if t in new_failures]
         return tests
 
     def _fetch_tests_with_new_failures(self, build):
         """Fetches a list of tests that failed with a patch in a given try job but not without."""
         buildbot = self._tool.buildbot
         content = buildbot.fetch_retry_summary_json(build)
         if content is None:
             return None
         try:
             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
     def _log_test_prefix_list(test_prefix_list):
         """Logs the tests to download new baselines for."""
         if not test_prefix_list:
             _log.info('No tests to rebaseline; exiting.')
             return
         _log.debug('Tests to rebaseline:')
         for test, builds in test_prefix_list.iteritems():
             _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