Updating swarming api to check for chartjson results from a finished

Updating swarming api to add chartjson output. 

This updates the api to check for chartjson results from a finished task in output directory with a file named chart-output.json.

BUG=chromium:633253
Committed: https://chromium.googlesource.com/chromium/tools/build/+/99542aa3517f274e8f444dd39290102fd75a2206

[eyaich1, 2016-09-13 16:36:36]

eyaich@chromium.org changed reviewers:
+ kbr@chromium.org

[eyaich1, 2016-09-13 16:36:43]

The CQ bit was checked by eyaich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 16:36:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 16:36:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 16:36:55]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-infra-committers".
Note that this has nothing to do with OWNERS files.

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

Looks like a better direction overall. A few comments.

Also, in the Chromium code review tool it's better if the first line in the CL
description is a self-contained sentence.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:443: extra_args =
self._check_and_set_output_flag( \
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.)

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:444: extra_args,
'isolated-script-test-chart-output', 'chart-output.json')
I'd like to suggest using "isolated-script-test-chartjson-output" if "chartjson"
is the well known name of this format.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:732: def
_merge_isolated_script_chart_ouput_shards(self, task, step_result):
chart -> chartjson?

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:746: # the chat from each shard.
chat -> chart

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:853:
step_result.isolated_script_chart_results = \
chart_results -> chartjson_results?

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:854:
self._merge_isolated_script_chart_ouput_shards(task, step_result)
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:02]

Description was changed from

==========
Updating swarming api to check for chartjson results from a finished
task in output directory with a file named chart-output.json.

BUG=chromium:633253
==========

to

==========
Updating swarming api to add chartjson output. 

This updates the api to check for chartjson results from a finished task in
output directory with a file named chart-output.json.

BUG=chromium:633253
==========

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

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:443: extra_args =
self._check_and_set_output_flag( \
On 2016/09/13 19:34:47, Ken Russell wrote:
> 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.)

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?

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:444: extra_args,
'isolated-script-test-chart-output', 'chart-output.json')
On 2016/09/13 19:34:46, Ken Russell wrote:
> I'd like to suggest using "isolated-script-test-chartjson-output" if
"chartjson"
> is the well known name of this format.

Done.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:732: def
_merge_isolated_script_chart_ouput_shards(self, task, step_result):
On 2016/09/13 19:34:46, Ken Russell wrote:
> chart -> chartjson?

Done.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:746: # the chat from each shard.
On 2016/09/13 19:34:46, Ken Russell wrote:
> chat -> chart

Done.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:853:
step_result.isolated_script_chart_results = \
On 2016/09/13 19:34:46, Ken Russell wrote:
> chart_results -> chartjson_results?

Done.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:854:
self._merge_isolated_script_chart_ouput_shards(task, step_result)
On 2016/09/13 19:34:46, Ken Russell wrote:
> 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.

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?

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

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:443: extra_args =
self._check_and_set_output_flag( \
On 2016/09/13 19:34:47, Ken Russell wrote:
> 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.)

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?

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:444: extra_args,
'isolated-script-test-chart-output', 'chart-output.json')
On 2016/09/13 19:34:46, Ken Russell wrote:
> I'd like to suggest using "isolated-script-test-chartjson-output" if
"chartjson"
> is the well known name of this format.

Done.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:732: def
_merge_isolated_script_chart_ouput_shards(self, task, step_result):
On 2016/09/13 19:34:46, Ken Russell wrote:
> chart -> chartjson?

Done.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:746: # the chat from each shard.
On 2016/09/13 19:34:46, Ken Russell wrote:
> chat -> chart

Done.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:853:
step_result.isolated_script_chart_results = \
On 2016/09/13 19:34:46, Ken Russell wrote:
> chart_results -> chartjson_results?

Done.

https://codereview.chromium.org/2336293002/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/swarming/api.py:854:
self._merge_isolated_script_chart_ouput_shards(task, step_result)
On 2016/09/13 19:34:46, Ken Russell wrote:
> 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.

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?

[eyaich1, 2016-09-15 18:35:11]

Ok PTAL

Finally understanding the recipe testing framework slightly better.  I got stuck
not realizing that the canned_isolated_script_output was being executed for the
yield statements as a response to the telemetry_gpu_unittests step as well as
again in the actual _isolated_script_collect_step, but the second call in the
actual method doesn't actually determine anything to do with what happens in the
rest of the method.  Slightly confusing. 

Anyways, I have added a few tests to exercise the corner cases.  As Dirk
mentioned in my other CL to modify the steps.py, this is not a scalable solution
for the swarming api for n output directories though.

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

Agree that it's difficult and not scalable to have to handle multiple kinds of
sharded output (JSON or otherwise) from these tests. Still, I think we have to
be very precise about what sort of output we are going to handle from them.

In the long run, the output needed from these tests is: (1) flakiness dashboard
output and (2) the chartjson results dashboard (is this the same thing as the
perf dashboard?). We can eliminate --isolated-script-test-output once (1) is
finally in place.

A couple of issues.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:422: return
extra_args.append(output_arg)
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

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:446: 'chartjson-output.json')
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

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:757: + " will be unset.")
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.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_failed_isolated_script_test.json
(left):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_failed_isolated_script_test.json:375:
"--isolated-script-test-output=${ISOLATED_OUTDIR}/output.json"
The loss here of the --isolated-script-test-output flag is a big warning sign
that something's wrong with this CL. See comment near the top of
scripts/slave/recipe_modules/swarming/api.py.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_isolated_script_test_linux_gpu.json
(left):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_isolated_script_test_linux_gpu.json:381:
"--isolated-script-test-output=${ISOLATED_OUTDIR}/output.json"
Same here.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_failed_isolated_script_test.json
(right):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_failed_isolated_script_test.json:430:
"@@@STEP_LOG_LINE@no_results_exc@'str' object has no attribute 'extend'@@@",
This exception looks problematic. It shows up in a couple of other places below
too.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.py (right):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.py:634: retcode=1)
As far as I can tell the parameters for this fake data are the same as for the
dynamic_swarmed_sharded_isolated_chartjson_test_missing_shard test, below. Can
this test be removed without losing 100% code coverage? If so let's do that.

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

Thanks for pointing me to the expectations files and the errors you saw.  I
hadn't looked at them much before and should have as it caught a couple of bugs.
 

Addressed all comments, PTAL

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:422: return
extra_args.append(output_arg)
On 2016/09/17 03:33:46, Ken Russell wrote:
> 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

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?

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:446: 'chartjson-output.json')
On 2016/09/17 03:33:46, Ken Russell wrote:
> 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

Done.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:757: + " will be unset.")
On 2016/09/17 03:33:46, Ken Russell wrote:
> 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.

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.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_failed_isolated_script_test.json
(left):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_failed_isolated_script_test.json:375:
"--isolated-script-test-output=${ISOLATED_OUTDIR}/output.json"
On 2016/09/17 03:33:46, Ken Russell wrote:
> The loss here of the --isolated-script-test-output flag is a big warning sign
> that something's wrong with this CL. See comment near the top of
> scripts/slave/recipe_modules/swarming/api.py.

Done.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_isolated_script_test_linux_gpu.json
(left):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_isolated_script_test_linux_gpu.json:381:
"--isolated-script-test-output=${ISOLATED_OUTDIR}/output.json"
On 2016/09/17 03:33:46, Ken Russell wrote:
> Same here.

Done.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_failed_isolated_script_test.json
(right):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/dynamic_swarmed_sharded_failed_isolated_script_test.json:430:
"@@@STEP_LOG_LINE@no_results_exc@'str' object has no attribute 'extend'@@@",
On 2016/09/17 03:33:46, Ken Russell wrote:
> This exception looks problematic. It shows up in a couple of other places
below
> too.

Done.

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.py (right):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.py:634: retcode=1)
On 2016/09/17 03:33:47, Ken Russell wrote:
> As far as I can tell the parameters for this fake data are the same as for the
> dynamic_swarmed_sharded_isolated_chartjson_test_missing_shard test, below. Can
> this test be removed without losing 100% code coverage? If so let's do that.

missing/empty shards are the difference in the parameters.  They are necessary
for the failure case and the failure to parse the json case.

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

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/40001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/swarming/api.py:422: return
extra_args.append(output_arg)
On 2016/09/19 17:56:16, eyaich1 wrote:
> On 2016/09/17 03:33:46, Ken Russell wrote:
> > 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
> 
> 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?

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.

[Ken Russell (switch to Gerrit), 2016-09-19 23:41:18]

kbr@chromium.org changed reviewers:
+ dpranke@chromium.org, phajdan.jr@chromium.org

[Ken Russell (switch to Gerrit), 2016-09-19 23:41:19]

LGTM

I'm not an OWNER of most of these files. Suggest dpranke@ or phajdan.jr@ to
approve. maruel@ and vadimsh@ are the closest owners of swarming/ but you'll
need one of the others to approve as well anyway.

https://codereview.chromium.org/2336293002/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/swarming/api.py:416: if bad_args:  # pragma: no
cover
It would be better to write a test for this code path -- is that not possible?

https://codereview.chromium.org/2336293002/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/test_utils/test_api.py (right):

https://codereview.chromium.org/2336293002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/test_utils/test_api.py:169: if not
chartjson_output_missing:
I think it would be more realistic to only synthesize the per-shard chartjson
files if output_chartjson is true. What do you think? That would avoid the
"outdir_json" expectation changes in the majority of the recipe simulation tests
(example:
scripts/slave/recipes/chromium.expected/dynamic_swarmed_isolated_script_test_linux_gpu.json
), making those easier to visually confirm.

[eyaich1, 2016-09-20 13:56:59]

https://codereview.chromium.org/2336293002/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/swarming/api.py:416: if bad_args:  # pragma: no
cover
On 2016/09/19 23:41:19, Ken Russell wrote:
> It would be better to write a test for this code path -- is that not possible?

So this was copy/pasted code so I didn't notice it before.  You can easily add a
test to simulate a bad flag, but what do you do with the raised error in the
test suite?  If you notice in this file there is other untested code for
exceptions being thrown and not caught, can the expectations suite handle
uncaught exceptions?  When I run the test suite it fails on the thrown
exception.

https://codereview.chromium.org/2336293002/diff/100001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/test_utils/test_api.py (right):

https://codereview.chromium.org/2336293002/diff/100001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/test_utils/test_api.py:169: if not
chartjson_output_missing:
On 2016/09/19 23:41:19, Ken Russell wrote:
> I think it would be more realistic to only synthesize the per-shard chartjson
> files if output_chartjson is true. What do you think? That would avoid the
> "outdir_json" expectation changes in the majority of the recipe simulation
tests
> (example:
>
scripts/slave/recipes/chromium.expected/dynamic_swarmed_isolated_script_test_linux_gpu.json
> ), making those easier to visually confirm.

Done.

[Paweł Hajdan Jr., 2016-09-20 17:35:48]

LGTM w/nits

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/swarming/api.py:418: raise ValueError(error)
nit: Why not just combine this with the previous statement? "raise
ValueError('--%s should not be used' % flag)"

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/swarming/api.py:440: extra_args =
self._check_and_set_output_flag( \
nit: "\" should not be needed.

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/test_utils/test_api.py (right):

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/test_utils/test_api.py:141:
chartjsonish_results['charts'] = {'entry%d' % idx: "chart%d" % idx, \
nit: Use single quotes, here and below. "\" not needed.

[eyaich1, 2016-09-20 17:43:20]

thanks!

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/swarming/api.py (right):

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/swarming/api.py:418: raise ValueError(error)
On 2016/09/20 17:35:47, Paweł Hajdan Jr. wrote:
> nit: Why not just combine this with the previous statement? "raise
> ValueError('--%s should not be used' % flag)"

Done.

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/swarming/api.py:440: extra_args =
self._check_and_set_output_flag( \
On 2016/09/20 17:35:47, Paweł Hajdan Jr. wrote:
> nit: "\" should not be needed.

Done.

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
File scripts/slave/recipe_modules/test_utils/test_api.py (right):

https://codereview.chromium.org/2336293002/diff/140001/scripts/slave/recipe_m...
scripts/slave/recipe_modules/test_utils/test_api.py:141:
chartjsonish_results['charts'] = {'entry%d' % idx: "chart%d" % idx, \
On 2016/09/20 17:35:48, Paweł Hajdan Jr. wrote:
> nit: Use single quotes, here and below. "\" not needed.

Done.

[eyaich1, 2016-09-20 17:43:34]

The CQ bit was checked by eyaich@chromium.org

[eyaich1, 2016-09-20 17:43:35]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2336293002/#ps160001
(title: "Responding to review comments")

[commit-bot: I haz the power, 2016-09-20 17:43:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 17:51:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 17:51:57]

Try jobs failed on following builders:
  Build Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/3161879305549e10)

[eyaich1, 2016-09-20 17:55:48]

The CQ bit was checked by eyaich@chromium.org

[eyaich1, 2016-09-20 17:55:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2336293002/#ps180001
(title: "Adding missing expectatoins")

[commit-bot: I haz the power, 2016-09-20 17:55:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 18:00:16]

Description was changed from

==========
Updating swarming api to add chartjson output. 

This updates the api to check for chartjson results from a finished task in
output directory with a file named chart-output.json.

BUG=chromium:633253
==========

to

==========
Updating swarming api to add chartjson output. 

This updates the api to check for chartjson results from a finished task in
output directory with a file named chart-output.json.

BUG=chromium:633253

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/99542aa3517f274e8f44...
==========

[commit-bot: I haz the power, 2016-09-20 18:00:17]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/tools/build/+/99542aa3517f274e8f44...

[jbudorick, 2016-09-20 19:55:15]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2352183002/ by jbudorick@chromium.org.

The reason for reverting is: I think this might have broken
telemetry_perf_unittests on linux_android_rel_ng (at least)..

[jbudorick, 2016-09-20 19:57:24]

On 2016/09/20 19:55:15, jbudorick wrote:
> A revert of this CL (patchset #10 id:180001) has been created in
> https://codereview.chromium.org/2352183002/ by mailto:jbudorick@chromium.org.
> 
> The reason for reverting is: I think this might have broken
> telemetry_perf_unittests on linux_android_rel_ng (at least)..

e.g. from
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

telemetry_perf_unittests succeeds:

Command: /usr/bin/python ../../testing/scripts/run_telemetry_as_googletest.py
../../tools/perf/run_tests -v --browser=android-chromium --device=android
--isolated-script-test-output=/b/swarm_slave/w/ioN4U0EC/output.json
--isolated-script-test-chartjson-output=/b/swarm_slave/w/ioN4U0EC/chartjson-output.json
+-----------------------------------------------------------------------------+
| End of shard 0  Pending: 534.1s  Duration: 469.7s  Bot: build90-b4  Exit: 0 |
+-----------------------------------------------------------------------------+

but then we fail to decode the json:

@@@STEP_LOG_LINE@no_results_exc@error decoding JSON results from shard #0@@@

and then fail w/ TEST RESULTS WERE INVALID

[Ken Russell (switch to Gerrit), 2016-09-21 00:28:58]

Description was changed from

==========
Updating swarming api to add chartjson output. 

This updates the api to check for chartjson results from a finished task in
output directory with a file named chart-output.json.

BUG=chromium:633253

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/99542aa3517f274e8f44...
==========

to

==========
Updating swarming api to add chartjson output. 

This updates the api to check for chartjson results from a finished task in
output directory with a file named chart-output.json.

BUG=chromium:633253

Committed:
https://chromium.googlesource.com/chromium/tools/build/+/99542aa3517f274e8f44...
==========

[Ken Russell (switch to Gerrit), 2016-09-21 00:29:58]

On 2016/09/20 19:57:24, jbudorick wrote:
> On 2016/09/20 19:55:15, jbudorick wrote:
> > A revert of this CL (patchset #10 id:180001) has been created in
> > https://codereview.chromium.org/2352183002/ by
mailto:jbudorick@chromium.org.
> > 
> > The reason for reverting is: I think this might have broken
> > telemetry_perf_unittests on linux_android_rel_ng (at least)..
> 
> e.g. from
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...
> 
> telemetry_perf_unittests succeeds:
> 
> Command: /usr/bin/python ../../testing/scripts/run_telemetry_as_googletest.py
> ../../tools/perf/run_tests -v --browser=android-chromium --device=android
> --isolated-script-test-output=/b/swarm_slave/w/ioN4U0EC/output.json
>
--isolated-script-test-chartjson-output=/b/swarm_slave/w/ioN4U0EC/chartjson-output.json
>
+-----------------------------------------------------------------------------+
> | End of shard 0  Pending: 534.1s  Duration: 469.7s  Bot: build90-b4  Exit: 0
|
>
+-----------------------------------------------------------------------------+
> 
> but then we fail to decode the json:
> 
> @@@STEP_LOG_LINE@no_results_exc@error decoding JSON results from shard #0@@@
> 
> and then fail w/ TEST RESULTS WERE INVALID

I dug into some of these logs but found it quite difficult to figure out what
was going wrong in the processing of the JSON results.

I'd suggest trying to run the recipe locally, though this can be somewhat
painful. See here for some hints:

https://www.chromium.org/developers/testing/chromium-build-infrastructure/rep...
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/tools...

It would be awesome if it were possible to filter the test steps that
run_recipe.py triggered so that you could do a build and run just one of the
test steps.

[eyaich1, 2016-09-21 12:17:48]

On 2016/09/21 00:29:58, Ken Russell wrote:
> On 2016/09/20 19:57:24, jbudorick wrote:
> > On 2016/09/20 19:55:15, jbudorick wrote:
> > > A revert of this CL (patchset #10 id:180001) has been created in
> > > https://codereview.chromium.org/2352183002/ by
> mailto:jbudorick@chromium.org.
> > > 
> > > The reason for reverting is: I think this might have broken
> > > telemetry_perf_unittests on linux_android_rel_ng (at least)..
> > 
> > e.g. from
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...
> > 
> > telemetry_perf_unittests succeeds:
> > 
> > Command: /usr/bin/python
../../testing/scripts/run_telemetry_as_googletest.py
> > ../../tools/perf/run_tests -v --browser=android-chromium --device=android
> > --isolated-script-test-output=/b/swarm_slave/w/ioN4U0EC/output.json
> >
>
--isolated-script-test-chartjson-output=/b/swarm_slave/w/ioN4U0EC/chartjson-output.json
> >
>
+-----------------------------------------------------------------------------+
> > | End of shard 0  Pending: 534.1s  Duration: 469.7s  Bot: build90-b4  Exit:
0
> |
> >
>
+-----------------------------------------------------------------------------+
> > 
> > but then we fail to decode the json:
> > 
> > @@@STEP_LOG_LINE@no_results_exc@error decoding JSON results from shard #0@@@
> > 
> > and then fail w/ TEST RESULTS WERE INVALID
> 
> I dug into some of these logs but found it quite difficult to figure out what
> was going wrong in the processing of the JSON results.
> 
> I'd suggest trying to run the recipe locally, though this can be somewhat
> painful. See here for some hints:
> 
>
https://www.chromium.org/developers/testing/chromium-build-infrastructure/rep...
>
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/tools...
> 
> It would be awesome if it were possible to filter the test steps that
> run_recipe.py triggered so that you could do a build and run just one of the
> test steps.

I am glad I am not the only one!  I was looking through last night and was
scratching my
head asthe json seems correct:

{
  "0/chartjson-output.json": "", 
  "0/output.json": "{\"failures\": [], \"valid\": true}", 
}

I will do a little more digging this morning and then maybe try and repro
locally like you suggested.

It appears like this was failing on both the android tryserver and the main
chromium.linux waterfall at least.

[seanmccullough1, 2016-09-22 21:06:15]

On 2016/09/21 12:17:48, eyaich1 wrote:
> On 2016/09/21 00:29:58, Ken Russell wrote:
> > On 2016/09/20 19:57:24, jbudorick wrote:
> > > On 2016/09/20 19:55:15, jbudorick wrote:
> > > > A revert of this CL (patchset #10 id:180001) has been created in
> > > > https://codereview.chromium.org/2352183002/ by
> > mailto:jbudorick@chromium.org.
> > > > 
> > > > The reason for reverting is: I think this might have broken
> > > > telemetry_perf_unittests on linux_android_rel_ng (at least)..
> > > 
> > > e.g. from
> > >
> >
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...
> > > 
> > > telemetry_perf_unittests succeeds:
> > > 
> > > Command: /usr/bin/python
> ../../testing/scripts/run_telemetry_as_googletest.py
> > > ../../tools/perf/run_tests -v --browser=android-chromium --device=android
> > > --isolated-script-test-output=/b/swarm_slave/w/ioN4U0EC/output.json
> > >
> >
>
--isolated-script-test-chartjson-output=/b/swarm_slave/w/ioN4U0EC/chartjson-output.json
> > >
> >
>
+-----------------------------------------------------------------------------+
> > > | End of shard 0  Pending: 534.1s  Duration: 469.7s  Bot: build90-b4 
Exit:
> 0
> > |
> > >
> >
>
+-----------------------------------------------------------------------------+
> > > 
> > > but then we fail to decode the json:
> > > 
> > > @@@STEP_LOG_LINE@no_results_exc@error decoding JSON results from shard
#0@@@
> > > 
> > > and then fail w/ TEST RESULTS WERE INVALID
> > 
> > I dug into some of these logs but found it quite difficult to figure out
what
> > was going wrong in the processing of the JSON results.
> > 
> > I'd suggest trying to run the recipe locally, though this can be somewhat
> > painful. See here for some hints:
> > 
> >
>
https://www.chromium.org/developers/testing/chromium-build-infrastructure/rep...
> >
>
https://chromium.googlesource.com/chromium/tools/build/+/master/scripts/tools...
> > 
> > It would be awesome if it were possible to filter the test steps that
> > run_recipe.py triggered so that you could do a build and run just one of the
> > test steps.
> 
> I am glad I am not the only one!  I was looking through last night and was
> scratching my
> head asthe json seems correct:
> 
> {
>   "0/chartjson-output.json": "", 
>   "0/output.json": "{\"failures\": [], \"valid\": true}", 
> }
> 
> I will do a little more digging this morning and then maybe try and repro
> locally like you suggested.
> 
> It appears like this was failing on both the android tryserver and the main
> chromium.linux waterfall at least.

Did this get re-submitted? This issue was re-opened recently:
https://bugs.chromium.org/p/chromium/issues/detail?id=648727