[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..568b5a8cb0060d04f9970ba4bcdc90e94e9322d3 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -12,35 +12,125 @@ from pipeline_wrapper import BasePipeline
 class IdentifyTryJobCulpritPipeline(BasePipeline):
   """A pipeline to identify culprit CL info based on try job compile results."""
 
-  # 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
+  @staticmethod
+  def _GetFailedRevisionFromResultsDict(results_dict):
+    """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.
+    """
+    for revision, result in results_dict.iteritems():
+      if result.lower() == 'failed':
+        return revision
+    return None
+
+  @staticmethod
+  def _GetFailedRevisionFromCompileResult(compile_result):
+    """Determines the failed revision given compile_result.
+
+    Args:
+      compile_result: A dict containing the results from a compile. Curently two
+        formats are supported.
+
+        The old format:
+        {
+            'report': [
+                ['rev1', 'passed'],
+                ['rev2', 'failed']
+            ],
+            'url': try job url,
+            'try_job_id': try job id
+        }
+
+        The new format:
+        {
+            'report': {
+                'result': {
+                    'rev1': 'passed',
+                    'rev2': 'failed',
+                },
+                ... (other metadata from the compile result)
+            },
+            'url': try job url,
+            'try_job_id': try job id
+        }
+
+    Returns:
+      The failed revision from compile_results, or None if not found.
+    """
+    if not compile_result:
+      return None
+
+    report = compile_result.get('report')
+
+    if not report:
+      return None
+
+    failed_revision = None
+
+    if isinstance(report, list):

[chanli, 2016/01/27 18:58:10]

It just occurs to me, since we always check the result from running try job
here, report should always be dict, right?

I think the only place need to be compatible is the result page?

[lijeffrey, 2016/01/28 02:14:35]

in the old format result (now renamed report) is still a list. I think the
result page should be fine, since it still consumes a try_job_result dict which
contains the culprit separately. I'll double check this once we make the change
to return the report dict from the recipe

+      # 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.
 
-    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]
+      result_for_last_checked_revision = report[-1]
       failed_revision = (
           result_for_last_checked_revision[0] if
           result_for_last_checked_revision[1].lower() == 'failed' else None)
+    else:
+      revision_results = report.get('result', {})
+      failed_revision = (
+          IdentifyTryJobCulpritPipeline._GetFailedRevisionFromResultsDict(
+              revision_results))
+
+    return failed_revision
+
+  @staticmethod
+  def _GetCulpritFromFailedRevision(failed_revision):
+    """Returns a culprit (dict) using failed_revision, or None."""
+    if not failed_revision:
+      return None
 
-      if failed_revision:
-        git_repo = GitRepository(
-            'https://chromium.googlesource.com/chromium/src.git', HttpClient())
-        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
+    git_repo = GitRepository(
+        'https://chromium.googlesource.com/chromium/src.git', HttpClient())
+    change_log = git_repo.GetChangeLog(failed_revision)
+
+    if not change_log:
+      return None

[stgao, 2016/01/27 01:24:20]

In this case, should we still return the failed revision as it is already found?

return { 'revision': failed_revision}

[lijeffrey, 2016/01/28 02:14:35]

This will be addressed in a separate CL
https://code.google.com/p/chromium/issues/detail?id=581922

+
+    return {
+        'revision': failed_revision,
+        'commit_position': change_log.commit_position,
+        'review_url': change_log.code_review_url
+    }
+
+  # 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
+    failed_revision = self._GetFailedRevisionFromCompileResult(compile_result)
+    culprit = self._GetCulpritFromFailedRevision(failed_revision)
 
     # Store try job results.
     try_job_result = WfTryJob.Get(master_name, builder_name, build_number)
     if culprit:
+      compile_result['culprit'] = 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)