Aborting bisect early when the bug specified in the bisect config is closed.

Aborting bisect early when the bug specified in the bisect config is closed.

Also, renaming valid_params and valid_parameters when validating bisect options
to required_params and required_parameters respectively to better reflect what
they actually are.

Fixing some runtime errors introduced by the refactoring to use ArgsParser
instead of OptionsParser.

BUG=424688
Committed: https://crrev.com/b20bc1fcb69905c2edd73e690b0b6ec58e658ec0
Cr-Commit-Position: refs/heads/master@{#302468}

[RobertoCN, 2014-10-31 23:50:28]

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

[RobertoCN, 2014-10-31 23:54:54]

WDYT

[qyearsley, 2014-11-01 16:56:49]

On 2014/10/31 23:54:54, robertocn wrote:
> WDYT

Looks good in general; Some style comments.

Regarding the name of the module... "crbug_query" or "query_crbug"? The first
makes sense, but I used "fetch_build" instead of "build_fetch". Also, the main
module was called "bisect_perf_regression" instead of "perf_regression_bisect".

The perf dashboard has got slightly inconsistent module names as well, where we
have "bug_update_from_rietveld", but "change_internal_only". We have
"edit_sheriffs", but "bug_create". This inconsistency kind of bugs me.

Maybe module names should all be made to be noun phrases instead of verb
phrases?

Prasad, Sergiy, any opinions on this matter?

[qyearsley, 2014-11-01 16:56:57]

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
File tools/auto_bisect/crbug_query.py (right):

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:24: def querySingleIssue(issueId,
urlTemplate=SINGLE_ISSUE_URL):
Chromium style:
 - Function and method names have first letter capitalized.
 - Variable names and arguments use lower_with_underscore.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:27: This uses the deprecated Issue Tracker API
to fetch a json representation of
I personally like capitalizing abbreviations like JSON, URL, and ID. I think if
you capitalize those as well it will be a bit more consistent because API is
capitalized here.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:32: urlTemplate (optional): to override the url
to request from.
No need to say (optional) here. You could say "The URL to request from, with
"%s" where the bug ID will go."

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:33: Returns:
Style: blank line between Args and Returns sections.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:34: A dictionary as parsed by the json library
from the tracker response.
After the Returns section, you could also put a "Raises" section, where say that
it may raise urllib2.URLError. This is optional.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:36: issueId = str(issueId)
It will also be implicitly converted to a string with str() when it's put into
the template, because the template contains "%s".

Example of this conversion:

.>>> class Obj(object):
.>>>   def __str__(self): return 'asdf'
.>>> print 'xxx %s xxx' % Obj()
xxx asdf xxx

So, this line is not needed.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:37: assert issueId.isdigit()
Cool, I didn't know that isdigit would check all the characters in a string and
make sure they're all digits. This makes bisect_utils.IsStringInt redundant (it
serves the same purpose).

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:42: def getIssueState(issueId):
What are the possible values of state? It's just "open" and "closed", right? You
could put this in a docstring if you want.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:52: )
Style: closing parenthesis goes on the same line with the argument, no need to
put it on its own line.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:66: return getIssueState(issueId) == u'closed'
No need to explicitly use a unicode string, u"closed" == "closed".

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
File tools/auto_bisect/crbug_query_test_data.py (right):

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query_test_data.py:20: BROKEN_ISSUE_DATA =
"\n<HTML><HEAD><TITLE>Not a JSON Doc</TITLE></HEAD></HTML>"
This could also go directly in crbug_query_test.py, right? Any reason for having
it in a separate file?

[RobertoCN, 2014-11-02 07:48:35]

Addressed comments. Looking better now?

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
File tools/auto_bisect/crbug_query.py (right):

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:24: def querySingleIssue(issueId,
urlTemplate=SINGLE_ISSUE_URL):
On 2014/11/01 16:56:56, qyearsley wrote:
> Chromium style:
>  - Function and method names have first letter capitalized.
>  - Variable names and arguments use lower_with_underscore.

Done.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:27: This uses the deprecated Issue Tracker API
to fetch a json representation of
On 2014/11/01 16:56:56, qyearsley wrote:
> I personally like capitalizing abbreviations like JSON, URL, and ID. I think
if
> you capitalize those as well it will be a bit more consistent because API is
> capitalized here.

Done.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:32: urlTemplate (optional): to override the url
to request from.
On 2014/11/01 16:56:56, qyearsley wrote:
> No need to say (optional) here. You could say "The URL to request from, with
> "%s" where the bug ID will go."

Done.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:33: Returns:
On 2014/11/01 16:56:56, qyearsley wrote:
> Style: blank line between Args and Returns sections.

Done.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:34: A dictionary as parsed by the json library
from the tracker response.
On 2014/11/01 16:56:57, qyearsley wrote:
> After the Returns section, you could also put a "Raises" section, where say
that
> it may raise urllib2.URLError. This is optional.

Done.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:36: issueId = str(issueId)
On 2014/11/01 16:56:57, qyearsley wrote:
> It will also be implicitly converted to a string with str() when it's put into
> the template, because the template contains "%s".
> 
> Example of this conversion:
> 
> .>>> class Obj(object):
> .>>>   def __str__(self): return 'asdf'
> .>>> print 'xxx %s xxx' % Obj()
> xxx asdf xxx
> 
> So, this line is not needed.

Done.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:37: assert issueId.isdigit()
On 2014/11/01 16:56:56, qyearsley wrote:
> Cool, I didn't know that isdigit would check all the characters in a string
and
> make sure they're all digits. This makes bisect_utils.IsStringInt redundant
(it
> serves the same purpose).

They are slightly different, current implementation of isStringInt will work
with strings like '-1', '0.05' and so, Whereas isDigit is stricter in the sense
that it allows exactly [0-9] which is what I want for inserting into the URL.
Besides, I do not anticipate leading zeroes making any difference but this would
keep them.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:42: def getIssueState(issueId):
On 2014/11/01 16:56:56, qyearsley wrote:
> What are the possible values of state? It's just "open" and "closed", right?
You
> could put this in a docstring if you want.

Done.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:52: )
On 2014/11/01 16:56:56, qyearsley wrote:
> Style: closing parenthesis goes on the same line with the argument, no need to
> put it on its own line.

Done.

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query.py:66: return getIssueState(issueId) == u'closed'
On 2014/11/01 16:56:56, qyearsley wrote:
> No need to explicitly use a unicode string, u"closed" == "closed".

You are right. I just wondered if I should made the comparison explicit to state
the assumption, since I am comparing to the result from parsing a json output
(where string are always parsed as unicode)

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
File tools/auto_bisect/crbug_query_test_data.py (right):

https://codereview.chromium.org/697713003/diff/1/tools/auto_bisect/crbug_quer...
tools/auto_bisect/crbug_query_test_data.py:20: BROKEN_ISSUE_DATA =
"\n<HTML><HEAD><TITLE>Not a JSON Doc</TITLE></HEAD></HTML>"
On 2014/11/01 16:56:57, qyearsley wrote:
> This could also go directly in crbug_query_test.py, right? Any reason for
having
> it in a separate file?

Originally it contained the full text of the json responses. I guess it does
make more sense now to move it back to the test file.

[RobertoCN, 2014-11-02 21:03:08]

Sergiy, could you please take a look too? Specifically I'd like you to review
the changes to the ArgsParser. I did not test it extensively, I just addressed
the runtime errors that I saw when testing my own feature.

[Sergiy Byelozyorov, 2014-11-03 13:42:00]

Oops. Sorry for breaking it. I'll write some integration tests for it in my next
CL. l-g-t-m for BisectOptions changes.

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

https://codereview.chromium.org/697713003/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:2537: group.add_argument('--bug_id',
type=str, default='',
No need for type=str and default=''. These are defaults anyway.

https://codereview.chromium.org/697713003/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:2539: 'If this number is given,
bisect will attempt to ' +
Please align with opening ' on the line before like everywhere else.

https://codereview.chromium.org/697713003/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:2540: ' verify that the bug is not
closed before starting.')
Remove space in the beginning - it is already present on previous line.

https://codereview.chromium.org/697713003/diff/20001/tools/auto_bisect/bisect...
tools/auto_bisect/bisect_perf_regression.py:2552:
group.add_argument('--build_preference', type=str,
Thanks for fixing it. Please remove type='str' argument here and below as it's
str by default.

[qyearsley, 2014-11-03 17:23:41]

LGTM now :-)

I copied your changes to BisectOptions in http://crrev.com/302414, so now when
you rebase your changes there should be the same as the latest version.
Although, I agree with Sergiy that you could remove type=str and since this is
the default.

[prasadv, 2014-11-03 17:48:12]

lgtm

[RobertoCN, 2014-11-03 19:07:28]

The CQ bit was checked by robertocn@chromium.org

[commit-bot: I haz the power, 2014-11-03 19:08:23]

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

[commit-bot: I haz the power, 2014-11-03 19:50:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-11-03 19:51:16]

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