Updating swarming api to check for chartjson results from a finished

scripts/slave/recipe_modules/swarming/api.py

 # Copyright 2014 The Chromium Authors. All rights reserved.
 # 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
 
 from recipe_engine import recipe_api
 
 
 # Minimally supported version of swarming.py script (reported by --version).
 MINIMAL_SWARMING_VERSION = (0, 8, 6)
 
 
@@ skipping 385 matching lines @@
     extra_args.append(
         '--test-launcher-summary-output=${ISOLATED_OUTDIR}/output.json')
 
     # Make a task, configure it to be collected through shim script.
     task = self.task(title, isolated_hash, extra_args=extra_args,
                      cipd_packages=cipd_packages, **kwargs)
     task.collect_step = lambda *args, **kw: (
         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.
 
     Swarming recipe module knows how collect JSON file with test execution
     summary produced by isolated script tests launcher. Since isolated script
     tests do not support sharding, no merging of the results is performed.
     Parsed JSON summary is returned from the collect step.
 
     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)
     task.collect_step = self._isolated_script_collect_step
     return task
 
   def check_client_version(self, step_test_data=None):
     """Yields steps to verify compatibility with swarming_client version."""
     return self.m.swarming_client.ensure_script_version(
         'swarming.py', MINIMAL_SWARMING_VERSION, step_test_data)
@@ skipping 267 matching lines @@
         # Show any remaining isolated outputs (such as logcats).
         # Note that collect_gtest_task.py uses the default summary.json, which
         # only has 'outputs_ref' instead of the deprecated 'isolated_out'.
         for index, shard in enumerate(swarming_summary.get('shards', [])):
           outputs_ref = shard.get('outputs_ref')
           if outputs_ref:
             link_name = 'shard #%d isolated out' % index
             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
     # isolated scripts should be changed to use the now-standard
     # Chromium JSON test results format:
     # https://www.chromium.org/developers/the-json-test-results-format
     # . Note that gtests, above, don't seem to conform to this
     # format yet, so it didn't seem like a good prerequisite to
     # switch the isolated tests over when adding sharding support.
     #
@@ skipping 62 matching lines @@
         # in recipe runs.)
         links = step_result.presentation.links
         for index in xrange(task.shards):
           url = task.get_shard_view_url(index)
           if url:
             links['shard #%d' % index] = url
 
         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)]
         self.m.step.active_result.isolated_script_results = None
 
   def _get_step_name(self, prefix, task):
     """SwarmingTask -> name of a step of a waterfall.
 
     Will take a task title (+ step name prefix) and append OS dimension to it.
 
@@ skipping 177 matching lines @@
 
   def get_shard_view_url(self, index):
     """Returns URL of HTML page with shard details or None if not available.
 
     Works only after the task has been successfully triggered.
     """
     if self._trigger_output and self._trigger_output.get('tasks'):
       for shard_dict in self._trigger_output['tasks'].itervalues():
         if shard_dict['shard_index'] == index:
           return shard_dict['view_url']