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

tools/auto_bisect/crbug_query.py

+# Copyright 2014 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Utility functions to query the chromium issue tracker.
+
+Note that documentation for the Issue Tracker API says it's DEPRECATED, however
+it seems to be in use in other places like the performance dashboard. Also,
+this module attempts to handle most exceptions thrown by querying the tracker
+so that when and if this api is turned off no impact is caused to the bisection
+process."""
+
+import json
+import urllib2
+
+SINGLE_ISSUE_URL = ('https://code.google.com/feeds/issues/p/chromium/issues'
+                    '/full?id=%s&alt=json')
+
+
+class IssueTrackerQueryException(Exception):
+  pass
+
+
+def querySingleIssue(issueId, urlTemplate=SINGLE_ISSUE_URL):

[qyearsley, 2014/11/01 16:56:56]

Chromium style:
 - Function and method names have first letter capitalized.
 - Variable names and arguments use lower_with_underscore.

[RobertoCN, 2014/11/02 07:48:34]

Done.

+  """Queries the tracker for a specific issue. Returns a dict.
+
+  This uses the deprecated Issue Tracker API to fetch a json representation of

[qyearsley, 2014/11/01 16:56:56]

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.

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

Done.

+  the issue details.
+
+  Args:
+    issueId: An int or string representing the issue id.
+    urlTemplate (optional): to override the url to request from.

[qyearsley, 2014/11/01 16:56:56]

No need to say (optional) here. You could say "The URL to request from, with
"%s" where the bug ID will go."

[RobertoCN, 2014/11/02 07:48:34]

Done.

+  Returns:

[qyearsley, 2014/11/01 16:56:56]

Style: blank line between Args and Returns sections.

[RobertoCN, 2014/11/02 07:48:34]

Done.

+    A dictionary as parsed by the json library from the tracker response.

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

After the Returns section, you could also put a "Raises" section, where say that
it may raise urllib2.URLError. This is optional.

[RobertoCN, 2014/11/02 07:48:34]

Done.

+  """
+  issueId = str(issueId)

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

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.

[RobertoCN, 2014/11/02 07:48:34]

Done.

+  assert issueId.isdigit()

[qyearsley, 2014/11/01 16:56:56]

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).

[RobertoCN, 2014/11/02 07:48:34]

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.

+  response = urllib2.urlopen(urlTemplate % issueId).read()
+  return json.loads(response)
+
+
+def getIssueState(issueId):

[qyearsley, 2014/11/01 16:56:56]

What are the possible values of state? It's just "open" and "closed", right? You
could put this in a docstring if you want.

[RobertoCN, 2014/11/02 07:48:34]

Done.

+  try:
+    queryResponse = querySingleIssue(issueId)
+    # We assume the query returns a single result hence the [0]
+    issueDetail = queryResponse['feed']['entry'][0]
+    state = issueDetail['issues$state']['$t']
+    return state
+  except urllib2.URLError:
+    raise IssueTrackerQueryException(
+        'Could not fetch the details form the issue tracker.'
+    )

[qyearsley, 2014/11/01 16:56:56]

Style: closing parenthesis goes on the same line with the argument, no need to
put it on its own line.

[RobertoCN, 2014/11/02 07:48:34]

Done.

+  except ValueError:
+    raise IssueTrackerQueryException(
+        'Could not parse the issue tracker\'s response as a json doc.'
+    )
+  except KeyError:
+    raise IssueTrackerQueryException(
+        'The data from the issue tracker is not in the expected format.'
+    )
+
+
+def checkIssueClosed(issueId):
+  """Checks if a given issue is closed. Returns False when in doubt."""
+  try:
+    return getIssueState(issueId) == u'closed'

[qyearsley, 2014/11/01 16:56:56]

No need to explicitly use a unicode string, u"closed" == "closed".

[RobertoCN, 2014/11/02 07:48:34]

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)

+  except IssueTrackerQueryException:
+    # When we can't be sure we return false
+    return False