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

Review URL: https://codereview.chromium.org/697713003

Cr-Commit-Position: refs/heads/master@{#302468}

7 files changed, 540 insertions, 6 deletions

diff --git a/tools/auto_bisect/bisect_perf_regression.py b/tools/auto_bisect/bisect_perf_regression.py
index 89ba5e2..032ef13 100755
--- a/tools/auto_bisect/bisect_perf_regression.py
+++ b/tools/auto_bisect/bisect_perf_regression.py
@@ -54,6 +54,7 @@ from bisect_results import BisectResults
 from bisect_state import BisectState
 import bisect_utils
 import builder
+import query_crbug
 import math_utils
 import request_build
 import source_control
@@ -2134,6 +2135,7 @@ class BisectPerformanceMetrics(object):
     Returns:
       A BisectResults object.
     """
+
     # Choose depot to bisect first
     target_depot = 'chromium'
     if self.opts.target_platform == 'android-chrome':
@@ -2489,6 +2491,7 @@ class BisectOptions(object):
     self.builder_port = None
     self.bisect_mode = bisect_utils.BISECT_MODE_MEAN
     self.improvement_direction = 0
+    self.bug_id = ''
 
   @staticmethod
   def _AddBisectOptionsGroup(parser):
@@ -2536,6 +2539,11 @@ class BisectOptions(object):
                                 bisect_utils.BISECT_MODE_RETURN_CODE],
                        help='The bisect mode. Choices are to bisect on the '
                             'difference in mean, std_dev, or return_code.')
+    group.add_argument('--bug_id', default='',
+                       help='The id for the bug associated with this bisect. ' +
+                            'If this number is given, bisect will attempt to ' +
+                            'verify that the bug is not closed before '
+                            'starting.')
 
   @staticmethod
   def _AddBuildOptionsGroup(parser):
@@ -2547,12 +2555,12 @@ class BisectOptions(object):
                        'working_directory and that will be used to perform the '
                        'bisection. This parameter is optional, if it is not '
                        'supplied, the script will work from the current depot.')
-    group.add_argument('--build_preference', type=str,
+    group.add_argument('--build_preference',
                        choices=['msvs', 'ninja', 'make'],
                        help='The preferred build system to use. On linux/mac '
                             'the options are make/ninja. On Windows, the '
                             'options are msvs/ninja.')
-    group.add_argument('--target_platform', type=str, default='chromium',
+    group.add_argument('--target_platform', default='chromium',
                        choices=['chromium', 'android', 'android-chrome'],
                        help='The target platform. Choices are "chromium" '
                             '(current platform), or "android". If you specify '
@@ -2577,11 +2585,11 @@ class BisectOptions(object):
     group.add_argument('--gs_bucket', default='', dest='gs_bucket',
                        help='Name of Google Storage bucket to upload or '
                             'download build. e.g., chrome-perf')
-    group.add_argument('--target_arch', type=str, default='ia32',
+    group.add_argument('--target_arch', default='ia32',
                        dest='target_arch', choices=['ia32', 'x64', 'arm'],
                        help='The target build architecture. Choices are "ia32" '
                             '(default), "x64" or "arm".')
-    group.add_argument('--target_build_type', type=str, default='Release',
+    group.add_argument('--target_build_type', default='Release',
                        choices=['Release', 'Debug'],
                        help='The target build type. Choices are "Release" '
                             '(default), or "Debug".')
@@ -2712,6 +2720,23 @@ def main():
     opts = BisectOptions()
     opts.ParseCommandLine()
 
+    if opts.bug_id:
+      if opts.output_buildbot_annotations:
+        bisect_utils.OutputAnnotationStepStart('Checking Issue Tracker')
+      issue_closed = query_crbug.CheckIssueClosed(opts.bug_id)
+      if issue_closed:
+        print 'Aborting bisect because bug is closed'
+      else:
+        print 'Could not confirm bug is closed, proceeding.'
+      if opts.output_buildbot_annotations:
+        bisect_utils.OutputAnnotationStepClosed()
+      if issue_closed:
+        results = BisectResults(abort_reason='the bug is closed.')
+        bisect_test = BisectPerformanceMetrics(opts, os.getcwd())
+        bisect_test.printer.FormatAndPrintResults(results)
+        return 0
+
+
     if opts.extra_src:
       extra_src = bisect_utils.LoadExtraSrc(opts.extra_src)
       if not extra_src:
diff --git a/tools/auto_bisect/bisect_printer.py b/tools/auto_bisect/bisect_printer.py
index ecac6eb..f58c28f 100644
--- a/tools/auto_bisect/bisect_printer.py
+++ b/tools/auto_bisect/bisect_printer.py
@@ -24,6 +24,20 @@ Test Metric: %(metrics)s
 Relative Change: %(change)s
 Estimated Confidence: %(confidence).02f%%"""
 
+# When the bisect was aborted without a bisect failure the following template
+# is used.
+ABORT_REASON_TEMPLATE = """
+===== BISECTION ABORTED =====
+The bisect was aborted because %(abort_reason)s
+Please contact the the team (see below) if you believe this is in error.
+
+Bug ID: %(bug_id)s
+
+Test Command: %(command)s
+Test Metric: %(metric)s
+Good revision: %(good_revision)s
+Bad revision: %(bad_revision)s """
+
 # The perf dashboard specifically looks for the string
 # "Author  : " to parse out who to cc on a bug. If you change the
 # formatting here, please update the perf dashboard as well.
@@ -87,6 +101,10 @@ class BisectPrinter(object):
     Args:
       bisect_results: BisectResult object containing results to be printed.
     """
+    if bisect_results.abort_reason:
+      self._PrintAbortResults(bisect_results.abort_reason)
+      return
+
     if self.opts.output_buildbot_annotations:
       bisect_utils.OutputAnnotationStepStart('Build Status Per Revision')
 
@@ -137,6 +155,22 @@ class BisectPrinter(object):
     self._PrintTestedCommitsTable(revision_states, first_working_rev,
                                   last_broken_rev, 100, final_step=False)
 
+  def _PrintAbortResults(self, abort_reason):
+
+    if self.opts.output_buildbot_annotations:
+      bisect_utils.OutputAnnotationStepStart('Results')
+    print ABORT_REASON_TEMPLATE % {
+        'abort_reason': abort_reason,
+        'bug_id': self.opts.bug_id or 'NOT SPECIFIED',
+        'command': self.opts.command,
+        'metric': '/'.join(self.opts.metric),
+        'good_revision': self.opts.good_revision,
+        'bad_revision': self.opts.bad_revision,
+    }
+    self._PrintThankYou()
+    if self.opts.output_buildbot_annotations:
+      bisect_utils.OutputAnnotationStepClosed()
+
   @staticmethod
   def _PrintThankYou():
     print RESULTS_THANKYOU
diff --git a/tools/auto_bisect/bisect_results.py b/tools/auto_bisect/bisect_results.py
index 6a7c6e3..df93de6 100644
--- a/tools/auto_bisect/bisect_results.py
+++ b/tools/auto_bisect/bisect_results.py
@@ -38,7 +38,7 @@ class BisectResults(object):
   """
 
   def __init__(self, bisect_state=None, depot_registry=None, opts=None,
-               runtime_warnings=None, error=None):
+               runtime_warnings=None, error=None, abort_reason=None):
     """Computes final bisect results after a bisect run is complete.
 
     This constructor should be called in one of the following ways:
@@ -58,7 +58,8 @@ class BisectResults(object):
     """
 
     self.error = error
-    if error is not None:
+    self.abort_reason = abort_reason
+    if error is not None or abort_reason is not None:
       return
 
     assert (bisect_state is not None and depot_registry is not None and
diff --git a/tools/auto_bisect/query_crbug.py b/tools/auto_bisect/query_crbug.py
new file mode 100644
index 0000000..b8f4fcb
--- /dev/null
+++ b/tools/auto_bisect/query_crbug.py
@@ -0,0 +1,78 @@
+# 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(issue_id, url_template=SINGLE_ISSUE_URL):
+  """Queries the tracker for a specific issue. Returns a dict.
+
+  This uses the deprecated Issue Tracker API to fetch a JSON representation of
+  the issue details.
+
+  Args:
+    issue_id: An int or string representing the issue id.
+    url_template: URL to query the tracker with '%s' instead of the bug id.
+
+  Returns:
+    A dictionary as parsed by the JSON library from the tracker response.
+
+  Raises:
+    urllib2.HTTPError when appropriate.
+  """
+  assert str(issue_id).isdigit()
+  response = urllib2.urlopen(url_template % issue_id).read()
+  return json.loads(response)
+
+
+def GetIssueState(issue_id):
+  """Returns either 'closed' or 'open' for the given bug ID.
+
+  Args:
+    issue_id: string or string-castable object containing a numeric bug ID.
+  Returns:
+    'open' or 'closed' depending on the state of the bug.
+  Raises:
+    IssueTrackerQueryException if the data cannot be retrieved or parsed.
+  """
+  try:
+    query_response = QuerySingleIssue(issue_id)
+    # We assume the query returns a single result hence the [0]
+    issue_detail = query_response['feed']['entry'][0]
+    state = issue_detail['issues$state']['$t']
+    return state
+  except urllib2.URLError:
+    raise IssueTrackerQueryException(
+        'Could not fetch the details form the issue tracker.')
+  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(issue_id):
+  """Checks if a given issue is closed. Returns False when in doubt."""
+  try:
+    return GetIssueState(issue_id) == 'closed'
+  except IssueTrackerQueryException:
+    # When we can't be sure we return false
+    return False
diff --git a/tools/auto_bisect/query_crbug_test.py b/tools/auto_bisect/query_crbug_test.py
new file mode 100644
index 0000000..40e6fa8
--- /dev/null
+++ b/tools/auto_bisect/query_crbug_test.py
@@ -0,0 +1,82 @@
+# 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.
+
+import os
+import sys
+import unittest
+import urllib2
+
+from query_crbug import CheckIssueClosed
+
+SRC = os.path.join(os.path.dirname(__file__), os.path.pardir, os.path.pardir)
+sys.path.append(os.path.join(SRC, 'third_party', 'pymock'))
+
+import mock
+
+_current_directory = os.path.dirname(__file__)
+_test_data_directory = os.path.join(_current_directory, 'test_data')
+
+# These strings are simulated responses to various conditions when querying
+# the chromium issue tracker.
+CLOSED_ISSUE_DATA = open(os.path.join(_test_data_directory,
+                                      'closed.json')).read()
+OPEN_ISSUE_DATA = open(os.path.join(_test_data_directory,
+                                    'open.json')).read()
+UNEXPECTED_FORMAT_DATA = CLOSED_ISSUE_DATA.replace('issues$state', 'gibberish')
+BROKEN_ISSUE_DATA = "\n<HTML><HEAD><TITLE>Not a JSON Doc</TITLE></HEAD></HTML>"
+
+
+class mockResponse(object):
+  def __init__(self, result):
+    self._result = result
+
+  def read(self):
+    return self._result
+
+
+def mockUrlOpen(url):
+  # Note that these strings DO NOT represent http responses. They are just
+  # memorable numeric bug ids to use.
+  if '200' in url:
+    return mockResponse(CLOSED_ISSUE_DATA)
+  elif '201' in url:
+    return mockResponse(OPEN_ISSUE_DATA)
+  elif '300' in url:
+    return mockResponse(UNEXPECTED_FORMAT_DATA)
+  elif '403' in url:
+    raise urllib2.URLError('')
+  elif '404' in url:
+    return mockResponse('')
+  elif '500' in url:
+    return mockResponse(BROKEN_ISSUE_DATA)
+
+
+class crbugQueryTest(unittest.TestCase):
+  @mock.patch('urllib2.urlopen',mockUrlOpen)
+  def testClosedIssueIsClosed(self):
+    self.assertTrue(CheckIssueClosed(200))
+
+  @mock.patch('urllib2.urlopen',mockUrlOpen)
+  def testOpenIssueIsNotClosed(self):
+    self.assertFalse(CheckIssueClosed(201))
+
+  @mock.patch('urllib2.urlopen',mockUrlOpen)
+  def testUnexpectedFormat(self):
+    self.assertFalse(CheckIssueClosed(300))
+
+  @mock.patch('urllib2.urlopen',mockUrlOpen)
+  def testUrlError(self):
+    self.assertFalse(CheckIssueClosed(403))
+
+  @mock.patch('urllib2.urlopen',mockUrlOpen)
+  def testEmptyResponse(self):
+    self.assertFalse(CheckIssueClosed(404))
+
+  @mock.patch('urllib2.urlopen',mockUrlOpen)
+  def testBrokenResponse(self):
+    self.assertFalse(CheckIssueClosed(500))
+
+
+if __name__ == '__main__':
+  unittest.main()
diff --git a/tools/auto_bisect/test_data/closed.json b/tools/auto_bisect/test_data/closed.json
new file mode 100644
index 0000000..ab87a21
--- /dev/null
+++ b/tools/auto_bisect/test_data/closed.json
@@ -0,0 +1,140 @@
+{
+ "version": "1.0",
+ "encoding": "UTF-8",
+ "feed": {
+  "xmlns": "http://www.w3.org/2005/Atom",
+  "xmlns$openSearch": "http://a9.com/-/spec/opensearch/1.1/",
+  "xmlns$gd": "http://schemas.google.com/g/2005",
+  "xmlns$issues": "http://schemas.google.com/projecthosting/issues/2009",
+  "id": {
+   "$t": "http://code.google.com/feeds/issues/p/chromium/issues/full"
+  },
+  "updated": {
+   "$t": "2014-10-31T23:44:30.795Z"
+  },
+  "title": {
+   "$t": "Issues - chromium"
+  },
+  "subtitle": {
+   "$t": "Issues - chromium"
+  },
+  "link": [
+   {
+    "rel": "alternate",
+    "type": "text/html",
+    "href": "http://code.google.com/p/chromium/issues/list"
+   },
+   {
+    "rel": "http://schemas.google.com/g/2005#feed",
+    "type": "application/atom+xml",
+    "href": "https://code.google.com/feeds/issues/p/chromium/issues/full"
+   },
+   {
+    "rel": "http://schemas.google.com/g/2005#post",
+    "type": "application/atom+xml",
+    "href": "https://code.google.com/feeds/issues/p/chromium/issues/full"
+   },
+   {
+    "rel": "self",
+    "type": "application/atom+xml",
+    "href": "https://code.google.com/feeds/issues/p/chromium/issues/full?alt=json&max-results=1&id=422382"
+   }
+  ],
+  "generator": {
+   "$t": "ProjectHosting",
+   "version": "1.0",
+   "uri": "http://code.google.com/feeds/issues"
+  },
+  "openSearch$totalResults": {
+   "$t": 1
+  },
+  "openSearch$startIndex": {
+   "$t": 1
+  },
+  "openSearch$itemsPerPage": {
+   "$t": 1
+  },
+  "entry": [
+   {
+    "gd$etag": "W/\"CUUFSX47eCl7ImA9XRdQGEk.\"",
+    "id": {
+     "$t": "http://code.google.com/feeds/issues/p/chromium/issues/full/422382"
+    },
+    "published": {
+     "$t": "2014-10-10T17:07:06.000Z"
+    },
+    "updated": {
+     "$t": "2014-10-20T22:13:38.000Z"
+    },
+    "title": {
+     "$t": "11.1% regression in indexeddb_perf at 298653:298680"
+    },
+    "content": {
+     "$t": "See the link to graphs below.",
+     "type": "html"
+    },
+    "link": [
+     {
+      "rel": "replies",
+      "type": "application/atom+xml",
+      "href": "http://code.google.com/feeds/issues/p/chromium/issues/422382/comments/full"
+     },
+     {
+      "rel": "alternate",
+      "type": "text/html",
+      "href": "http://code.google.com/p/chromium/issues/detail?id=422382"
+     },
+     {
+      "rel": "self",
+      "type": "application/atom+xml",
+      "href": "https://code.google.com/feeds/issues/p/chromium/issues/full/422382"
+     }
+    ],
+    "author": [
+     {
+      "name": {
+       "$t": "gov...@chromium.org"
+      },
+      "uri": {
+       "$t": "/u/104724762920274240672/"
+      }
+     }
+    ],
+    "issues$closedDate": {
+     "$t": "2014-10-20T22:10:22.000Z"
+    },
+    "issues$id": {
+     "$t": 422382
+    },
+    "issues$label": [
+     {
+      "$t": "Type-Bug-Regression"
+     },
+     {
+      "$t": "Performance-Sheriff"
+     },
+     {
+      "$t": "Pri-2"
+     }
+    ],
+    "issues$owner": {
+     "issues$uri": {
+      "$t": "/u/104724762920274240672/"
+     },
+     "issues$username": {
+      "$t": "gov...@chromium.org"
+     }
+    },
+    "issues$stars": {
+     "$t": 0
+    },
+    "issues$state": {
+     "$t": "closed"
+    },
+    "issues$status": {
+     "$t": "WontFix"
+    }
+   }
+  ]
+ }
+}
diff --git a/tools/auto_bisect/test_data/open.json b/tools/auto_bisect/test_data/open.json
new file mode 100644
index 0000000..e23f2f4
--- /dev/null
+++ b/tools/auto_bisect/test_data/open.json
@@ -0,0 +1,174 @@
+{
+ "version": "1.0",
+ "encoding": "UTF-8",
+ "feed": {
+  "xmlns": "http://www.w3.org/2005/Atom",
+  "xmlns$openSearch": "http://a9.com/-/spec/opensearch/1.1/",
+  "xmlns$gd": "http://schemas.google.com/g/2005",
+  "xmlns$issues": "http://schemas.google.com/projecthosting/issues/2009",
+  "id": {
+   "$t": "http://code.google.com/feeds/issues/p/chromium/issues/full"
+  },
+  "updated": {
+   "$t": "2014-10-31T23:44:18.640Z"
+  },
+  "title": {
+   "$t": "Issues - chromium"
+  },
+  "subtitle": {
+   "$t": "Issues - chromium"
+  },
+  "link": [
+   {
+    "rel": "alternate",
+    "type": "text/html",
+    "href": "http://code.google.com/p/chromium/issues/list"
+   },
+   {
+    "rel": "http://schemas.google.com/g/2005#feed",
+    "type": "application/atom+xml",
+    "href": "https://code.google.com/feeds/issues/p/chromium/issues/full"
+   },
+   {
+    "rel": "http://schemas.google.com/g/2005#post",
+    "type": "application/atom+xml",
+    "href": "https://code.google.com/feeds/issues/p/chromium/issues/full"
+   },
+   {
+    "rel": "self",
+    "type": "application/atom+xml",
+    "href": "https://code.google.com/feeds/issues/p/chromium/issues/full?alt=json&max-results=1&id=424688"
+   }
+  ],
+  "generator": {
+   "$t": "ProjectHosting",
+   "version": "1.0",
+   "uri": "http://code.google.com/feeds/issues"
+  },
+  "openSearch$totalResults": {
+   "$t": 1
+  },
+  "openSearch$startIndex": {
+   "$t": 1
+  },
+  "openSearch$itemsPerPage": {
+   "$t": 1
+  },
+  "entry": [
+   {
+    "gd$etag": "W/\"A08NQX47eCl7ImA9XRdXFkw.\"",
+    "id": {
+     "$t": "http://code.google.com/feeds/issues/p/chromium/issues/full/424688"
+    },
+    "published": {
+     "$t": "2014-10-17T18:50:15.000Z"
+    },
+    "updated": {
+     "$t": "2014-10-29T21:58:10.000Z"
+    },
+    "title": {
+     "$t": "Should Not Start Bisect on Closed Bugs."
+    },
+    "content": {
+     "$t": "I have noticed that in some cases bisect jobs are running for closed bugs:\r\nhttps://code.google.com/p/chromium/issues/detail?id=422661\r\nhttps://code.google.com/p/chromium/issues/detail?id=422228\r\nhttps://code.google.com/p/chromium/issues/detail?id=421488\r\n\r\nIt is possible that the bugs can be marked as closed when the corresponding bisect jobs are in the queue. So to avoid bisects on such bugs, can we please add logic to bisect script to first check for bug state before running the bisect. This will save us from doing some unnecessary bisects.\r\n\r\n",
+     "type": "html"
+    },
+    "link": [
+     {
+      "rel": "replies",
+      "type": "application/atom+xml",
+      "href": "http://code.google.com/feeds/issues/p/chromium/issues/424688/comments/full"
+     },
+     {
+      "rel": "alternate",
+      "type": "text/html",
+      "href": "http://code.google.com/p/chromium/issues/detail?id=424688"
+     },
+     {
+      "rel": "self",
+      "type": "application/atom+xml",
+      "href": "https://code.google.com/feeds/issues/p/chromium/issues/full/424688"
+     }
+    ],
+    "author": [
+     {
+      "name": {
+       "$t": "anan...@chromium.org"
+      },
+      "uri": {
+       "$t": "/u/112777092906361529031/"
+      }
+     }
+    ],
+    "issues$cc": [
+     {
+      "issues$uri": {
+       "$t": "/u/116704265016059607269/"
+      },
+      "issues$username": {
+       "$t": "pras...@chromium.org"
+      }
+     },
+     {
+      "issues$uri": {
+       "$t": "/u/107012661329935444717/"
+      },
+      "issues$username": {
+       "$t": "qyears...@chromium.org"
+      }
+     },
+     {
+      "issues$uri": {
+       "$t": "/u/tonyg@chromium.org/"
+      },
+      "issues$username": {
+       "$t": "tonyg@chromium.org"
+      }
+     },
+     {
+      "issues$uri": {
+       "$t": "/u/114810703796781371055/"
+      },
+      "issues$username": {
+       "$t": "robert...@chromium.org"
+      }
+     }
+    ],
+    "issues$id": {
+     "$t": 424688
+    },
+    "issues$label": [
+     {
+      "$t": "Type-Feature"
+     },
+     {
+      "$t": "Pri-2"
+     },
+     {
+      "$t": "Cr-Tests-AutoBisect"
+     },
+     {
+      "$t": "OS-All"
+     }
+    ],
+    "issues$owner": {
+     "issues$uri": {
+      "$t": "/u/114810703796781371055/"
+     },
+     "issues$username": {
+      "$t": "robert...@chromium.org"
+     }
+    },
+    "issues$stars": {
+     "$t": 1
+    },
+    "issues$state": {
+     "$t": "open"
+    },
+    "issues$status": {
+     "$t": "Assigned"
+    }
+   }
+  ]
+ }
+}