[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..37ab8f69e821aff0784db8530d6e39cb4f746dcb 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -12,35 +12,92 @@ 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 _FindFailedRevisionFromCompileResult(compile_result):
+    """Returns the failed revision (string) from compile_result if any."""
+    if not compile_result:
+      return None
+
+    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.
 
-    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]

[stgao, 2016/01/26 01:22:33]

result -> report.

Need to update buildbucket_client.py and monitor_try_job_pipeline.py too.
And in monitor_try_job_pipeline.py, return the build.result/report directly.

[lijeffrey, 2016/01/27 01:06:00]

Done.

       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):

[stgao, 2016/01/26 01:22:33]

compile_result.get('analysis_info') -> report.get('result')

[chanli, 2016/01/26 18:15:28]

Is this format finalized?

[stgao, 2016/01/26 18:32:26]

Jeff and I had a discussion on the new format to make it extensible.
We could discuss more on it.

What's your recommendation?

[chanli, 2016/01/26 18:52:19]

Should the format like what Jeff mentioned in the description?
{
  'analysis_info': {
    'results': {
      'rev1': 'passed',
      'rev2': 'failed',
    }
  }
}

If so we need to modify monitor_try_job_pipeline as well.

[stgao, 2016/01/26 19:03:35]

Yes, very similar to that, expect no root node "analysis_info". I guess Jeff
will update the CL description when he uploads the new patch.

[lijeffrey, 2016/01/27 01:06:00]

Done.

+      revision_results = compile_result.get(
+          'analysis_info', {}).get('result', {})
+      failed_revision = (
+          IdentifyTryJobCulpritPipeline._GetFailedRevisionFromResultsDict(
+              revision_results))
 
-      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
+    return failed_revision
+
+  @staticmethod
+  def _GetCulpritFromFailedRevision(failed_revision):
+    """Returns a culprit (dict) using failed_revision, or None."""
+    if not failed_revision:
+      return None
+
+    culprit = None
+    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
+      }
+
+    return culprit

[qyearsley, 2016/01/26 00:40:04]

Slightly different way to write the same thing:

  ...
  if not change_log:
    return None

  return {
      'revision': failed_revision,
      'commit_position': change_log.commit_position,
      'review_url': change_log.code_review_url
  }

[lijeffrey, 2016/01/27 01:06:00]

Done.

+
+  # 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._FindFailedRevisionFromCompileResult(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)