[Findit] Modify result page to display tryjob result for test failure.

appengine/findit/handlers/try_job_result.py

Index: appengine/findit/handlers/try_job_result.py
diff --git a/appengine/findit/handlers/try_job_result.py b/appengine/findit/handlers/try_job_result.py
index 7bceeff552fa2b66f279b42dc1c5ec057866ed82..051814bb2cc5a86667d9e33c4db1091e3c1a5c34 100644
--- a/appengine/findit/handlers/try_job_result.py
+++ b/appengine/findit/handlers/try_job_result.py
@@ -10,34 +10,95 @@ from model.wf_try_job import WfTryJob
 from waterfall import buildbot
 
 
-def _GetTryJobResult(master_name, builder_name, build_number):
-  # Get the latest try job result if it's compile failure.
+def _GetTryJobBuildNumber(url):
+  build_keys = buildbot.ParseBuildUrl(url)
+  return build_keys[2]
+
+
+def _GetCulpritInfoForTryJobResult(try_job_key, culprits_info):
+  referred_build_keys = try_job_key.split('/')
+  try_job = WfTryJob.Get(
+        referred_build_keys[0], referred_build_keys[1], referred_build_keys[2])

[qyearsley, 2016/02/03 18:38:28]

Nit: indent should be 4 spaces

Also, if referred_build_keys should always have a length of 3, you could also
do:
try_job = WfTryJob.Get(*referred_build_keys)

[chanli, 2016/02/03 19:24:42]

Done.

+  if try_job:

[lijeffrey, 2016/02/03 06:08:37]

nit: maybe to make this slightly more readable do something like

if not try_job:
  return

if try_job.compile_results:
  ...

[chanli, 2016/02/03 19:20:27]

Done.

+    if try_job.compile_results:
+      try_job_result = try_job.compile_results[-1]
+    elif try_job.test_results:
+      try_job_result = try_job.test_results[-1]
+    else:
+      try_job_result = None
+
+    for culprit_info in culprits_info.values():
+      if culprit_info['try_job_key'] != try_job_key:
+        continue
+
+      culprit_info['status'] = (
+          wf_analysis_status.TRY_JOB_STATUS_TO_DESCRIPTION[try_job.status])
+
+      if try_job_result:
+        if try_job_result.get('url'):
+          culprit_info['try_job_url'] = try_job_result['url']
+          culprit_info['try_job_build_number'] = (
+              _GetTryJobBuildNumber(try_job_result['url']))
+        if try_job_result.get('culprit'):
+          try_job_culprits = try_job_result['culprit']
+          step = culprit_info['step']
+          test = culprit_info['test']
+          if step in try_job_culprits:  # pragma: no cover

[lijeffrey, 2016/02/03 06:08:37]

is it possible to make a test that covers this?

[chanli, 2016/02/03 19:20:27]

This line has been removed since it'll never be reached.

+            if not try_job_culprits[step].get('tests',{}):  # Only step level

[lijeffrey, 2016/02/03 06:08:37]

nit: whitespace 'tests', {}

[chanli, 2016/02/03 19:20:27]

Done.

+              culprit = try_job_culprits[step]
+            elif test in try_job_culprits[step]['tests']:  # pragma: no cover
+              culprit = try_job_culprits[step]['tests'][test]
+
+            culprit_info['revision'] = culprit['revision']

[lijeffrey, 2016/02/03 06:08:37]

nit: since the others are using .get, why not use .get('rivision') here as well?

[qyearsley, 2016/02/03 18:38:28]

Is "revision" required (cannot be None) and the others are optional (can be
None)? If so, then using bracket notation seems OK to me since it implies 'I
want a KeyError to be raised if "revision" is not in culprit_info'.

[chanli, 2016/02/03 19:20:27]

This change is actually optional because revision is guaranteed but not the
other 2.

[chanli, 2016/02/03 19:24:42]

It's actually more of culprit is created only when there is a revision.

+            culprit_info['commit_position'] = culprit.get('commit_position')
+            culprit_info['review_url'] = culprit.get('review_url')
+
+
+def _GetAllTryJobResults(master_name, builder_name, build_number):
+  culprits_info = {}
+  try_job_keys = []
+
   analysis = WfAnalysis.Get(master_name, builder_name, build_number)
   if not analysis:

[lijeffrey, 2016/02/03 06:08:37]

nit: why not keep this the same, then declare culprits_info and try_job_keys
after the if statement?

[chanli, 2016/02/03 19:20:27]

To be consistent with below, I'm returning culprits_info here.

-    return {}
+    return culprits_info
 
   failure_result_map = analysis.failure_result_map
+  if failure_result_map:
+    # failure_result_map uses step_names as keys and save referred_try_job_keys.

[lijeffrey, 2016/02/03 06:08:37]

nit: saves

[lijeffrey, 2016/02/03 06:08:37]

what's referred_try_job_key?

[chanli, 2016/02/03 19:20:27]

Done.

[chanli, 2016/02/03 19:20:27]

Is 'referred try_job_key' better? Basically it is the key point to try job
result of first failed build. 

+    # If non-swarming, step_name and referred_try_job_key matches directly as:

[lijeffrey, 2016/02/03 06:08:37]

nit: match

[chanli, 2016/02/03 19:20:27]

Done.

+    # step_name: try_job_key
+    # If swarming, add one more layer of tests, so the format would be:
+    # step_name: {
+    #     test_name1: try_job_key1,
+    #     test_name2: try_job_key2,
+    #     ...
+    # }
+    for failed_step in failure_result_map:
+      if isinstance(failure_result_map[failed_step], dict):
+        for failed_test, try_job_key in (
+            failure_result_map[failed_step].iteritems()):
+          step_test_key = '%s-%s' % (failed_step, failed_test)
+          culprits_info[step_test_key] = {
+              'step': failed_step,
+              'test': failed_test,
+              'try_job_key': try_job_key
+          }
+          if try_job_key not in try_job_keys:
+            try_job_keys.append(try_job_key)
+      else:
+        culprits_info[failed_step] = {
+            'step': failed_step,
+            'test': 'N/A',
+            'try_job_key': failure_result_map[failed_step]
+        }
+        if (failure_result_map[failed_step] not in
+            try_job_keys):  # pragma: no cover
+          try_job_keys.append(failure_result_map[failed_step])
+
+    for try_job_key in try_job_keys:
+      _GetCulpritInfoForTryJobResult(try_job_key, culprits_info)
 
-  if failure_result_map and failure_result_map.get('compile'):
-    try_job_key = failure_result_map['compile']
-    point_build_keys = try_job_key.split('/')
-    try_job = WfTryJob.Get(
-          point_build_keys[0], point_build_keys[1], point_build_keys[2])
-    if try_job:
-      try_job_result = {}
-      try_job_result['status'] = (
-          wf_analysis_status.STATUS_TO_DESCRIPTION[try_job.status])
-      if try_job.compile_results:
-        if try_job.compile_results[-1].get('culprit'):
-          culprit = try_job.compile_results[-1]['culprit']
-          try_job_result['revision'] = culprit['revision']
-          try_job_result['commit_position'] = culprit['commit_position']
-          try_job_result['review_url'] = culprit['review_url']
-        if try_job.compile_results[-1].get('url'):
-          try_job_result['try_job_url'] = try_job.compile_results[-1]['url']
-      return try_job_result
-
-  return {}
+  return culprits_info
 
 
 class TryJobResult(BaseHandler):
@@ -51,7 +112,7 @@ class TryJobResult(BaseHandler):
     if not build_keys:  # pragma: no cover
       return {'data': {}}
 
-    data = _GetTryJobResult(*build_keys)
+    data = _GetAllTryJobResults(*build_keys)
 
     return {'data': data}