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)

[Ken Russell (switch to Gerrit), 2016/09/17 03:33:46]

I think there's a bug here and that this is returning None. I think it needs to
do:
  extra_args.append(output_arg)
  return extra_args

[eyaich1, 2016/09/19 17:56:16]

Nice catch.  I wonder why the unittests didn't catch that. Or I guess you just
have to try and understand the expectations to see if anything looks fishy like
you did?

[Ken Russell (switch to Gerrit), 2016/09/19 23:28:43]

Yes, unfortunately it's necessary to carefully study the expectations changes.
It's a longstanding problem that there aren't unittests separate from the recipe
expectations for much of this code. If you're interested in improving the state
of the code in this area I'm sure the Chrome Infra team would like to talk with
you.

+
   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')
+    # chartjson-output.json name is expected by benchmarks generating chartjson
+    # output
+    extra_args = self._check_and_set_output_flag(
+      extra_args,
+      'isolated-script-test-chartjson-output',
+      'chartjson-output.json')

[Ken Russell (switch to Gerrit), 2016/09/17 03:33:46]

Note: passing this down all the time has implications for all of the isolated
script test wrappers in Chromium's src/testing/scripts/ directory, per comments
on https://codereview.chromium.org/2331993003/ . Without updating all those
wrappers, adding this extra argument will break the tests on the commit queue.

Because the CQ for the tools/build workspace doesn't run any Chromium tests,
it's important to be as careful as possible with changes to the contract between
recipes and the Chromium workspace, and watch bots like the tryservers to make
sure there's no breakage when landing changes like this. e.g.:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng

[eyaich1, 2016/09/19 17:56:16]

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_chartjson_ouput_shards(self, task, step_result):
+    # 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 chart from each shard.
+    merged_results = {}
+    seen_first_shard = False
+    for i in xrange(task.shards):
+      path = self.m.path.join(str(i), 'chartjson-output.json')
+      if path not in step_result.raw_io.output_dir:
+        # chartjson results were not written for this shard, not an error,
+        # just return
+        logging.info("No chartjson present, isolated_script_chartjson_results "
+          + " will be unset.")

[Ken Russell (switch to Gerrit), 2016/09/17 03:33:46]

Not sure whether logging will show up as expected in the recipes' output. The
owners of this module (recommend asking vadimsh@, but also maruel@) would
probably know.

[eyaich1, 2016/09/19 17:56:16]

Honestly I am not sure it is necessary, especially if it is the common case for
now that chartjson results won't be present.  I will remove for now and check
with them.

+        continue
+      results_raw = step_result.raw_io.output_dir[path]
+      try:
+        chartjson_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 = chartjson_results_json
+        seen_first_shard = True
+        continue
+      for key in chartjson_results_json:
+        if key == 'charts':
+          merged_results[key].extend(chartjson_results_json[key])
+    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 chartjson results if present
+        step_result.isolated_script_chartjson_results = \
+          self._merge_isolated_script_chartjson_ouput_shards(task, step_result)
+
         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']