Updating perf recipe for disabled chartjson data.

Updating perf recipe for disabled chartjson data.

BUG=chromium:647714
Committed: https://chromium.googlesource.com/chromium/tools/build/+/b7b68b4c0396453d775c7fb924d45bbfdff6f349

[eyaich1, 2016-09-16 16:24:59]

Description was changed from

==========
asdf

BUG=
==========

to

==========
Updating perf recipe for disabled chartjson data.

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

[eyaich1, 2016-09-16 16:27:16]

eyaich@chromium.org changed reviewers:
+ dtu@chromium.org, perezju@chromium.org

[eyaich1, 2016-09-16 17:06:39]

Description was changed from

==========
Updating perf recipe for disabled chartjson data.

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

to

==========
Updating perf recipe for disabled chartjson data.

BUG=chromium:647714
==========

[perezju, 2016-09-22 08:50:27]

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/results_dashb...
File scripts/slave/results_dashboard.py (right):

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/results_dashb...
scripts/slave/results_dashboard.py:245: if chart_json['disabled']:
change this to chart_json.get('disabled', False), in case we get an old
chartjson (e.g. on release bots we sometimes run new recipe code with old
chromium/catapult repositories).

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/results_dashb...
scripts/slave/results_dashboard.py:246: return (True, chart_json)
I don't like this change. I think we still want the wrapping data with master,
versions, etc. (i.e. the "fields" dict below) to be shown on the waterfall, even
if the benchmark was disabled at the time.

I would suggest either:

1) copy the enabled/disabled value from the inner "chart_data" into the outer
"fields"; then test for results['enabled'] in _SendResultsToDashboard before
actually sending the data; or

2) don't change this function at all, and in _SendResultsToDashboard test for
results['chart_data'].get('enabled', False) before sending the data.

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/runtest.py
File scripts/slave/runtest.py (right):

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/runtest.py#ne...
scripts/slave/runtest.py:726: results, args['url'], args['build_dir'])
nit: two extra indent spaces

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/unittests/run...
File scripts/slave/unittests/runtest_test.py (right):

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/unittests/run...
scripts/slave/unittests/runtest_test.py:135: {'chart': {'traces': {'x': [1, 0]},
'rev': 1000}}
why this change?

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/unittests/run...
scripts/slave/unittests/runtest_test.py:170: self, SendResults,
MakeDashboardJsonV1):
Also add a test case showing that data is not sent if the benchmark was
disabled.

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/unittests/run...
scripts/slave/unittests/runtest_test.py:175: {'chart': {'traces': {'x': [1, 0]},
'rev': 1000}, 'disabled': False}
The style guide discourages the use of backslash for line continuation:
http://google.github.io/styleguide/pyguide.html#Line_length

Break the line after the first "{" instead.

[eyaich1, 2016-09-26 15:22:11]

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/results_dashb...
File scripts/slave/results_dashboard.py (right):

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/results_dashb...
scripts/slave/results_dashboard.py:245: if chart_json['disabled']:
On 2016/09/22 08:50:27, perezju wrote:
> change this to chart_json.get('disabled', False), in case we get an old
> chartjson (e.g. on release bots we sometimes run new recipe code with old
> chromium/catapult repositories).

Done.

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/results_dashb...
scripts/slave/results_dashboard.py:246: return (True, chart_json)
On 2016/09/22 08:50:27, perezju wrote:
> I don't like this change. I think we still want the wrapping data with master,
> versions, etc. (i.e. the "fields" dict below) to be shown on the waterfall,
even
> if the benchmark was disabled at the time.
> 
> I would suggest either:
> 
> 1) copy the enabled/disabled value from the inner "chart_data" into the outer
> "fields"; then test for results['enabled'] in _SendResultsToDashboard before
> actually sending the data; or
> 
> 2) don't change this function at all, and in _SendResultsToDashboard test for
> results['chart_data'].get('enabled', False) before sending the data.

I went with #2.  My original thinking was that I reuse this method in my new
swarming recipe in https://codereview.chromium.org/2330133002/ so I was trying
to do the disabled check only once, but this makes more sense.  Hopefully the
code in runtests.py will be going away soon once the perf dashboard is swarmed
:)

Done.

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/runtest.py
File scripts/slave/runtest.py (right):

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/runtest.py#ne...
scripts/slave/runtest.py:726: results, args['url'], args['build_dir'])
On 2016/09/22 08:50:27, perezju wrote:
> nit: two extra indent spaces

Done.

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/unittests/run...
File scripts/slave/unittests/runtest_test.py (right):

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/unittests/run...
scripts/slave/unittests/runtest_test.py:135: {'chart': {'traces': {'x': [1, 0]},
'rev': 1000}}
On 2016/09/22 08:50:27, perezju wrote:
> why this change?

Not sure. Done.

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/unittests/run...
scripts/slave/unittests/runtest_test.py:170: self, SendResults,
MakeDashboardJsonV1):
On 2016/09/22 08:50:27, perezju wrote:
> Also add a test case showing that data is not sent if the benchmark was
> disabled.

Done.

https://codereview.chromium.org/2346963003/diff/1/scripts/slave/unittests/run...
scripts/slave/unittests/runtest_test.py:175: {'chart': {'traces': {'x': [1, 0]},
'rev': 1000}, 'disabled': False}
On 2016/09/22 08:50:27, perezju wrote:
> The style guide discourages the use of backslash for line continuation:
> http://google.github.io/styleguide/pyguide.html#Line_length
> 
> Break the line after the first "{" instead.

Done.

[perezju, 2016-09-26 15:48:06]

non-owner lgtm w/nits

https://codereview.chromium.org/2346963003/diff/20001/scripts/slave/runtest.py
File scripts/slave/runtest.py (right):

https://codereview.chromium.org/2346963003/diff/20001/scripts/slave/runtest.p...
scripts/slave/runtest.py:715: return True
suggestion, maybe just keep the variable name as |results|, and then the change
needed is just:

  if results and not results['chart_data'].get('enabled', True):
    return True  # A successful run, but the benchmark was disabled.

https://codereview.chromium.org/2346963003/diff/20001/scripts/slave/unittests...
File scripts/slave/unittests/runtest_test.py (right):

https://codereview.chromium.org/2346963003/diff/20001/scripts/slave/unittests...
scripts/slave/unittests/runtest_test.py:174: 'enabled': True}
nit: indent is wrong, "stuff on first line forbidden"

http://google.github.io/styleguide/pyguide.html?showone=Indentation#Indentation

[eyaich1, 2016-09-26 16:28:29]

https://codereview.chromium.org/2346963003/diff/20001/scripts/slave/runtest.py
File scripts/slave/runtest.py (right):

https://codereview.chromium.org/2346963003/diff/20001/scripts/slave/runtest.p...
scripts/slave/runtest.py:715: return True
On 2016/09/26 15:48:06, perezju wrote:
> suggestion, maybe just keep the variable name as |results|, and then the
change
> needed is just:
> 
>   if results and not results['chart_data'].get('enabled', True):
>     return True  # A successful run, but the benchmark was disabled.

Done.

https://codereview.chromium.org/2346963003/diff/20001/scripts/slave/unittests...
File scripts/slave/unittests/runtest_test.py (right):

https://codereview.chromium.org/2346963003/diff/20001/scripts/slave/unittests...
scripts/slave/unittests/runtest_test.py:174: 'enabled': True}
On 2016/09/26 15:48:06, perezju wrote:
> nit: indent is wrong, "stuff on first line forbidden"
> 
>
http://google.github.io/styleguide/pyguide.html?showone=Indentation#Indentation

Done.

[dtu, 2016-09-26 17:33:32]

lgtm

[eyaich1, 2016-09-26 17:34:11]

eyaich@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[eyaich1, 2016-09-26 17:34:11]

+phajdan for owners approval

[Paweł Hajdan Jr., 2016-09-26 21:41:32]

LGTM

[eyaich1, 2016-09-27 13:12:16]

The CQ bit was checked by eyaich@chromium.org

[eyaich1, 2016-09-27 13:12:17]

The patchset sent to the CQ was uploaded after l-g-t-m from perezju@chromium.org
Link to the patchset: https://codereview.chromium.org/2346963003/#ps40001
(title: "responding to review comments")

[commit-bot: I haz the power, 2016-09-27 13:12:23]

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-27 13:16:40]

Description was changed from

==========
Updating perf recipe for disabled chartjson data.

BUG=chromium:647714
==========

to

==========
Updating perf recipe for disabled chartjson data.

BUG=chromium:647714

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

[commit-bot: I haz the power, 2016-09-27 13:16:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/tools/build/+/b7b68b4c0396453d775c...