Fix style in tools/auto_bisect.

Fix style in tools/auto_bisect.

BUG=

Committed: https://crrev.com/6cd2352b28e8832d1276df86de7ad9fa483de1ad
Cr-Commit-Position: refs/heads/master@{#321839}

[qyearsley, 2015-03-12 20:30:53]

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

[qyearsley, 2015-03-12 20:30:53]

Looks like pylint isn't catching all of the style issues that I wish it would
catch.

Main thing worth noting is the rules about indentation and alignment: one should
either use "aligned with first arg" indentation", or "hanging indentation"
(https://www.python.org/dev/peps/pep-0008/#indentation).

Also, the has_key method of dict is technically still OK, but it's deprecated in
Python 3 (https://docs.python.org/3.1/whatsnew/3.0.html#builtins).

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

https://codereview.chromium.org/1001033004/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:407:
self.assertFalse(self._CheckAbortsEarly(MULTIPLE_VALUES))
The incorrect names of these methods was an error I accidentally introduced in
https://codereview.chromium.org/764733005.

[RobertoCN, 2015-03-12 20:57:04]

LGTM

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

https://codereview.chromium.org/1001033004/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression.py:2338: # Marking the 'bad' revision
as good BECAUSE XXX ...
because if there is no confidence that there's a regression, the revision given
as 'bad' is likely within noise range of what is considered good.

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

https://codereview.chromium.org/1001033004/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:118: def _MockRunTests(*args,
**kwargs):  # pylint: disable=unused-argument
Nifty trick

https://codereview.chromium.org/1001033004/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression_test.py:407:
self.assertFalse(self._CheckAbortsEarly(MULTIPLE_VALUES))
On 2015/03/12 20:30:53, qyearsley wrote:
> The incorrect names of these methods was an error I accidentally introduced in
> https://codereview.chromium.org/764733005.

Acknowledged.

[qyearsley, 2015-03-12 21:09:04]

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

https://codereview.chromium.org/1001033004/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression.py:2338: # Marking the 'bad' revision
as good BECAUSE XXX ...
On 2015/03/12 20:57:03, robertocn wrote:
> because if there is no confidence that there's a regression, the revision
given
> as 'bad' is likely within noise range of what is considered good.

Ah, thanks for catching this, I meant to expand on the comment or remove the
comment.

I was wondering whether the passed attribute of the bad revision RevisionState
is specially checked later on, like in bisect_results or bisect_printer? Or
whether it would cause the output to be confusing in some way if this wasn't set
to True?

(If this statement isn't necessary then I'd like to remove it...)

[qyearsley, 2015-03-13 20:07:10]

I tried removing the that line and the tests failed; since this is meant to be
just a style CL, I think I'll just leave it as is for now.

[qyearsley, 2015-03-13 20:07:13]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2015-03-13 20:07:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertocn@chromium.org
Link to the patchset: https://codereview.chromium.org/1001033004/#ps20001
(title: "Update comment above bad_revision_state.passed = True.")

[commit-bot: I haz the power, 2015-03-13 20:07:46]

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

[commit-bot: I haz the power, 2015-03-13 20:07:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-13 20:07:49]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[qyearsley, 2015-03-13 20:13:09]

Need another LGTM now; Prasad, could you review?

[RobertoCN, 2015-03-13 22:46:17]

I forgot to submit this comment.

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

https://codereview.chromium.org/1001033004/diff/1/tools/auto_bisect/bisect_pe...
tools/auto_bisect/bisect_perf_regression.py:2338: # Marking the 'bad' revision
as good BECAUSE XXX ...
On 2015/03/12 21:09:03, qyearsley wrote:
> On 2015/03/12 20:57:03, robertocn wrote:
> > because if there is no confidence that there's a regression, the revision
> given
> > as 'bad' is likely within noise range of what is considered good.
> 
> Ah, thanks for catching this, I meant to expand on the comment or remove the
> comment.
> 
> I was wondering whether the passed attribute of the bad revision RevisionState
> is specially checked later on, like in bisect_results or bisect_printer? Or
> whether it would cause the output to be confusing in some way if this wasn't
set
> to True?
> 
> (If this statement isn't necessary then I'd like to remove it...)

I don't remember there being any real need for this. I probably marked it as
good to prevent the culprit finding function from accidentally locating one
(IIRC my thoughts were: if the bisect is given the range of 2 adjacent revisions
and there's no confidence, none of them should be considered bad, lest the
results printer or some other component find a false culprit)

[qyearsley, 2015-03-14 01:02:23]

On 2015/03/13 22:46:17, robertocn wrote:
> I forgot to submit this comment.
> 
>
https://codereview.chromium.org/1001033004/diff/1/tools/auto_bisect/bisect_pe...
> File tools/auto_bisect/bisect_perf_regression.py (right):
> 
>
https://codereview.chromium.org/1001033004/diff/1/tools/auto_bisect/bisect_pe...
> tools/auto_bisect/bisect_perf_regression.py:2338: # Marking the 'bad' revision
> as good BECAUSE XXX ...
> On 2015/03/12 21:09:03, qyearsley wrote:
> > On 2015/03/12 20:57:03, robertocn wrote:
> > > because if there is no confidence that there's a regression, the revision
> > given
> > > as 'bad' is likely within noise range of what is considered good.
> > 
> > Ah, thanks for catching this, I meant to expand on the comment or remove the
> > comment.
> > 
> > I was wondering whether the passed attribute of the bad revision
RevisionState
> > is specially checked later on, like in bisect_results or bisect_printer? Or
> > whether it would cause the output to be confusing in some way if this wasn't
> set
> > to True?
> > 
> > (If this statement isn't necessary then I'd like to remove it...)
> 
> I don't remember there being any real need for this. I probably marked it as
> good to prevent the culprit finding function from accidentally locating one
> (IIRC my thoughts were: if the bisect is given the range of 2 adjacent
revisions
> and there's no confidence, none of them should be considered bad, lest the
> results printer or some other component find a false culprit)

Alright, that makes sense. I'm also not sure whether it should be removed, but
because this CL is not meant to introduce any functional changes, I'll just add
a comment for now.

[qyearsley, 2015-03-23 01:58:02]

Still waiting on committer LGTM before submitting this patch.

Prasad, could you take a look?

[prasadv, 2015-03-23 19:19:05]

lgtm

[qyearsley, 2015-03-23 20:22:00]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2015-03-23 20:22:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertocn@chromium.org
Link to the patchset: https://codereview.chromium.org/1001033004/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2015-03-23 20:23:06]

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

[commit-bot: I haz the power, 2015-03-23 20:47:03]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-23 20:47:37]

Patchset 4 (id:??) landed as
https://crrev.com/6cd2352b28e8832d1276df86de7ad9fa483de1ad
Cr-Commit-Position: refs/heads/master@{#321839}