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

appengine/findit/waterfall/identify_try_job_culprit_pipeline.py

Index: appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
diff --git a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
index 873db7b33193b2f654898b808481ec07e649e6ce..3139eb6d9cb76d1e604d4e5da82de3f67b6a717a 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -12,23 +12,61 @@ 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 = {