Making check_initial_confidence verify return_code bisects.

Making check_initial_confidence verify return_code bisects.

This is the code that performs the repro check. We had mistakenly
assumed that it only really aplied to perf bisects.

R=prasadv,qyearsley
BUG=596086
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=299453

[qyearsley, 2016-03-23 20:54:40]

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:492: # of the test, i.e. 0
if all runs returned 0, 1 otherwise.
mean_value is a potentially confusing or misleading name.

If we were to rename it (in a separate CL), what would it be? summary_value? Or
might mean_value for return code bisects be ignored, and instead set a separate
attribute called overall_return_code (or something similar)?

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector_test.py (right):

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:155: # When confidence
is not required, confidence_score should not be called.
This comment is now confusing since this test method doesn't reference use or
check confidence_score (but it checks that significantly_different is not called
-- which could again be a separate little test method).

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:164: 
This test method might be a bit clearer if it's split into separate methods that
each test one simple behavior/case: there could be one case where
check_initial_confidence() returns True and another where it returns False.

[RobertoCN, 2016-03-23 21:52:09]

Addressed feedback.

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector.py (right):

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector.py:492: # of the test, i.e. 0
if all runs returned 0, 1 otherwise.
On 2016/03/23 20:54:40, qyearsley wrote:
> mean_value is a potentially confusing or misleading name.
> 
> If we were to rename it (in a separate CL), what would it be? summary_value?
Or
> might mean_value for return code bisects be ignored, and instead set a
separate
> attribute called overall_return_code (or something similar)?

Created a new member of the revision class called overall_return_code.
Still need to refactor mean_value from the code that posts results back to
bisect.

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
File scripts/slave/recipe_modules/auto_bisect/bisector_test.py (right):

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:155: # When confidence
is not required, confidence_score should not be called.
On 2016/03/23 20:54:40, qyearsley wrote:
> This comment is now confusing since this test method doesn't reference use or
> check confidence_score (but it checks that significantly_different is not
called
> -- which could again be a separate little test method).


Removed the check for not calling significantly different for two reasons:
- Bypass repro check is no longer a use case (the parameter is ignored)
- Significantly_different has been modified to support return_code bisects.

Split the test for return code confidence check into pass and fail.

https://codereview.chromium.org/1825993003/diff/1/scripts/slave/recipe_module...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:164: 
On 2016/03/23 20:54:40, qyearsley wrote:
> This test method might be a bit clearer if it's split into separate methods
that
> each test one simple behavior/case: there could be one case where
> check_initial_confidence() returns True and another where it returns False.

Done.

[qyearsley, 2016-03-23 22:09:36]

Excellent, LGTM now, possibly first removing the @mock.patch decorators in those
two test methods if they're unnecessary now.

https://codereview.chromium.org/1825993003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/bisector_test.py (right):

https://codereview.chromium.org/1825993003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/bisector_test.py:163: mock.MagicMock())
There is no longer the line
self.assertFalse(bisector.significantly_different.called), so I believe that
this @mock.patch can be removed (same in
test_check_initial_confidence_return_code_pass)

https://codereview.chromium.org/1825993003/diff/20001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):

https://codereview.chromium.org/1825993003/diff/20001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/auto_bisect/revision_state.py:337: # to dashboard.
What do you think we should do specifically when uploading to the perf
dashboard? I see two main choices:

1. In Bisector._revision_data
(https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...),
send a field "value" or "overall_return_code" instead of "mean_value", and also
update the perf dashboard to deal with that. (I like this option a bit better)

2. In Bisector._revision_data, send either overall_return_code or mean_value,
but keep using the key "mean_value" so that the perf dashboard doesn't have to
be changed.

Do you agree?

[RobertoCN, 2016-03-23 22:16:29]

On 2016/03/23 22:09:36, qyearsley wrote:
> Excellent, LGTM now, possibly first removing the @mock.patch decorators in
those
> two test methods if they're unnecessary now.
> 
>
https://codereview.chromium.org/1825993003/diff/20001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/auto_bisect/bisector_test.py (right):
> 
>
https://codereview.chromium.org/1825993003/diff/20001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/auto_bisect/bisector_test.py:163:
mock.MagicMock())
> There is no longer the line
> self.assertFalse(bisector.significantly_different.called), so I believe that
> this @mock.patch can be removed (same in
> test_check_initial_confidence_return_code_pass)

Done.


> 
>
https://codereview.chromium.org/1825993003/diff/20001/scripts/slave/recipe_mo...
> File scripts/slave/recipe_modules/auto_bisect/revision_state.py (right):
> 
>
https://codereview.chromium.org/1825993003/diff/20001/scripts/slave/recipe_mo...
> scripts/slave/recipe_modules/auto_bisect/revision_state.py:337: # to
dashboard.
> What do you think we should do specifically when uploading to the perf
> dashboard? I see two main choices:
> 
> 1. In Bisector._revision_data
>
(https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...),
> send a field "value" or "overall_return_code" instead of "mean_value", and
also
> update the perf dashboard to deal with that. (I like this option a bit better)
> 
> 2. In Bisector._revision_data, send either overall_return_code or mean_value,
> but keep using the key "mean_value" so that the perf dashboard doesn't have to
> be changed.
> 
> Do you agree?

I like option 1. I am thinking that 'summary_value' is the better name for the
key.

[RobertoCN, 2016-03-23 22:16:47]

The CQ bit was checked by robertocn@chromium.org

[RobertoCN, 2016-03-23 22:16:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/1825993003/#ps40001
(title: "Removing unnecessary mocks.")

[commit-bot: I haz the power, 2016-03-23 22:16:51]

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

[commit-bot: I haz the power, 2016-03-23 22:20:48]

Description was changed from

==========
Making check_initial_confidence verify return_code bisects.

This is the code that performs the repro check. We had mistakenly
assumed that it only really aplied to perf bisects.

R=prasadv,qyearsley
BUG=596086
==========

to

==========
Making check_initial_confidence verify return_code bisects.

This is the code that performs the repro check. We had mistakenly
assumed that it only really aplied to perf bisects.

R=prasadv,qyearsley
BUG=596086

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

[commit-bot: I haz the power, 2016-03-23 22:20:50]

Committed patchset #3 (id:40001) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=299453