Allow expectations for different graphs within tests....

Allow expectations for different graphs within tests.

Only load perf expectations if the 'loaded' key has a
true value.

Change the format required of the perf_expectations.json
file to use a simpler layout and support the
graph/result type after the slave/test specifier.

BUG=18597
TEST=regression/improvements appear in local tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25403

[chase, 2009-09-03 09:16:36]

This change allows us to set perf expectations based not just on slave, test,
and result type, but also on the specific test's graph name.

Here are some examples of the new key format in perf_expectations:

  linux-release-hardy/startup/warm/t
  linux-release-hardy/morejs/times/t
  linux-release-hardy/new-tab-ui-cold/new_tab/tab_cold

[M-A Ruel, 2009-09-03 15:13:03]

few minor changes

http://codereview.chromium.org/199005/diff/1/2
File log_parser/process_log.py (right):

http://codereview.chromium.org/199005/diff/1/2#newcode122
Line 122: self._expected_performance = None
you should set it to {} and remove line 148-149.

http://codereview.chromium.org/199005/diff/1/2#newcode150
Line 150: if not self._expected_performance.has_key(graph_name):
this is what you want:
self._expected_performance.setdefault(graph_name, {})

http://codereview.chromium.org/199005/diff/1/2#newcode199
Line 199: if (not self._expected_performance.has_key(graph_name) or
FYI, you can also use the format
if (not graph_name in self._expected_performance or ...):

that doesn't matter much.

http://codereview.chromium.org/199005/diff/1/2#newcode204
Line 204: if not self._actual_performance[graph_name][result_type]['delta']:
that could throw an exception.

http://codereview.chromium.org/199005/diff/1/2#newcode231
Line 231: return
??

http://codereview.chromium.org/199005/diff/1/2#newcode243
Line 243: for result_type in self._actual_performance[graph_name].keys():
"keys() returns a copy of the dictionary’s list of keys."

use:
for result_type in self._actual_performance[graph_name]:

http://codereview.chromium.org/199005/diff/1/2#newcode582
Line 582: if not self._actual_performance.has_key(graph_name):
setdefault

[chase, 2009-09-03 19:10:50]

http://codereview.chromium.org/199005/diff/1/2
File log_parser/process_log.py (right):

http://codereview.chromium.org/199005/diff/1/2#newcode122
Line 122: self._expected_performance = None
On 2009/09/03 15:13:03, Marc-Antoine Ruel wrote:
> you should set it to {} and remove line 148-149.

Done.

http://codereview.chromium.org/199005/diff/1/2#newcode150
Line 150: if not self._expected_performance.has_key(graph_name):
On 2009/09/03 15:13:03, Marc-Antoine Ruel wrote:
> this is what you want:
> self._expected_performance.setdefault(graph_name, {})

Done.

http://codereview.chromium.org/199005/diff/1/2#newcode199
Line 199: if (not self._expected_performance.has_key(graph_name) or
On 2009/09/03 15:13:03, Marc-Antoine Ruel wrote:
> FYI, you can also use the format
> if (not graph_name in self._expected_performance or ...):
> 
> that doesn't matter much.

Done.

http://codereview.chromium.org/199005/diff/1/2#newcode204
Line 204: if not self._actual_performance[graph_name][result_type]['delta']:
On 2009/09/03 15:13:03, Marc-Antoine Ruel wrote:
> that could throw an exception.

Thanks, done.

http://codereview.chromium.org/199005/diff/1/2#newcode231
Line 231: return
On 2009/09/03 15:13:03, Marc-Antoine Ruel wrote:
> ??

Done.

http://codereview.chromium.org/199005/diff/1/2#newcode243
Line 243: for result_type in self._actual_performance[graph_name].keys():
On 2009/09/03 15:13:03, Marc-Antoine Ruel wrote:
> "keys() returns a copy of the dictionary’s list of keys."
> 
> use:
> for result_type in self._actual_performance[graph_name]:

Done.

http://codereview.chromium.org/199005/diff/1/2#newcode582
Line 582: if not self._actual_performance.has_key(graph_name):
On 2009/09/03 15:13:03, Marc-Antoine Ruel wrote:
> setdefault

Done.

[M-A Ruel, 2009-09-03 19:34:33]

still lgtm

http://codereview.chromium.org/199005/diff/5001/5002
File log_parser/process_log.py (right):

http://codereview.chromium.org/199005/diff/5001/5002#newcode147
Line 147: self._expected_performance.setdefault(graph_name, {})
FYI, another trick but that would probably make the code unreadable.
self._expected_performance.setdefault(graph_name, {}).setdefault(result_type,
{})

http://codereview.chromium.org/199005/diff/5001/5002#newcode149
Line 149: self._expected_performance[graph_name][result_type] =
perf_data[perf_key]
I don't understand the two previous lines since in line 149 you defeat line 148.

http://codereview.chromium.org/199005/diff/5001/5002#newcode174
Line 174: # Set actual performance data when we come across useful values.
Please use docstring instead.

http://codereview.chromium.org/199005/diff/5001/5002#newcode187
Line 187: 
nit: I like dense stuff. I wouldn't put an empty line before conditions or other
statements.

http://codereview.chromium.org/199005/diff/5001/5002#newcode246
Line 246: raise Exception, msg
raise Exception(msg)

[Nicolas Sylvain, 2009-09-03 19:58:18]

LGTM if it works ;)

http://codereview.chromium.org/199005/diff/5001/5002
File log_parser/process_log.py (right):

http://codereview.chromium.org/199005/diff/5001/5002#newcode195
Line 195: if m: # a test on the reference build
2 spaces before the comment would be nice

[chase, 2009-09-03 23:20:35]

Making sure to send these even though the patch is done.  Thanks.

http://codereview.chromium.org/199005/diff/5001/5002
File log_parser/process_log.py (right):

http://codereview.chromium.org/199005/diff/5001/5002#newcode147
Line 147: self._expected_performance.setdefault(graph_name, {})
Good to know, I agree makes it less readable so I'll stick with the current
code.

http://codereview.chromium.org/199005/diff/5001/5002#newcode149
Line 149: self._expected_performance[graph_name][result_type] =
perf_data[perf_key]
On 2009/09/03 19:34:34, Marc-Antoine Ruel wrote:
> I don't understand the two previous lines since in line 149 you defeat line
148.

Thanks, fixed.

http://codereview.chromium.org/199005/diff/5001/5002#newcode174
Line 174: # Set actual performance data when we come across useful values.
On 2009/09/03 19:34:34, Marc-Antoine Ruel wrote:
> Please use docstring instead.

Done.

http://codereview.chromium.org/199005/diff/5001/5002#newcode187
Line 187: 
On 2009/09/03 19:34:34, Marc-Antoine Ruel wrote:
> nit: I like dense stuff. I wouldn't put an empty line before conditions or
other
> statements.

Done.

http://codereview.chromium.org/199005/diff/5001/5002#newcode195
Line 195: if m: # a test on the reference build
On 2009/09/03 19:58:18, Nicolas Sylvain wrote:
> 2 spaces before the comment would be nice

Done.

http://codereview.chromium.org/199005/diff/5001/5002#newcode246
Line 246: raise Exception, msg
On 2009/09/03 19:34:34, Marc-Antoine Ruel wrote:
> raise Exception(msg)

Done.

[M-A Ruel, 2009-09-03 23:43:41]

last post commit note, feel free to ignore

http://codereview.chromium.org/199005/diff/4003/4004
File log_parser/process_log.py (right):

http://codereview.chromium.org/199005/diff/4003/4004#newcode238
Line 238: if not 'delta' in expected or not 'var' in expected:
nit: Why do you care at all if you want to throw an exception anyway?

[M-A Ruel, 2009-09-04 00:02:38]

No, it's ripping lines 236 to 246. The exception will be thrown at
line 253 anyway.

I don't really mind, it's just my philosophy of "less is better" :)

M-A

On Thu, Sep 3, 2009 at 7:56 PM, <chase@chromium.org> wrote:
>
> http://codereview.chromium.org/199005/diff/4003/4004
> File log_parser/process_log.py (right):
>
> http://codereview.chromium.org/199005/diff/4003/4004#newcode238
> Line 238: if not 'delta' in expected or not 'var' in expected:
> On 2009/09/03 23:43:41, Marc-Antoine Ruel wrote:
>>
>> nit: Why do you care at all if you want to throw an exception anyway?
>
> I can imagine restructuring this so I only throw the exception when
> missing isn't empty below and get rid of the conditional on line 238.
> Is that your nit?
>
> http://codereview.chromium.org/199005
>

[chase, 2009-09-04 00:23:39]

On 2009/09/04 00:02:38, Marc-Antoine Ruel wrote:
> No, it's ripping lines 236 to 246. The exception will be thrown at
> line 253 anyway.
> 
> I don't really mind, it's just my philosophy of "less is better" :)

Ah, thanks for clarifying and thanks for the reviews.

I will be adding more checks to the presubmit code for the
perf_expectations.json file to ensure someone doesn't check in with missing
'var' and 'delta' keys.  Still, if someone bypasses the commit tests, the slave
failing with an exception in the perf step "name 'delta' is not defined" is not
the clear error I want to show them.  This exception makes the error clearer.  I
plan to leave it as-is for now.  Let me know if you think I should reconsider.