Make bisect to abort early when the return codes for known good and known bad revisions are same.

Make bisect to abort early when the return codes for known good and known bad revisions are same.

For bisect mode = return_code, there is no point in bisecting revision which returned same return code,
in such cases we abort bisect early.

BUG=502433
Committed: https://crrev.com/a98c2e971a8a9b6326bc7e92a9c10a4f0a76d15c
Cr-Commit-Position: refs/heads/master@{#335778}

[prasadv, 2015-06-23 20:08:40]

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

[qyearsley, 2015-06-23 20:47:45]

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression.py:2314: # Early abort bisection when
the return_codes for known good
Phrasing nits:
 "Early abort bisection" -> "Abort bisect early"
 "return_codes" -> "return codes"

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:114: # that use _MockRunTests.
This comment should be updated.

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:117: def
_MakeMockRunTests(is_return_code=False):
Optional: Maybe it would be clearer if this argument was called
bisect_mode_is_return_code, although this is a bit long (so I'm not completely
sure whether it's better).

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:137: all(current_value == 0 for
current_value in values)) else 1
This would fit on one line if you shortened the variable name:

mean = 0 if all(v == 0 for v in values) else 1

Normally shorter variable names make it less readable, but in this case since
the variable scope is limited to only one line, I think it's OK.

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:449: self.assertTrue
(self._CheckAbortsEarlyForReturnCode([[0,0,0], [0,0,0]]))
Nit: space after assertTrue

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pr...
File tools/auto_bisect/bisect_printer.py (right):

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pr...
tools/auto_bisect/bisect_printer.py:134: if self.opts.metric else 'NOT
SPECIFIED'),
This change could also potentially go in a separate CL.

[RobertoCN, 2015-06-23 21:03:03]

Save for what Quinten said and this one issue, this looks good.

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:120: def _MockRunTests(*args,
**kwargs):
I think we only need one of these two functions.

[prasadv, 2015-06-23 21:37:49]

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
File tools/auto_bisect/bisect_perf_regression.py (right):

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression.py:2314: # Early abort bisection when
the return_codes for known good
On 2015/06/23 20:47:45, qyearsley wrote:
> Phrasing nits:
>  "Early abort bisection" -> "Abort bisect early"
>  "return_codes" -> "return codes"

Done.

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:114: # that use _MockRunTests.
On 2015/06/23 20:47:45, qyearsley wrote:
> This comment should be updated.

Now used by only _MockRunTests

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:117: def
_MakeMockRunTests(is_return_code=False):
On 2015/06/23 20:47:45, qyearsley wrote:
> Optional: Maybe it would be clearer if this argument was called
> bisect_mode_is_return_code, although this is a bit long (so I'm not completely
> sure whether it's better).

Done.

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:120: def _MockRunTests(*args,
**kwargs):
On 2015/06/23 21:03:03, RobertoCN wrote:
> I think we only need one of these two functions.

Done.

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:137: all(current_value == 0 for
current_value in values)) else 1
On 2015/06/23 20:47:45, qyearsley wrote:
> This would fit on one line if you shortened the variable name:
> 
> mean = 0 if all(v == 0 for v in values) else 1
> 
> Normally shorter variable names make it less readable, but in this case since
> the variable scope is limited to only one line, I think it's OK.

Done.

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:449: self.assertTrue
(self._CheckAbortsEarlyForReturnCode([[0,0,0], [0,0,0]]))
On 2015/06/23 20:47:45, qyearsley wrote:
> Nit: space after assertTrue

Done.

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pr...
File tools/auto_bisect/bisect_printer.py (right):

https://codereview.chromium.org/1205663002/diff/1/tools/auto_bisect/bisect_pr...
tools/auto_bisect/bisect_printer.py:134: if self.opts.metric else 'NOT
SPECIFIED'),
On 2015/06/23 20:47:45, qyearsley wrote:
> This change could also potentially go in a separate CL.

I think this should be a part of this CL itself

[qyearsley, 2015-06-23 21:46:15]

lgtm

https://codereview.chromium.org/1205663002/diff/40001/tools/auto_bisect/bisec...
File tools/auto_bisect/bisect_perf_regression_test.py (right):

https://codereview.chromium.org/1205663002/diff/40001/tools/auto_bisect/bisec...
tools/auto_bisect/bisect_perf_regression_test.py:118: def _MockRunTests(*args,
**kwargs):  # pylint: disable=unused-argument
Nit: Since this is defined inside of another function, it's not directly
accessible to other functions in this module, so the underscore prefix is
optional (it could also be called MockRunTests).

[chromium-reviews, 2015-06-23 22:06:00]

Quinten got it all.. LGTM.

On Tue, Jun 23, 2015 at 2:46 PM, <qyearsley@chromium.org> wrote:

> lgtm
>
>
>
>
https://codereview.chromium.org/1205663002/diff/40001/tools/auto_bisect/bisec...
> File tools/auto_bisect/bisect_perf_regression_test.py (right):
>
>
>
https://codereview.chromium.org/1205663002/diff/40001/tools/auto_bisect/bisec...
> tools/auto_bisect/bisect_perf_regression_test.py:118: def
> _MockRunTests(*args, **kwargs):  # pylint: disable=unused-argument
> Nit: Since this is defined inside of another function, it's not directly
> accessible to other functions in this module, so the underscore prefix
> is optional (it could also be called MockRunTests).
>
> https://codereview.chromium.org/1205663002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[anantha, 2015-06-23 22:07:54]

lgtm

[prasadv, 2015-06-23 22:13:36]

The CQ bit was checked by prasadv@chromium.org

[commit-bot: I haz the power, 2015-06-23 22:14:38]

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

[commit-bot: I haz the power, 2015-06-23 22:27:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-23 22:29:00]

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