diff options
author | David Trowbridge <trowbrds@gmail.com> | 2010-02-06 10:21:25 -0800 |
---|---|---|
committer | David Trowbridge <trowbrds@gmail.com> | 2010-02-06 10:21:25 -0800 |
commit | d46241b9517211430fb29b7868146a6e41abe2e0 (patch) | |
tree | 4d51aa08e5806cbc1f4c83ccb52bc5b946769f15 | |
parent | 84f17d2c858213f726ab0a2f33bfcb3c4d765009 (diff) | |
parent | 5968f84456d1e349bd174c2b79d348de77321cd8 (diff) |
Merge commit 'origin/master'
-rwxr-xr-x | rbtools/postreview.py | 90 | ||||
-rw-r--r-- | rbtools/tests.py | 114 |
2 files changed, 164 insertions, 40 deletions
diff --git a/rbtools/postreview.py b/rbtools/postreview.py index 2419b92..39b29f8 100755 --- a/rbtools/postreview.py +++ b/rbtools/postreview.py @@ -113,7 +113,22 @@ options = None class APIError(Exception): - pass + def __init__(self, http_status, error_code, rsp=None, *args, **kwargs): + Exception.__init__(self, *args, **kwargs) + self.http_status = http_status + self.error_code = error_code + self.rsp = rsp + + def __str__(self): + code_str = "HTTP %d" % self.http_status + + if self.error_code: + code_str += ', API Error %d' % self.error_code + + if 'err' in self.rsp: + return '%s (%s)' % (self.rsp['err']['msg'], code_str) + else: + return code_str class RepositoryInfo: @@ -194,8 +209,7 @@ class SvnRepositoryInfo(RepositoryInfo): # If the server couldn't fetch the repository info, it will return # code 210. Ignore those. # Other more serious errors should still be raised, though. - rsp = e.args[0] - if rsp['err']['code'] == 210: + if e.error_code == 210: return None raise e @@ -327,10 +341,7 @@ class ReviewBoardServer(object): 'password': password, }) except APIError, e: - rsp, = e.args - - die("Unable to log in: %s (%s)" % (rsp["err"]["msg"], - rsp["err"]["code"])) + die("Unable to log in: %s" % e) debug("Logged in.") @@ -405,9 +416,7 @@ class ReviewBoardServer(object): rsp = self.api_post('api/json/reviewrequests/new/', data) except APIError, e: - rsp, = e.args - - if rsp['err']['code'] == 204: # Change number in use + if e.error_code == 204: # Change number in use if options.diff_only: # In this case, fall through and return to tempt_fate. debug("Review request already exists.") @@ -517,10 +526,26 @@ class ReviewBoardServer(object): rsp = json.loads(data) if rsp['stat'] == 'fail': - raise APIError, rsp + self.process_error(200, data) return rsp + def process_error(self, http_status, data): + """Processes an error, raising an APIError with the information.""" + try: + rsp = json.loads(data) + + assert rsp['stat'] == 'fail' + + debug("Got API Error %d (HTTP code %d): %s" % + (rsp['err']['code'], http_status, rsp['err']['msg'])) + debug("Error data: %r" % rsp) + raise APIError(http_status, rsp['err']['code'], rsp, + rsp['err']['msg']) + except ValueError: + debug("Got HTTP error: %s: %s" % (http_status, data)) + raise APIError(http_status, None, None, data) + def http_get(self, path): """ Performs an HTTP GET on the specified path, storing any cookies that @@ -529,19 +554,9 @@ class ReviewBoardServer(object): debug('HTTP GETting %s' % path) url = self._make_url(path) - - try: - rsp = urllib2.urlopen(url).read() - self.cookie_jar.save(self.cookie_file) - return rsp - except urllib2.HTTPError, e: - print "Unable to access %s (%s). The host path may be invalid" % \ - (url, e.code) - try: - debug(e.read()) - except AttributeError: - pass - die() + rsp = urllib2.urlopen(url).read() + self.cookie_jar.save(self.cookie_file) + return rsp def _make_url(self, path): """Given a path on the server returns a full http:// style url""" @@ -559,7 +574,10 @@ class ReviewBoardServer(object): """ Performs an API call using HTTP GET at the specified path. """ - return self.process_json(self.http_get(path)) + try: + return self.process_json(self.http_get(path)) + except urllib2.HTTPError, e: + self.process_error(e.code, e.read()) def http_post(self, path, fields, files=None): """ @@ -595,15 +613,15 @@ class ReviewBoardServer(object): die("Unable to access %s. The host path may be invalid\n%s" % \ (url, e)) - except urllib2.HTTPError, e: - die("Unable to access %s (%s). The host path may be invalid\n%s" % \ - (url, e.code, e.read())) def api_post(self, path, fields=None, files=None): """ Performs an API call using HTTP POST at the specified path. """ - return self.process_json(self.http_post(path, fields, files)) + try: + return self.process_json(self.http_post(path, fields, files)) + except urllib2.HTTPError, e: + self.process_error(e.code, e.read()) def _encode_multipart_formdata(self, fields, files): """ @@ -2434,8 +2452,7 @@ def tempt_fate(server, tool, changenum, diff_content=None, server.set_review_request_field(review_request, 'testing_done', options.testing_done) except APIError, e: - rsp, = e.args - if rsp['err']['code'] == 103: # Not logged in + if e.error_code == 103: # Not logged in retries = retries - 1 # We had an odd issue where the server ended up a couple of @@ -2450,11 +2467,9 @@ def tempt_fate(server, tool, changenum, diff_content=None, return if options.rid: - die("Error getting review request %s: %s (code %s)" % \ - (options.rid, rsp['err']['msg'], rsp['err']['code'])) + die("Error getting review request %s: %s" % (options.rid, e)) else: - die("Error creating review request: %s (code %s)" % \ - (rsp['err']['msg'], rsp['err']['code'])) + die("Error creating review request: %s" % e) if not server.info.supports_changesets or not options.change_only: @@ -2462,10 +2477,7 @@ def tempt_fate(server, tool, changenum, diff_content=None, server.upload_diff(review_request, diff_content, parent_diff_content) except APIError, e: - rsp, = e.args - print "Error uploading diff: %s (%s)" % (rsp['err']['msg'], - rsp['err']['code']) - debug(rsp) + print "Error uploading diff: %s" % e die("Your review request still exists, but the diff is not " + "attached.") diff --git a/rbtools/tests.py b/rbtools/tests.py index f0bc369..f0906e3 100644 --- a/rbtools/tests.py +++ b/rbtools/tests.py @@ -4,9 +4,21 @@ import shutil import sys import tempfile import unittest +import urllib2 + +try: + from cStringIO import StringIO +except ImportError: + from StringIO import StringIO + +try: + import json +except ImportError: + import simplejson as json from rbtools.postreview import execute, load_config_file -from rbtools.postreview import GitClient, RepositoryInfo +from rbtools.postreview import APIError, GitClient, RepositoryInfo, \ + ReviewBoardServer import rbtools.postreview @@ -87,6 +99,32 @@ def is_exe_in_path(name): return False +class MockHttpUnitTest(unittest.TestCase): + def setUp(self): + # Save the old http_get and http_post + self.saved_http_get = ReviewBoardServer.http_get + self.saved_http_post = ReviewBoardServer.http_post + + self.server = ReviewBoardServer('http://localhost:8080/', + RepositoryInfo(), None) + ReviewBoardServer.http_get = self._http_method + ReviewBoardServer.http_post = self._http_method + + self.http_response = "" + + rbtools.postreview.options = OptionsStub() + + def tearDown(self): + ReviewBoardServer.http_get = self.saved_http_get + ReviewBoardServer.http_post = self.saved_http_post + + def _http_method(self, *args, **kwargs): + if isinstance(self.http_response, Exception): + raise self.http_response + else: + return self.http_response + + class OptionsStub(object): def __init__(self): self.debug = True @@ -377,3 +415,77 @@ class GitClientTests(unittest.TestCase): ri = self.client.get_repository_info() self.assertEqual(self.client.diff(None), (diff, None)) + +class ApiTests(MockHttpUnitTest): + SAMPLE_ERROR_STR = json.dumps({ + 'stat': 'fail', + 'err': { + 'code': 100, + 'msg': 'This is a test failure', + } + }) + + def test_parse_get_error_http_200(self): + self.http_response = self.SAMPLE_ERROR_STR + + try: + data = self.server.api_get('/foo/') + + # Shouldn't be reached + self._assert(False) + except APIError, e: + self.assertEqual(e.http_status, 200) + self.assertEqual(e.error_code, 100) + self.assertEqual(e.rsp['stat'], 'fail') + self.assertEqual(str(e), + 'This is a test failure (HTTP 200, API Error 100)') + + def test_parse_post_error_http_200(self): + self.http_response = self.SAMPLE_ERROR_STR + + try: + data = self.server.api_post('/foo/') + + # Shouldn't be reached + self._assert(False) + except APIError, e: + self.assertEqual(e.http_status, 200) + self.assertEqual(e.error_code, 100) + self.assertEqual(e.rsp['stat'], 'fail') + self.assertEqual(str(e), + 'This is a test failure (HTTP 200, API Error 100)') + + def test_parse_get_error_http_400(self): + self.http_response = self._make_http_error('/foo/', 400, + self.SAMPLE_ERROR_STR) + + try: + data = self.server.api_get('/foo/') + + # Shouldn't be reached + self._assert(False) + except APIError, e: + self.assertEqual(e.http_status, 400) + self.assertEqual(e.error_code, 100) + self.assertEqual(e.rsp['stat'], 'fail') + self.assertEqual(str(e), + 'This is a test failure (HTTP 400, API Error 100)') + + def test_parse_post_error_http_400(self): + self.http_response = self._make_http_error('/foo/', 400, + self.SAMPLE_ERROR_STR) + + try: + data = self.server.api_post('/foo/') + + # Shouldn't be reached + self._assert(False) + except APIError, e: + self.assertEqual(e.http_status, 400) + self.assertEqual(e.error_code, 100) + self.assertEqual(e.rsp['stat'], 'fail') + self.assertEqual(str(e), + 'This is a test failure (HTTP 400, API Error 100)') + + def _make_http_error(self, url, code, body): + return urllib2.HTTPError(url, code, body, {}, StringIO(body)) |