[Findit] Adding Try Job suspected CLs and result status to analysis

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 503f0749f143e5325e32f73c3280d222806220fe..6a8228c04427341b6f93f8b3a4d0005143a9b25d 100644
--- a/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
+++ b/appengine/findit/waterfall/identify_try_job_culprit_pipeline.py
@@ -6,6 +6,8 @@ from common.git_repository import GitRepository
 from common.http_client_appengine import HttpClientAppengine as HttpClient
 from common.pipeline_wrapper import BasePipeline
 from model import analysis_status
+from model import result_status
+from model.wf_analysis import WfAnalysis
 from model.wf_try_job import WfTryJob
 from model.wf_try_job_data import WfTryJobData
 from waterfall.try_job_type import TryJobType
@@ -15,15 +17,98 @@ GIT_REPO = GitRepository(
     'https://chromium.googlesource.com/chromium/src.git', HttpClient())
 
 
+def _GetResultAnalysisStatus(analysis, result):
+  """Returns the analysis status based on existing status and try job result.
+
+  Args:
+    analysis: The WfAnalysis entity corresponding to this try job.
+    result: A result dict containing the result of this try job.
+
+  Returns:
+    A result_status code.
+  """
+  # Only return an updated analysis result status if no results were already
+  # found (by the heuristic-based approach) but were by the try job. Note it is
+  # possible the heuristic-based result was triaged before the completion of
+  # this try job.
+  old_result_status = analysis.result_status
+  try_job_found_culprit = result and result.get('culprit')
+
+  if old_result_status is None:
+    if try_job_found_culprit:
+      return result_status.FOUND_UNTRIAGED
+    return result_status.NOT_FOUND_UNTRIAGED
+
+  if (try_job_found_culprit and
+      (old_result_status == result_status.NOT_FOUND_UNTRIAGED or
+       old_result_status == result_status.NOT_FOUND_INCORRECT or
+       old_result_status == result_status.NOT_FOUND_CORRECT)):
+    return result_status.FOUND_UNTRIAGED
+
+  return old_result_status
+
+
+def _GetSuspectedCLs(analysis, result):
+  """Returns a list of suspected CLs.
+
+  Args:
+    analysis: The WfAnalysis entity corresponding to this try job.
+    result: A result dict containing the culprit from the results of
+      this try job.
+
+  Returns:
+    A combined list of suspected CLs from those already in analysis and those
+    found by this try job.
+  """
+
+  suspected_cls = analysis.suspected_cls[:] if analysis.suspected_cls else []
+
+  if not result:
+    return suspected_cls
+
+  culprit = result.get('culprit')
+  if not culprit:
+    return suspected_cls
+
+  compile_cl_info = culprit.get('compile')
+  if compile_cl_info:
+    # Suspected CL is from compile failure.
+    if compile_cl_info not in suspected_cls:
+      suspected_cls.append(compile_cl_info)
+    return suspected_cls
+
+  # Suspected CLs are from test failures.
+  for results in culprit.itervalues():
+    if results.get('revision'):
+      # Non swarming test failures, only have step level failure info.
+      cl_info = {
+          'review_url': results.get('review_url'),
+          'repo_name': results.get('repo_name'),
+          'revision': results.get('revision'),
+          'commit_position': results.get('commit_position')
+      }
+      if cl_info not in suspected_cls:
+        suspected_cls.append(cl_info)
+    else:
+      for test_cl_info in results['tests'].values():
+        if test_cl_info not in suspected_cls:
+          suspected_cls.append(test_cl_info)
+
+  return suspected_cls
+
+
 class IdentifyTryJobCulpritPipeline(BasePipeline):
   """A pipeline to identify culprit CL info based on try job compile results."""
 
   def _GetCulpritInfo(self, failed_revisions):
     """Gets commit_positions and review_urls for revisions."""
     culprits = {}
+    # TODO(lijeffrey): remove hard-coded 'chromium' when DEPS file parsing is
+    # supported.
     for failed_revision in failed_revisions:
       culprits[failed_revision] = {
-          'revision': failed_revision
+          'revision': failed_revision,
+          'repo_name': 'chromium'
       }
       change_log = GIT_REPO.GetChangeLog(failed_revision)
       if change_log:
@@ -106,6 +191,7 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
             not culprit_map[step].get('revision')):
           # Non swarming test failures, only have step level failure info.
           culprit_map[step]['revision'] = revision
+          culprit_map[step]['repo_name'] = 'chromium'
 
         for failed_test in test_result['failures']:
           # Swarming tests, gets first failed revision for each test.
@@ -188,4 +274,14 @@ class IdentifyTryJobCulpritPipeline(BasePipeline):
     try_job_result.status = analysis_status.COMPLETED
     try_job_result.put()
 
+    # Update analysis result and suspected CLs with results of this try job.
+    analysis = WfAnalysis.Get(master_name, builder_name, build_number)

[stgao, 2016/04/12 22:41:19]

nit: If ``culprits`` is None or empty (which means that the try job didn't find
any culprit), do we still need to check?

[lijeffrey, 2016/04/13 21:36:24]

This is in case analysis_result was set to None by _GetAnalysisResultStatus from
identify_culprit_pipeline, which we want to update with NOT_FOUND_UNTRIAGED even
if 'culprits' is nne or empty, unless we want to keep it as None instead?

[lijeffrey, 2016/04/13 22:50:17]

Done. Per discussion we don't want to update even if the previous values are
None.

+    updated_result_status = _GetResultAnalysisStatus(analysis, result)
+    updated_suspected_cls = _GetSuspectedCLs(analysis, result)
+
+    if (analysis.result_status != updated_result_status or
+        analysis.suspected_cls != updated_suspected_cls):
+      analysis.result_status = updated_result_status
+      analysis.suspected_cls = updated_suspected_cls

[stgao, 2016/04/12 22:41:19]

A datastore write is needed as we want to update and save the data.

[lijeffrey, 2016/04/13 21:36:24]

Done. Good catch, was accidentally deleted.

+
     return result.get('culprit') if result else None