[Findit] Adding support for extracting revisions from dict instead of list

appengine/findit/waterfall/identify_try_job_culprit_pipeline.py

 # Copyright 2015 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.
 
 from common.git_repository import GitRepository
 from common.http_client_appengine import HttpClientAppengine as HttpClient
 from model import wf_analysis_status
 from model.wf_try_job import WfTryJob
 from pipeline_wrapper import BasePipeline
 
 
 class IdentifyTryJobCulpritPipeline(BasePipeline):
   """A pipeline to identify culprit CL info based on try job compile results."""
 
+  def _getFailedRevisionFromResultsDict(self, results_dict):

[qyearsley, 2016/01/24 21:01:06]

Nit: In Chromium style, function names start with a capital (except in special
cases where the name is determined by something else, e.g the special name run
below and the test method names which start with test).

[lijeffrey, 2016/01/26 00:34:52]

Done.

+    """Finds the failed revision from the given dict of revisions.
+
+    Args:
+        results_dict: (dict) A dict that maps revisions to their results. For
+        example:
+
+        {
+            'rev1': 'passed',
+            'rev2': 'passed',
+            'rev3': 'failed',
+        }
+
+        Note results_dict is expected only to have one failed revision which
+        will be the one to be returned.
+
+    Returns:
+        The revision corresponding to a failed result, if any.

[qyearsley, 2016/01/24 21:01:05]

Nit: Only need two spaces of indentation for Args and Returns section of
docstring.

[lijeffrey, 2016/01/26 00:34:52]

Done.

+    """
+    for revision, result in results_dict.iteritems():
+      if result.lower() == 'failed':
+        return revision
+    return None
+
   # Arguments number differs from overridden method - pylint: disable=W0221
   def run(
       self, master_name, builder_name, build_number, try_job_id,
       compile_result):
     culprit = None
 
-    if compile_result and len(compile_result.get('result', [])) > 0:
-      # For compile failures, the try job will stop if one revision fails, so
-      # the culprit will be the last revision in the result.
-      result_for_last_checked_revision = compile_result['result'][-1]
-      failed_revision = (
-          result_for_last_checked_revision[0] if
-          result_for_last_checked_revision[1].lower() == 'failed' else None)
+    if compile_result:
+      failed_revision = None
+      result_list = compile_result.get('result')
+      if result_list and isinstance(result_list, list):
+        # TODO(lijeffrey): The format for the result of the compile will change
+        # from a list to a dict. This branch is for backwards compatibility and
+        # should be removed once result is returned as a dict from the compile
+        # recipe. The test recipe may need to be considered as well.
+
+        # For compile failures, the try job will stop if one revision fails, so
+        # the culprit will be the last revision in the result.
+        result_for_last_checked_revision = compile_result['result'][-1]
+        failed_revision = (
+            result_for_last_checked_revision[0] if
+            result_for_last_checked_revision[1].lower() == 'failed' else None)
+      elif isinstance(compile_result.get('analysis_info'), dict):
+        revision_results = compile_result.get(
+            'analysis_info', {}).get('result', {})
+        failed_revision = self._getFailedRevisionFromResultsDict(
+            revision_results)

[stgao, 2016/01/25 19:19:52]

How about moving lines #46 - #64 into the new function above and making that
function handle both format?

If you make the new function a @staticmethod, I believe that the unittests below
could be much simplified too as we only need to test the new function itself.

[lijeffrey, 2016/01/26 00:34:52]

Done.

 
       if failed_revision:
         git_repo = GitRepository(
-            'https://chromium.googlesource.com/chromium/src.git', HttpClient())
+            'https://chromium.googlesource.com/chromium/src.git',
+            HttpClient())

[lijeffrey, 2016/01/23 02:24:09]

oops I'll put this back

[lijeffrey, 2016/01/26 00:34:52]

Done.

         change_log = git_repo.GetChangeLog(failed_revision)
         if change_log:
           culprit = {
               'revision': failed_revision,
               'commit_position': change_log.commit_position,
               'review_url': change_log.code_review_url
           }
           compile_result['culprit'] = culprit

[qyearsley, 2016/01/24 21:01:06]

This section (inside `if compile_result`) could potentially be extracted to a
helper method -- something like:

def _FindCulpritFromCompileResult(self, compile_result):
  """Returns a culprit (dict) if found, else None."""
  ...

[lijeffrey, 2016/01/26 00:34:52]

Done.

 
     # Store try job results.
     try_job_result = WfTryJob.Get(master_name, builder_name, build_number)
     if culprit:
       if (try_job_result.compile_results and
           try_job_result.compile_results[-1]['try_job_id'] == try_job_id):
         try_job_result.compile_results[-1].update(compile_result)
       else:  # pragma: no cover
         try_job_result.compile_results.append(compile_result)
 
     try_job_result.status = wf_analysis_status.ANALYZED
     try_job_result.put()
 
     return culprit