Add json test results format support for SwarmingIsolatedScriptTest

scripts/slave/recipe_modules/swarming/api.py

Index: scripts/slave/recipe_modules/swarming/api.py
diff --git a/scripts/slave/recipe_modules/swarming/api.py b/scripts/slave/recipe_modules/swarming/api.py
index da5afa4fa99aee6f42037ffb3c88659aba0d93e0..0aa2b99960d287bf68b12bf95f25771b855bb46d 100644
--- a/scripts/slave/recipe_modules/swarming/api.py
+++ b/scripts/slave/recipe_modules/swarming/api.py
@@ -770,6 +770,77 @@ class SwarmingApi(recipe_api.RecipeApi):
             merged_results[key][add_key] = chartjson_results_json[key][add_key]
     return merged_results
 
+  def _merge_simplified_test_result_format(self, shard_results_list):
+    merged_results = {
+      'successes': [],
+      'failures': [],
+      'valid': True,
+    }
+    for results_json in shard_results_list:
+      for key in merged_results:
+        if key in results_json:
+          if isinstance(merged_results[key], list):
+            merged_results[key].extend(results_json[key])
+          elif isinstance(merged_results[key], bool):
+            merged_results[key] = merged_results[key] and results_json[key]
+          else:
+            raise recipe_api.InfraFailure(
+              'Unknown key type ' + type(merged_results[key]) +
+              ' when handling key ' + key + '.')  # pragma: no cover
+    return merged_results
+
+  def _merge_json_test_result_format(self, shard_results_list):
+    merged_results = {
+      'tests': {},
+      'interrupted': false,
+      'path_delimiter': '',
+      'version': 3,
+      'seconds_since_epoch': float('inf'),
+      'num_failures_by_type': {
+      }
+    }
+    for result_json in shard_results_list:
+      if not ('tests' in result_json and
+              'interrupted' in result_json and
+              'path_delimiter' in result_json and
+              'version' in result_json and
+              'seconds_since_epoch' in result_json and
+              'num_failures_by_type' in result_json):
+        raise recipe_api.InfraFailure('Invalid json test results')

[Sergiy Byelozyorov, 2016/09/29 21:36:22]

I would double-check if everybody implements the full spec... I remember seeing
some fields missing in some test results, so I treated them all as optional.

[nednguyen, 2016/10/04 00:10:11]

Hmhh, the spec say these fields are required? 
http://www.chromium.org/developers/the-json-test-results-format

Currently, this is not used by anyone who use the script_isolated_test recipe,
so I think it's better to follow the spec here.

[Sergiy Byelozyorov, 2016/10/04 08:54:52]

Sure. I just meant that you should make sure it doesn't break with real-world
results. If you find any launchers that do not follow the spec, please file bugs
into Infra>Flakiness>Dashboard component for them.

[Dirk Pranke, 2016/10/04 20:04:02]

We should try to follow the spec and update the spec if need be if we can't
follow it, but we shouldn't arbitrarily diverge from the spec. 

(This is probably obvious).

+
+      # Traverse the results_json's test trie & merged_results's test tries in
+      # DFS order & add the n to merged['tests'].
+      curr_result_nodes_queue = [results_json['tests']]
+      merged_results_nodes_queue = [merged_results['tests']]
+      while nodes_queue:

[Sergiy Byelozyorov, 2016/09/29 21:36:22]

I don't see nodes_queue defined anywhere... does this have tests?

[nednguyen, 2016/10/04 00:10:11]

Yeah, I wasn't sure how to test these. Done.

+        curr_node = curr_result_nodes_queue.pop()
+        merged_node = merged_results_nodes_queue.pop()
+        for k, v in curr_node.iteritems():
+          if k in merged_node:
+            curr_result_nodes_queue.push(v)
+            merged_results_nodes_queue.push(merged_node[k])
+          else:
+            merged_node[k] = v
+
+      # Update the rest of the fields for merged_results.
+      merged_results['interrupted'] |= results_json['interrupted']
+      if not merged_results['path_delimiter']:
+        merged_results['path_delimiter'] = results_json['path_delimiter']
+      elif merged_results['path_delimiter'] != results_json['path_delimiter']:
+        raise Exception(
+            'Incosistent path delimiter: %s %s' %
+            (merged_results['path_delimiter'],
+             results_json['path_delimiter']))
+      if results_json['version'] != 3:
+        raise recipe_api.InfraFailure(
+            'Only version 3 of json test result format is supported')
+      merged_results['seconds_since_epoch'] = min(
+          merged_results['seconds_since_epoch'],
+          results_json['seconds_since_epoch'])

[Sergiy Byelozyorov, 2016/09/29 21:36:22]

Any particular reason to pick the smaller of the two values?

[nednguyen, 2016/10/04 00:10:11]

I honestly not sure what is the reason for this field & the spec says this is
required. Do you have any suggestion?

[Sergiy Byelozyorov, 2016/10/04 08:54:52]

AFAIK, this is when the tests were run... probably some random time measured by
the test launcher. Minimum of the two values is perfectly find with me. I was
just wondering if this was just an arbitrary choice or whether there was some
logic behind it.

[nednguyen, 2016/10/04 13:30:29]

Ah, I just think that the earliest seconds_since_epoch time of all the shards
best represent the seconds_since_epoch time of when the suite actually starts.

+      for result_type, count in results_json['num_failures_by_type']:
+        if not result_type in merged_results['num_failures_by_type']:

[Sergiy Byelozyorov, 2016/09/29 21:36:22]

IMHO this is more Pythonic:

merged_results['num_failures_by_type'].setdefault(result_type, 0)

[nednguyen, 2016/10/04 00:10:11]

Done.

+          merged_results['num_failures_by_type'][result_type] = 0
+        merged_results['num_failures_by_type'][result_type] += count
 
   def _merge_isolated_script_shards(self, task, step_result):
     # This code is unfortunately specialized to the "simplified"
@@ -782,11 +853,7 @@ class SwarmingApi(recipe_api.RecipeApi):
     # switch the isolated tests over when adding sharding support.
     #
     # These are the only keys we pay attention to in the output JSON.

[Ken Russell (switch to Gerrit), 2016/09/30 23:37:53]

Please move this comment to the top of _merge_simplified_test_result_format and
adjust as necessary.

[nednguyen, 2016/10/04 00:10:11]

Done.

-    merged_results = {
-      'successes': [],
-      'failures': [],
-      'valid': True,
-    }
+    shard_results_list = []
     for i in xrange(task.shards):
       path = self.m.path.join(str(i), 'output.json')
       if path not in step_result.raw_io.output_dir:
@@ -794,19 +861,15 @@ class SwarmingApi(recipe_api.RecipeApi):
       results_raw = step_result.raw_io.output_dir[path]
       try:
         results_json = self.m.json.loads(results_raw)
+        shard_results_list.append(results_json)
       except Exception as e:
         raise Exception('error decoding JSON results from shard #%d' % i)
-      for key in merged_results:
-        if key in results_json:
-          if isinstance(merged_results[key], list):
-            merged_results[key].extend(results_json[key])
-          elif isinstance(merged_results[key], bool):
-            merged_results[key] = merged_results[key] and results_json[key]
-          else:
-            raise recipe_api.InfraFailure(
-              'Unknown key type ' + type(merged_results[key]) +
-              ' when handling key ' + key + '.')  # pragma: no cover
-    return merged_results
+    if not shard_results_list:

[eyaich1, 2016/09/29 14:55:23]

no need for this check you create it above

[Sergiy Byelozyorov, 2016/09/29 21:36:22]

But it can be empty, can't it?

+      raise Exception('No shard results is created')

[Ken Russell (switch to Gerrit), 2016/09/30 23:37:53]

Grammar: remove "is".

[nednguyen, 2016/10/04 00:10:11]

Done.

+    elif 'seconds_since_epoch' in shard_results_list[0]:

[eyaich1, 2016/09/29 14:55:23]

is this the distinguishing factor between the results from unittests vs perf
tests?  Is that the correct variant to be checking?

[nednguyen, 2016/10/04 00:10:11]

This is the distinguished factor between the simplified json result vs chromium
json test result.

+      return self._merge_json_test_result_format[shard_results_list]
+    else:
+      return self._merge_simplified_json_format(shard_results_list)
 
   def _isolated_script_collect_step(self, task, **kwargs):
     step_test_data = kwargs.pop('step_test_data', None)