Merge pull request #63 from schra/patch/replace-httplib-with-requests

Replace httplib with requests

Author: thekashifmalik

.travis.yml

@@ -2,7 +2,7 @@ language: python
 python:
 - '2.7'
 - '3.5'
-install: pip install .
+install: pip install .[tests]
 script: python -m phabricator.tests.test_phabricator
 deploy:
   provider: pypi

phabricator/__init__.py

@@ -23,9 +23,10 @@
 import socket
 import pkgutil
 import time
+import requests
 
 from ._compat import (
-    MutableMapping, iteritems, string_types, httplib, urlparse, urlencode,
+    MutableMapping, iteritems, string_types, urlencode,
 )
 
 
@@ -290,43 +291,28 @@ def validate_kwarg(key, target):
             self.api.connect()
             kwargs['__conduit__'] = self.api._conduit
 
-        url = urlparse.urlparse(self.api.host)
-        if url.scheme == 'https':
-            conn = httplib.HTTPSConnection(url.netloc, timeout=self.api.timeout)
-        else:
-            conn = httplib.HTTPConnection(url.netloc, timeout=self.api.timeout)
-
-        path = url.path + '%s.%s' % (self.method, self.endpoint)
 
         headers = {
             'User-Agent': 'python-phabricator/%s' % str(self.api.clientVersion),
             'Content-Type': 'application/x-www-form-urlencoded'
         }
 
-        body = urlencode({
+        body = {
             "params": json.dumps(kwargs),
             "output": self.api.response_format
-        })
+        }
 
         # TODO: Use HTTP "method" from interfaces.json
-        conn.request('POST', path, body, headers)
-        response = conn.getresponse()
+        path = '%s%s.%s' % (self.api.host, self.method, self.endpoint)
+        response = requests.post(path, data=body, headers=headers, timeout=self.api.timeout)
 
         # Make sure we got a 2xx response indicating success
-        if not response.status >= 200 or not response.status < 300:
-            conn.close()
-            raise httplib.HTTPException(
-                'Bad response status: {0}'.format(response.status)
+        if not response.status_code >= 200 or not response.status_code < 300:
+            raise requests.exceptions.HTTPError(
+                'Bad response status: {0}'.format(response.status_code)
             )
 
-        response_data = response.read()
-        conn.close()
-        if isinstance(response_data, str):
-            response = response_data
-        else:
-            response = response_data.decode("utf-8")
-
-        data = self._parse_response(response)
+        data = self._parse_response(response.text)
 
         return Result(data['result'])
 

phabricator/_compat.py

@@ -7,16 +7,6 @@
 except ImportError:
     from collections import MutableMapping
 
-try:
-    import httplib
-except ImportError:
-    import http.client as httplib
-
-try:
-    import urlparse
-except ImportError:
-    import urllib.parse as urlparse
-
 try:
     from urllib import urlencode
 except ImportError:

phabricator/tests/test_phabricator.py

@@ -3,20 +3,14 @@
 except ImportError:
     import unittest
 
-try:
-    from StringIO import StringIO
-except ImportError:
-    from io import StringIO
-
-try:
-    import unittest.mock as mock
-except ImportError:
-    import mock
+import requests
+import responses
 
 from pkg_resources import resource_string
 import json
 
 import phabricator
+phabricator.ARCRC = {}  # overwrite any arcrc that might be read
 
 
 RESPONSES = json.loads(
@@ -40,7 +34,7 @@ def setUp(self):
         self.api = phabricator.Phabricator(
             username='test',
             certificate='test',
-            host='http://localhost'
+            host='http://localhost/api/'
         )
         self.api.certificate = CERTIFICATE
 
@@ -49,37 +43,32 @@ def test_generate_hash(self):
         hashed = self.api.generate_hash(token)
         self.assertEqual(hashed, 'f8d3bea4e58a2b2967d93d5b307bfa7c693b2e7f')
 
-    @mock.patch('phabricator.httplib.HTTPConnection')
-    def test_connect(self, mock_connection):
-        mock_obj = mock_connection.return_value = mock.Mock()
-        mock_obj.getresponse.return_value = StringIO(
-            RESPONSES['conduit.connect']
-        )
-        mock_obj.getresponse.return_value.status = 200
-        mock_obj.close = mock.Mock()
+    @responses.activate
+    def test_connect(self):
+        responses.add('POST', 'http://localhost/api/conduit.connect',
+                      body=RESPONSES['conduit.connect'], status=200)
 
         api = phabricator.Phabricator(
             username='test',
             certificate='test',
-            host='http://localhost'
+            host='http://localhost/api/'
         )
 
         api.connect()
         keys = api._conduit.keys()
         self.assertIn('sessionKey', keys)
         self.assertIn('connectionID', keys)
-        mock_obj.close.assert_called_once_with()
+        assert len(responses.calls) == 1
 
-    @mock.patch('phabricator.httplib.HTTPConnection')
-    def test_user_whoami(self, mock_connection):
-        mock_obj = mock_connection.return_value = mock.Mock()
-        mock_obj.getresponse.return_value = StringIO(RESPONSES['user.whoami'])
-        mock_obj.getresponse.return_value.status = 200
+    @responses.activate
+    def test_user_whoami(self):
+        responses.add('POST', 'http://localhost/api/user.whoami',
+                      body=RESPONSES['user.whoami'], status=200)
 
         api = phabricator.Phabricator(
             username='test',
             certificate='test',
-            host='http://localhost'
+            host='http://localhost/api/'
         )
         api._conduit = True
 
@@ -89,7 +78,7 @@ def test_classic_resources(self):
         api = phabricator.Phabricator(
             username='test',
             certificate='test',
-            host='http://localhost'
+            host='http://localhost/api/'
         )
 
         self.assertEqual(api.user.whoami.method, 'user')
@@ -99,42 +88,37 @@ def test_nested_resources(self):
         api = phabricator.Phabricator(
             username='test',
             certificate='test',
-            host='http://localhost'
+            host='http://localhost/api/'
         )
 
         self.assertEqual(api.diffusion.repository.edit.method, 'diffusion')
-        self.assertEqual(api.diffusion.repository.edit.endpoint, 'repository.edit')
+        self.assertEqual(
+            api.diffusion.repository.edit.endpoint, 'repository.edit')
 
-    @mock.patch('phabricator.httplib.HTTPConnection')
-    def test_bad_status(self, mock_connection):
-        mock_obj = mock_connection.return_value = mock.Mock()
-        mock_obj.getresponse.return_value = mock.Mock()
-        mock_obj.getresponse.return_value.status = 400
-        mock_obj.close = mock.Mock()
+    @responses.activate
+    def test_bad_status(self):
+        responses.add(
+            'POST', 'http://localhost/api/conduit.connect', status=400)
 
         api = phabricator.Phabricator(
-                username='test',
-                certificate='test',
-                host='http://localhost'
+            username='test',
+            certificate='test',
+            host='http://localhost/api/'
         )
-        api._conduit = True
 
-        with self.assertRaises(phabricator.httplib.HTTPException):
+        with self.assertRaises(requests.exceptions.HTTPError):
             api.user.whoami()
-        mock_obj.close.assert_called_once_with()
+        assert len(responses.calls) == 1
 
-    @mock.patch('phabricator.httplib.HTTPConnection')
-    def test_maniphest_find(self, mock_connection):
-        mock_obj = mock_connection.return_value = mock.Mock()
-        mock_obj.getresponse.return_value = StringIO(
-            RESPONSES['maniphest.find']
-        )
-        mock_obj.getresponse.return_value.status = 200
+    @responses.activate
+    def test_maniphest_find(self):
+        responses.add('POST', 'http://localhost/api/maniphest.find',
+                      body=RESPONSES['maniphest.find'], status=200)
 
         api = phabricator.Phabricator(
             username='test',
             certificate='test',
-            host='http://localhost'
+            host='http://localhost/api/'
         )
         api._conduit = True
 
@@ -165,16 +149,18 @@ def test_validation(self):
 
     def test_map_param_type(self):
         uint = 'uint'
-        self.assertEqual(phabricator.map_param_type(uint), int) 
+        self.assertEqual(phabricator.map_param_type(uint), int)
 
         list_bool = 'list<bool>'
-        self.assertEqual(phabricator.map_param_type(list_bool), [bool]) 
+        self.assertEqual(phabricator.map_param_type(list_bool), [bool])
 
         list_pair = 'list<pair<callsign, path>>'
-        self.assertEqual(phabricator.map_param_type(list_pair), [tuple]) 
+        self.assertEqual(phabricator.map_param_type(list_pair), [tuple])
 
         complex_list_pair = 'list<pair<string-constant<"gtcm">, string>>'
-        self.assertEqual(phabricator.map_param_type(complex_list_pair), [tuple])
+        self.assertEqual(phabricator.map_param_type(
+            complex_list_pair), [tuple])
+
 
     def test_endpoint_shadowing(self):
         shadowed_endpoints = [e for e in self.api.interface.keys() if e in self.api.__dict__]

setup.py

@@ -4,7 +4,7 @@
 
 from setuptools import setup, find_packages
 
-tests_requires = []
+tests_requires = ['responses>=0.12']
 
 if sys.version_info[:2] < (2, 7):
     tests_requires.append('unittest2')
@@ -21,8 +21,11 @@
     description='Phabricator API Bindings',
     packages=find_packages(),
     zip_safe=False,
+    install_requires=['requests>=2.22'],
     test_suite='phabricator.tests.test_phabricator',
-    tests_require=tests_requires,
+    extras_require={
+        'tests': tests_requires,
+    },
     include_package_data=True,
     classifiers=[
         'Intended Audience :: Developers',