Updating swarming api to check for chartjson results from a finished

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 de26cf14f11a01db085f0a8f4b7654c3a986b60e..b2fe03b1fb563ed4624f202f343026dd48efda2f 100644
--- a/scripts/slave/recipe_modules/swarming/api.py
+++ b/scripts/slave/recipe_modules/swarming/api.py
@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import logging
 import datetime
 import functools
 
@@ -407,6 +408,19 @@ class SwarmingApi(recipe_api.RecipeApi):
         self._gtest_collect_step(test_launcher_summary_output, *args, **kw))
     return task
 
+  def _check_and_set_output_flag(self, extra_args, flag, output_file_name):
+    extra_args = list(extra_args or [])
+    # Ensure flag is not already passed. We are going to overwrite it.
+    flag_value = '--%s=' % flag
+    bad_args = any(x.startswith(flag_value) for x in extra_args)
+    if bad_args:  # pragma: no cover
+      error = '--%s should not be used' % flag
+      raise ValueError(error)
+
+    # Append it.
+    output_arg = '--%s=${ISOLATED_OUTDIR}/%s' % (flag, output_file_name)
+    return extra_args.append(output_arg)
+
   def isolated_script_task(self, title, isolated_hash, extra_args=None,
                            idempotent=False, **kwargs):
     """Returns a new SwarmingTask to run an isolated script test on Swarming.
@@ -418,18 +432,16 @@ class SwarmingApi(recipe_api.RecipeApi):
 
     For meaning of the rest of the arguments see 'task' method.
     """
-    extra_args = list(extra_args or [])
 
-    # Ensure --isolated-script-test-output is not already passed. We are going
-    # to overwrite it.
-    bad_args = any(
-        x.startswith('--isolated-script-test-output=') for x in extra_args)
-    if bad_args:  # pragma: no cover
-      raise ValueError('--isolated-script-test-output should not be used.')
-
-    # Append it. output.json name is expected by collect_gtest_task.py.
-    extra_args.append(
-        '--isolated-script-test-output=${ISOLATED_OUTDIR}/output.json')
+    # Ensure output flags are not already passed. We are going
+    # to overwrite them.
+    # output.json name is expected by collect_gtest_task.py.
+    extra_args = self._check_and_set_output_flag( \
+      extra_args, 'isolated-script-test-output', 'output.json')
+    # chart-output.json name is expected by benchmarks generating chartjson
+    # output
+    extra_args = self._check_and_set_output_flag( \

[Ken Russell (switch to Gerrit), 2016/09/13 19:34:47]

Should the addition of this command line argument be conditional? That would
need a two-sided change between the Chromium workspace's src/testing/buildbot
JSON files to set the flag for various tests, and this workspace to handle the
flag. Or should the rule be that if no file is written, the recipe silently
skips processing it?

Note that --isolated-script-test-output is mandatory. We should keep the
required interface as simple as possible, I think. (Once flakiness dashboard
results are supported, --isolated-script-test-output should be completely
removed.)

[eyaich1, 2016/09/14 18:14:27]

This code is not requiring the command line, it is actually checking for the
absence of it because it is setting it in this code.  So yes this code will add
an additional flag to each job, but that won't hurt anything will it?  If the
flag is not utilized (like I have added to the
run_telemetry_benchmark_as_googletest.py) then my code on line 751 will just
pass on it.  Although I just realize I don't return on the check so it isn't
currently working, but I will add it. 

Am I missing something when adding this flag to the job?  Are there additional
concerns? 

+      extra_args, 'isolated-script-test-chart-output', 'chart-output.json')

[Ken Russell (switch to Gerrit), 2016/09/13 19:34:46]

I'd like to suggest using "isolated-script-test-chartjson-output" if "chartjson"
is the well known name of this format.

[eyaich1, 2016/09/14 18:14:28]

Done.

 
     task = self.task(title, isolated_hash, extra_args=extra_args,
                      idempotent=idempotent, **kwargs)
@@ -717,6 +729,44 @@ class SwarmingApi(recipe_api.RecipeApi):
             p.links[link_name] = outputs_ref['view_url']
 
 
+  def _merge_isolated_script_chart_ouput_shards(self, task, step_result):

[Ken Russell (switch to Gerrit), 2016/09/13 19:34:46]

chart -> chartjson?

[eyaich1, 2016/09/14 18:14:28]

Done.

+    # Taken from third_party/catapult/telemetry/telemetry/internal/results/
+    # chart_json_output_formatter.py, the json entries are as follows:
+    # result_dict = {
+    #   'format_version': '0.1',
+    #   'next_version': '0.2',
+    #   'benchmark_name': benchmark_metadata.name,
+    #   'benchmark_description': benchmark_metadata.description,
+    #   'trace_rerun_options': benchmark_metadata.rerun_options,
+    #   'benchmark_metadata': benchmark_metadata.AsDict(),
+    #   'charts': charts,
+    # }
+    #
+    # Therefore, all entries should be the same and we should only need to merge
+    # the chat from each shard.

[Ken Russell (switch to Gerrit), 2016/09/13 19:34:46]

chat -> chart

[eyaich1, 2016/09/14 18:14:27]

Done.

+    merged_results = {}
+    seen_first_shard = False
+    for i in xrange(task.shards):
+      path = self.m.path.join(str(i), 'chart-output.json')
+      if path not in step_result.raw_io.output_dir:
+        logging.info("No chart json present, isolated_script_chart_results " +
+          " will be unset.")
+      results_raw = step_result.raw_io.output_dir[path]
+      try:
+        chart_results_json = self.m.json.loads(results_raw)
+      except Exception as e:
+        raise Exception('error decoding JSON results from shard #%d' % i)
+      if not seen_first_shard:
+        merged_results = chart_results_json
+        seen_first_shard = True
+        continue
+      for key in chart_results_json:
+        if key == 'charts':
+          merged_results[key].extend(results_json[key])
+        break
+    return merged_results
+
+
   def _merge_isolated_script_shards(self, task, step_result):
     # This code is unfortunately specialized to the "simplified"
     # JSON format that used to be the standard for recipes. The
@@ -799,6 +849,10 @@ class SwarmingApi(recipe_api.RecipeApi):
         step_result.isolated_script_results = \
           self._merge_isolated_script_shards(task, step_result)
 
+        # Obtain chart json results if present
+        step_result.isolated_script_chart_results = \

[Ken Russell (switch to Gerrit), 2016/09/13 19:34:46]

chart_results -> chartjson_results?

[eyaich1, 2016/09/14 18:14:27]

Done.

+          self._merge_isolated_script_chart_ouput_shards(task, step_result)

[Ken Russell (switch to Gerrit), 2016/09/13 19:34:46]

Is this sufficient to test the above code paths? Have you tried a CQ dry run of
this change? The recipe workspace requires 100% code coverage, and I'd be
surprised if all of the above code paths are covered, since as far as I see no
test tries passing this flag. See for example
scripts/slave/recipe_modules/test_utils/test_api.py,
canned_isolated_script_output, and its use in this file. I think you'll need
some canned chartjson output too.

[eyaich1, 2016/09/14 18:14:28]

wow thanks for pointing me at the test_api.py file.  Yes I did train the data
and have only ever ran that and had it generate new expectations, I never had to
add test coverage before.  I didn't know where to begin there, I really
appreciate it.

that being said I am still slightly confused by these tests.  After spending the
last 2.5 hours trying to write a test that hit every exception and failure case
and still missing 2 lines, I don't believe that the almost identical method,
_merge_isolated_script_shards has 100% test coverage. 

This test gets run multiple times with different parameters and I can figure out
a better way to manipulate data for failure cases other than setting specific if
statements on certain parameters and setting corrupt data and/or invalid
results.  It seems to be doing the same thing for every run, how are they
testing different scenarios? ie how are they testing lines 793-794 above?

+
         self._display_pending(summary, step_result.presentation)
       except Exception as e:
         self.m.step.active_result.presentation.logs['no_results_exc'] = [str(e)]