Make tools/bisect_builds.py handle reversed bad/good revision ranges

Make tools/bisect_builds.py handle reversed bad/good revision ranges

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=165781

[enne (OOO), 2012-10-24 22:38:08]

Not sure who wants to look at this, so CCing folks on the blame list.

Also, it looks like bisect-builds.py has no tests? :(

[enne (OOO), 2012-10-29 17:15:04]

Putting thakis on the review line, since nobody volunteered.

[Nico, 2012-11-01 19:06:28]

I think this is a reasonable feature to implement, and your implementation looks
good. A small number of minor comments:

Good point about the tests. To get this started, can you add a test file and add
at least one test, however silly, to it?

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py#newcode145
tools/bisect-builds.py:145: numbers. The range starts with self.good_revision
and goes until
It looks like the 2nd sentence in the comment is no longer true. Can you remove
it?

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py#newcode453
tools/bisect-builds.py:453: min_str, max_str = ["good", "bad"][::1 - 2 *
(bad_rev < good_rev)]
What is this, perl? Just use an if and 4 lines.

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py#newcode673
tools/bisect-builds.py:673: print '  ' + WEBKIT_CHANGELOG_URL % (max_webkit_rev,
min_webkit_rev)
Should this be printed backwards too?

[enne (OOO), 2012-11-01 19:20:32]

Yeah, I can add a test.

Would you be opposed to renaming bisect-builds.py to bisect_builds.py so I can
import it into a test file? Or should I move all the functionality to
tools/python/bisect.py and have tools/bisect-builds.py call it so the name stays
the same?

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py#newcode453
tools/bisect-builds.py:453: min_str, max_str = ["good", "bad"][::1 - 2 *
(bad_rev < good_rev)]
On 2012/11/01 19:06:28, Nico wrote:
> What is this, perl? Just use an if and 4 lines.

Haha.  Will do.

[Nico, 2012-11-01 19:23:35]

On Thu, Nov 1, 2012 at 12:20 PM,  <enne@chromium.org> wrote:
> Yeah, I can add a test.
>
> Would you be opposed to renaming bisect-builds.py to bisect_builds.py so I
> can
> import it into a test file?

Yes, see the comments on
https://chromiumcodereview.appspot.com/10939023/ for the reasoning
(and also for a workaround: bisect_builds =
__import__('bisect-builds'))

> Or should I move all the functionality to
> tools/python/bisect.py and have tools/bisect-builds.py call it so the name
> stays
> the same?
>
>
>
> http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py
> File tools/bisect-builds.py (right):
>
>
http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py#newcode453
> tools/bisect-builds.py:453: min_str, max_str = ["good", "bad"][::1 - 2 *
> (bad_rev < good_rev)]
> On 2012/11/01 19:06:28, Nico wrote:
>>
>> What is this, perl? Just use an if and 4 lines.
>
>
> Haha.  Will do.
>
> http://codereview.chromium.org/11266025/

[enne (OOO), 2012-11-02 18:17:14]

Ah, thanks for the link.  That argument makes a lot of sense.

Sadly, testing anything useful here requires a good bit of refactoring. Are you
ok with the test just monkeypatching some problematic functions so I can
actually test the Bisect function's behavior?

[Nico, 2012-11-02 20:29:55]

On 2012/11/02 18:17:14, enne wrote:
> Ah, thanks for the link.  That argument makes a lot of sense.
> 
> Sadly, testing anything useful here requires a good bit of refactoring. Are
you
> ok with the test just monkeypatching some problematic functions so I can
> actually test the Bisect function's behavior?

Sounds good. I'm also happy if you just add a mostly empty test. This is just to
get tests on this file started – asking you to get this file from 0 to 80%
coverage doesn't sound fair. And if the test file exist, it'll make adding tests
on the next change look a lot less challenging I think.

[enne (OOO), 2012-11-02 22:26:57]

Added a minimal monkeypatching test.  I assume this shouldn't be manual, but I
am not quite sure where I should hook this in to be automatic.

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py#newcode145
tools/bisect-builds.py:145: numbers. The range starts with self.good_revision
and goes until
On 2012/11/01 19:06:28, Nico wrote:
> It looks like the 2nd sentence in the comment is no longer true. Can you
remove
> it?

Done.

http://codereview.chromium.org/11266025/diff/1/tools/bisect-builds.py#newcode673
tools/bisect-builds.py:673: print '  ' + WEBKIT_CHANGELOG_URL % (max_webkit_rev,
min_webkit_rev)
On 2012/11/01 19:06:28, Nico wrote:
> Should this be printed backwards too?

This is correct.

[Nico, 2012-11-02 22:32:11]

lgtm

http://codereview.chromium.org/11266025/diff/10001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

http://codereview.chromium.org/11266025/diff/10001/tools/bisect-builds.py#new...
tools/bisect-builds.py:453: min_str, max_str = ("bad", "good")
nit: parens on rhs not needed (but don't hurt either)