From 069c6a9199d4b7b085d18d08a2f9109f9d2d936b Mon Sep 17 00:00:00 2001 From: Simon Wahlstrom Date: Wed, 15 Feb 2017 17:14:33 +0100 Subject: [PATCH 1/4] Implement logic and add tests for failed requests. --- test/helper.py | 10 +++++-- test/tinify_client_test.py | 53 ++++++++++++++++++++++++++++++++++---- tinify/client.py | 41 ++++++++++++++++------------- 3 files changed, 79 insertions(+), 25 deletions(-) diff --git a/test/helper.py b/test/helper.py index 50bc8b8..14af0bf 100644 --- a/test/helper.py +++ b/test/helper.py @@ -6,6 +6,7 @@ import os import httpretty from nose.exc import SkipTest +from mock import DEFAULT if sys.version_info < (2, 7): import unittest2 as unittest @@ -20,11 +21,16 @@ import tinify class RaiseException(object): - def __init__(self, exception): + def __init__(self, exception, num=None): self.exception = exception + self.num = num def __call__(self, *args, **kwargs): - raise self.exception + if self.num == 0: + return DEFAULT + else: + if self.num: self.num -= 1 + raise self.exception class TestHelper(unittest.TestCase): def setUp(self): diff --git a/test/tinify_client_test.py b/test/tinify_client_test.py index a4ada89..4dba394 100644 --- a/test/tinify_client_test.py +++ b/test/tinify_client_test.py @@ -79,7 +79,7 @@ def test_should_issue_request_with_proxy_authorization(self): self.assertEqual(self.request.headers['proxy-authorization'], 'Basic dXNlcjpwYXNz') -class TinifyClientRequestWithTimeout(TestHelper): +class TinifyClientRequestWithTimeoutRepeatedly(TestHelper): @patch('requests.sessions.Session.request', RaiseException(requests.exceptions.Timeout)) def test_should_raise_connection_error(self): with self.assertRaises(ConnectionError) as context: @@ -92,7 +92,14 @@ def test_should_raise_connection_error_with_cause(self): Client('key').request('GET', '/') self.assertIsInstance(context.exception.__cause__, requests.exceptions.Timeout) -class TinifyClientRequestWithConnectionError(TestHelper): +class TinifyClientRequestWithTimeoutOnce(TestHelper): + @patch('requests.sessions.Session.request') + def test_should_issue_request(self, mock): + mock.side_effect = RaiseException(requests.exceptions.Timeout, num=1) + mock.return_value = requests.Response() + self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) + +class TinifyClientRequestWithConnectionErrorRepeatedly(TestHelper): @patch('requests.sessions.Session.request', RaiseException(requests.exceptions.ConnectionError('connection error'))) def test_should_raise_connection_error(self): with self.assertRaises(ConnectionError) as context: @@ -105,14 +112,28 @@ def test_should_raise_connection_error_with_cause(self): Client('key').request('GET', '/') self.assertIsInstance(context.exception.__cause__, requests.exceptions.ConnectionError) -class TinifyClientRequestWithSomeError(TestHelper): +class TinifyClientRequestWithConnectionErrorOnce(TestHelper): + @patch('requests.sessions.Session.request') + def test_should_issue_request(self, mock): + mock.side_effect = RaiseException(requests.exceptions.ConnectionError, num=1) + mock.return_value = requests.Response() + self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) + +class TinifyClientRequestWithSomeErrorRepeatedly(TestHelper): @patch('requests.sessions.Session.request', RaiseException(RuntimeError('some error'))) def test_should_raise_connection_error(self): with self.assertRaises(ConnectionError) as context: Client('key').request('GET', '/') self.assertEqual('Error while connecting: some error', str(context.exception)) -class TinifyClientRequestWithServerError(TestHelper): +class TinifyClientRequestWithSomeErrorOnce(TestHelper): + @patch('requests.sessions.Session.request') + def test_should_issue_request(self, mock): + mock.side_effect = RaiseException(RuntimeError('some error'), num=1) + mock.return_value = requests.Response() + self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) + +class TinifyClientRequestWithServerErrorRepeatedly(TestHelper): def test_should_raise_server_error(self): httpretty.register_uri(httpretty.GET, 'https://api.tinify.com/', status=584, body='{"error":"InternalServerError","message":"Oops!"}') @@ -121,7 +142,18 @@ def test_should_raise_server_error(self): Client('key').request('GET', '/') self.assertEqual('Oops! (HTTP 584/InternalServerError)', str(context.exception)) -class TinifyClientRequestWithBadServerResponse(TestHelper): +class TinifyClientRequestWithServerErrorOnce(TestHelper): + def test_should_issue_request(self): + httpretty.register_uri(httpretty.GET, 'https://api.tinify.com/', + responses=[ + httpretty.Response(body='{"error":"InternalServerError","message":"Oops!"}', status=584), + httpretty.Response(body='all good', status=201), + ]) + + response = Client('key').request('GET', '/') + self.assertEqual('201', str(response.status_code)) + +class TinifyClientRequestWithBadServerResponseRepeatedly(TestHelper): def test_should_raise_server_error(self): httpretty.register_uri(httpretty.GET, 'https://api.tinify.com/', status=543, body='') @@ -132,6 +164,17 @@ def test_should_raise_server_error(self): msg = r'Error while parsing response: .* \(HTTP 543/ParseError\)' self.assertRegexpMatches(str(context.exception), msg) +class TinifyClientRequestWithBadServerResponseOnce(TestHelper): + def test_should_issue_request(self): + httpretty.register_uri(httpretty.GET, 'https://api.tinify.com/', + responses=[ + httpretty.Response(body='', status=543), + httpretty.Response(body='all good', status=201), + ]) + + response = Client('key').request('GET', '/') + self.assertEqual('201', str(response.status_code)) + class TinifyClientRequestWithClientError(TestHelper): def test_should_raise_client_error(self): httpretty.register_uri(httpretty.GET, 'https://api.tinify.com/', status=492, diff --git a/tinify/client.py b/tinify/client.py index c097248..625c1b0 100644 --- a/tinify/client.py +++ b/tinify/client.py @@ -15,6 +15,7 @@ class Client(object): API_ENDPOINT = 'https://api.tinify.com' USER_AGENT = 'Tinify/{0} Python/{1} ({2})'.format(tinify.__version__, platform.python_version(), platform.python_implementation()) + RETRY_COUNT = 1 def __init__(self, key, app_identifier=None, proxy=None): self.session = requests.sessions.Session() @@ -46,23 +47,27 @@ def request(self, method, url, body=None, header={}): elif body: params['data'] = body - try: - response = self.session.request(method, url, **params) - except requests.exceptions.Timeout as err: - raise ConnectionError('Timeout while connecting', cause=err) - except Exception as err: - raise ConnectionError('Error while connecting: {0}'.format(err), cause=err) - - count = response.headers.get('compression-count') - if count: - tinify.compression_count = int(count) - - if response.ok: - return response - else: - details = None + for retries in range(self.RETRY_COUNT, -1, -1): try: - details = response.json() + response = self.session.request(method, url, **params) + except requests.exceptions.Timeout as err: + if retries > 0: continue + raise ConnectionError('Timeout while connecting', cause=err) except Exception as err: - details = {'message': 'Error while parsing response: {0}'.format(err), 'error': 'ParseError'} - raise Error.create(details.get('message'), details.get('error'), response.status_code) + if retries > 0: continue + raise ConnectionError('Error while connecting: {0}'.format(err), cause=err) + + count = response.headers.get('compression-count') + if count: + tinify.compression_count = int(count) + + if response.ok: + return response + else: + details = None + try: + details = response.json() + except Exception as err: + details = {'message': 'Error while parsing response: {0}'.format(err), 'error': 'ParseError'} + if retries > 0 and response.status_code >= 500: continue + raise Error.create(details.get('message'), details.get('error'), response.status_code) From f93ead215bb3cc825526cc19188dacd0b595bfd7 Mon Sep 17 00:00:00 2001 From: Simon Wahlstrom Date: Thu, 16 Feb 2017 09:57:44 +0100 Subject: [PATCH 2/4] Add delay for retries. --- test/tinify_client_test.py | 5 +++++ tinify/client.py | 8 +++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/test/tinify_client_test.py b/test/tinify_client_test.py index 4dba394..176418a 100644 --- a/test/tinify_client_test.py +++ b/test/tinify_client_test.py @@ -15,6 +15,8 @@ except ImportError: from mock import patch +Client.RETRY_DELAY = 10 + class TinifyClientRequestWhenValid(TestHelper): def setUp(self): super(type(self), self).setUp() @@ -96,6 +98,7 @@ class TinifyClientRequestWithTimeoutOnce(TestHelper): @patch('requests.sessions.Session.request') def test_should_issue_request(self, mock): mock.side_effect = RaiseException(requests.exceptions.Timeout, num=1) + mock.return_value.status_code = 201 mock.return_value = requests.Response() self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) @@ -116,6 +119,7 @@ class TinifyClientRequestWithConnectionErrorOnce(TestHelper): @patch('requests.sessions.Session.request') def test_should_issue_request(self, mock): mock.side_effect = RaiseException(requests.exceptions.ConnectionError, num=1) + mock.return_value.status_code = 201 mock.return_value = requests.Response() self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) @@ -130,6 +134,7 @@ class TinifyClientRequestWithSomeErrorOnce(TestHelper): @patch('requests.sessions.Session.request') def test_should_issue_request(self, mock): mock.side_effect = RaiseException(RuntimeError('some error'), num=1) + mock.return_value.status_code = 201 mock.return_value = requests.Response() self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) diff --git a/tinify/client.py b/tinify/client.py index 625c1b0..6e44e49 100644 --- a/tinify/client.py +++ b/tinify/client.py @@ -8,15 +8,20 @@ import requests.exceptions from requests.compat import json import traceback +import time import tinify from .errors import ConnectionError, Error class Client(object): API_ENDPOINT = 'https://api.tinify.com' - USER_AGENT = 'Tinify/{0} Python/{1} ({2})'.format(tinify.__version__, platform.python_version(), platform.python_implementation()) + RETRY_COUNT = 1 + RETRY_DELAY = 500 + + USER_AGENT = 'Tinify/{0} Python/{1} ({2})'.format(tinify.__version__, platform.python_version(), platform.python_implementation()) + def __init__(self, key, app_identifier=None, proxy=None): self.session = requests.sessions.Session() if proxy: @@ -48,6 +53,7 @@ def request(self, method, url, body=None, header={}): params['data'] = body for retries in range(self.RETRY_COUNT, -1, -1): + if retries < self.RETRY_COUNT: time.sleep(self.RETRY_DELAY / 1000.0) try: response = self.session.request(method, url, **params) except requests.exceptions.Timeout as err: From 90770416ef740ff5a3a15f881daeba5686d69b0e Mon Sep 17 00:00:00 2001 From: Simon Wahlstrom Date: Thu, 16 Feb 2017 10:38:02 +0100 Subject: [PATCH 3/4] Make sure status is set correctly for tests. --- test/tinify_client_test.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/tinify_client_test.py b/test/tinify_client_test.py index 176418a..96bda04 100644 --- a/test/tinify_client_test.py +++ b/test/tinify_client_test.py @@ -98,8 +98,8 @@ class TinifyClientRequestWithTimeoutOnce(TestHelper): @patch('requests.sessions.Session.request') def test_should_issue_request(self, mock): mock.side_effect = RaiseException(requests.exceptions.Timeout, num=1) - mock.return_value.status_code = 201 mock.return_value = requests.Response() + mock.return_value.status_code = 201 self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) class TinifyClientRequestWithConnectionErrorRepeatedly(TestHelper): @@ -119,8 +119,8 @@ class TinifyClientRequestWithConnectionErrorOnce(TestHelper): @patch('requests.sessions.Session.request') def test_should_issue_request(self, mock): mock.side_effect = RaiseException(requests.exceptions.ConnectionError, num=1) - mock.return_value.status_code = 201 mock.return_value = requests.Response() + mock.return_value.status_code = 201 self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) class TinifyClientRequestWithSomeErrorRepeatedly(TestHelper): @@ -134,8 +134,8 @@ class TinifyClientRequestWithSomeErrorOnce(TestHelper): @patch('requests.sessions.Session.request') def test_should_issue_request(self, mock): mock.side_effect = RaiseException(RuntimeError('some error'), num=1) - mock.return_value.status_code = 201 mock.return_value = requests.Response() + mock.return_value.status_code = 201 self.assertIsInstance(Client('key').request('GET', '/', {}), requests.Response) class TinifyClientRequestWithServerErrorRepeatedly(TestHelper): From 437d9b189a7479a9c5cd6f3c1e5c334e83ed1dd0 Mon Sep 17 00:00:00 2001 From: Simon Wahlstrom Date: Thu, 16 Feb 2017 11:24:23 +0100 Subject: [PATCH 4/4] Use unittest.mock for > 3.3. Credits to Rolf. --- test/helper.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/helper.py b/test/helper.py index 14af0bf..7160d04 100644 --- a/test/helper.py +++ b/test/helper.py @@ -6,7 +6,11 @@ import os import httpretty from nose.exc import SkipTest -from mock import DEFAULT + +if sys.version_info < (3, 3): + from mock import DEFAULT +else: + from unittest.mock import DEFAULT if sys.version_info < (2, 7): import unittest2 as unittest