Implementing direction_of_improvement

Implementing direction_of_improvement

BUG=418690
Committed: https://crrev.com/fdd5b1c0c6a4ec50137f7b5a2c6b7c02ae2149bc
Cr-Commit-Position: refs/heads/master@{#300167}

[RobertoCN, 2014-10-11 17:34:49]

robertocn@chromium.org changed reviewers:
+ prasadv@chromium.org, qyearsley@chromium.org

[RobertoCN, 2014-10-11 17:49:15]

Implementing the bisect script side of this first. Dashboard changes will come
later.

[qyearsley, 2014-10-12 03:37:12]

Hey, this looks good.

What do you think about calling it "improvement_direction" instead of
"direction_of_improvement"? In the dashboard code "improvement_direction" is
used.

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:1670: # passed.
In this comment, it might be clearer to say "set" rather than "passed".

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:2455: # flag is non-false.
What can it be if its non-false? It can be -1 or 1, right? Maybe say non-zero
instead of non-false?

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:308: """
Style nit: In general we've been putting the closing """ on the first line if
the docstring is only one line.

[RobertoCN, 2014-10-15 18:44:44]

Applied all suggestions.

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:1670: # passed.
On 2014/10/12 03:37:11, qyearsley wrote:
> In this comment, it might be clearer to say "set" rather than "passed".

Done.

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:2455: # flag is non-false.
On 2014/10/12 03:37:11, qyearsley wrote:
> What can it be if its non-false? It can be -1 or 1, right? Maybe say non-zero
> instead of non-false?

Done.

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/645263002/diff/70001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression_test.py:308: """
On 2014/10/12 03:37:11, qyearsley wrote:
> Style nit: In general we've been putting the closing """ on the first line if
> the docstring is only one line.

Done.

[qyearsley, 2014-10-15 20:33:00]

lgtm

Alright, so we're only expecting the behavior to be different if
"improvement_direction" is given and the actual direction is different from the
expected.

Maybe it's worth verifying by running a try job (maybe take the config for a
recent successful bisect run at
build.chromium.org/p/tryserver.chromium.perf/builders and add in an
improvement_direction field)?

[RobertoCN, 2014-10-17 01:04:33]

On 2014/10/15 20:33:00, qyearsley wrote:
> lgtm
> 
> Alright, so we're only expecting the behavior to be different if
> "improvement_direction" is given and the actual direction is different from
the
> expected.
> 
> Maybe it's worth verifying by running a try job (maybe take the config for a
> recent successful bisect run at
> build.chromium.org/p/tryserver.chromium.perf/builders and add in an
> improvement_direction field)?

Did that. Fixed discovered errors.

[qyearsley, 2014-10-17 21:20:14]

On 2014/10/17 01:04:33, robertocn wrote:
> On 2014/10/15 20:33:00, qyearsley wrote:
> > lgtm
> > 
> > Alright, so we're only expecting the behavior to be different if
> > "improvement_direction" is given and the actual direction is different from
> the
> > expected.
> > 
> > Maybe it's worth verifying by running a try job (maybe take the config for a
> > recent successful bisect run at
> > build.chromium.org/p/tryserver.chromium.perf/builders and add in an
> > improvement_direction field)?
> 
> Did that. Fixed discovered errors.

Alright, new changes lgtm.

[RobertoCN, 2014-10-17 21:25:37]

The CQ bit was checked by robertocn@chromium.org

[commit-bot: I haz the power, 2014-10-17 21:27:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/645263002/160001

[commit-bot: I haz the power, 2014-10-17 21:55:42]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2014-10-17 21:56:25]

Patchset 5 (id:??) landed as
https://crrev.com/fdd5b1c0c6a4ec50137f7b5a2c6b7c02ae2149bc
Cr-Commit-Position: refs/heads/master@{#300167}