Merge lp:~cjwatson/canonical-identity-provider/readonly-requests into lp:canonical-identity-provider/release

[Colin Watson]

Convert identityprovider.readonly to requests, which is easier to test.

While working out what would be involved in a Python 3 port, I ran across the urllib2 mocks here, which are ugly and a bit difficult to make 2/3 bilingual, so I thought it'd be best to just port this code to requests.

Incidentally, the old code was buggy: it doesn't restore the previous default socket timeout if urlopen raises an exception. Who knows if this ever caused a problem ...

I had to upgrade wsgi-intercept to support requests. u1testutils.wsgi_intercept isn't friends with the new version, but it seems like unnecessary complexity anyway, so I just open-coded the few lines that we need.

Requires https://code.launchpad.net/~cjwatson/canonical-identity-provider/dependencies-readonly-requests/+merge/336738.

[Adam Collard (adam-collard)]

Nice, responses is much easier to read.

review: Approve

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/51/

[Otto Co-Pilot (otto-copilot)]

Running landing tests failed
https://jenkins.ols.canonical.com/online-services/job/canonical-identity-provider/54/

=== modified file 'requirements.txt'
--- requirements.txt	2017-04-03 17:28:42 +0000
+++ requirements.txt	2018-01-29 17:03:41 +0000
@@ -31,6 +31,7 @@
 pycparser==2.14
 pymacaroons==0.9.1
 raven==6.0.0
+requests==2.10.0
 requests-oauthlib==0.4.2
 six==1.10.0
 statsd==3.1

=== modified file 'requirements_devel.txt'
--- requirements_devel.txt	2016-07-06 00:29:59 +0000
+++ requirements_devel.txt	2018-01-29 17:03:41 +0000
@@ -1,5 +1,6 @@
 -r requirements.txt
 
+cookies==2.2.1
 coverage==3.6
 cssselect==0.7.1
 ecdsa==0.11
@@ -19,6 +20,7 @@
 pyquery==1.2.1
 python-mimeparse==0.1.4
 python-subunit==1.1.0
+responses==0.8.1
 selenium==2.53.6
 sqlparse==0.1.4
 sst==0.2.5dev
@@ -26,6 +28,6 @@
 testtools==0.9.39
 txfixtures==0.1.4
 u1-test-utils==0.5
-wsgi-intercept==0.5.1
+wsgi-intercept==1.6.0
 -r requirements_docs.txt
 pygpgme==0.3

=== modified file 'src/api/v10/tests/utils.py'
--- src/api/v10/tests/utils.py	2015-11-24 16:30:04 +0000
+++ src/api/v10/tests/utils.py	2018-01-29 17:03:41 +0000
@@ -10,7 +10,12 @@
 from django.core.handlers.wsgi import WSGIHandler
 from django.test.utils import override_settings
 from lazr.restfulclient.resource import ServiceRoot
-from u1testutils.wsgi_intercept import WSGIInterceptedTestCase
+from wsgi_intercept import (
+    add_wsgi_intercept,
+    httplib2_intercept,
+    remove_wsgi_intercept,
+    urllib_intercept,
+)
 
 from identityprovider.tests.utils import SSOBaseTransactionTestCase
 from identityprovider.tests.factory import SSOObjectFactory
@@ -22,19 +27,21 @@
 
 
 @override_settings(CAPTCHA_PUBLIC_KEY='public', CAPTCHA_PRIVATE_KEY='private')
-class AnonAPITestCase(SSOBaseTransactionTestCase, WSGIInterceptedTestCase):
+class AnonAPITestCase(SSOBaseTransactionTestCase):
     factory = SSOObjectFactory()
 
     def setUp(self):
         super(AnonAPITestCase, self).setUp()
         host_url = settings.SSO_ROOT_URL.rstrip('/')
         parse = urlparse(host_url)
-        callbacks = {
-            'default': WSGIHandler,
-            (parse.hostname, parse.port): WSGIHandler,
-        }
-        self.setup_intercept(callbacks, intercept_api=True)
-        self.addCleanup(self.teardown_intercept)
+        self.unset_env('http_proxy')
+        self.unset_env('https_proxy')
+        urllib_intercept.install_opener()
+        self.addCleanup(urllib_intercept.uninstall_opener)
+        httplib2_intercept.install()
+        self.addCleanup(httplib2_intercept.uninstall)
+        add_wsgi_intercept(parse.hostname, parse.port, WSGIHandler)
+        self.addCleanup(remove_wsgi_intercept, parse.hostname, parse.port)
         self.api = ServiceRoot(None, host_url + '/api/1.0')
 
         # patch Captcha so it never hits the real network

=== modified file 'src/identityprovider/readonly.py'
--- src/identityprovider/readonly.py	2013-06-20 15:37:44 +0000
+++ src/identityprovider/readonly.py	2018-01-29 17:03:41 +0000
@@ -3,11 +3,9 @@
 
 import json
 import os
-import socket
 import stat
-import urllib
-import urllib2
 
+import requests
 from django.conf import settings
 
 # When a request arrives, the middleware will:
@@ -54,7 +52,6 @@
     if post is None:
         post = []
     post.append(('secret', settings.READONLY_SECRET))
-    post = urllib.urlencode(post)
 
     if scheme is None:
         scheme = 'http'
@@ -66,14 +63,10 @@
     headers = {}
     if virtual_host is not None:
         headers['Host'] = virtual_host
-    req = urllib2.Request(url, headers=headers, data=post)
     try:
-        oldtimeout = socket.getdefaulttimeout()
-        socket.setdefaulttimeout(5)
-        datafile = urllib2.urlopen(req)
-        data = datafile.read()
-        socket.setdefaulttimeout(oldtimeout)
-    except urllib2.URLError:
+        response = requests.post(url, headers=headers, data=post, timeout=5)
+        data = response.content
+    except requests.RequestException:
         data = None
     return data
 

=== modified file 'src/identityprovider/tests/test_command_readonly.py'
--- src/identityprovider/tests/test_command_readonly.py	2018-01-29 01:46:19 +0000
+++ src/identityprovider/tests/test_command_readonly.py	2018-01-29 17:03:41 +0000
@@ -12,7 +12,7 @@
 from wsgi_intercept import (
     add_wsgi_intercept,
     remove_wsgi_intercept,
-    urllib2_intercept,
+    requests_intercept,
 )
 
 from identityprovider.readonly import ReadOnlyManager
@@ -46,8 +46,10 @@
                         _DBFAILOVER_FLAG_DIR)
 
         # setup wsgi intercept mechanism to simulate wsgi server
-        urllib2_intercept.install_opener()
-        self.addCleanup(urllib2_intercept.uninstall_opener)
+        self.unset_env('http_proxy')
+        self.unset_env('https_proxy')
+        requests_intercept.install()
+        self.addCleanup(requests_intercept.uninstall)
         for server in self.servers:
             add_wsgi_intercept(server['HOST'], int(server['PORT']),
                                WSGIHandler)

=== modified file 'src/identityprovider/tests/test_readonly.py'
--- src/identityprovider/tests/test_readonly.py	2015-11-24 16:47:21 +0000
+++ src/identityprovider/tests/test_readonly.py	2018-01-29 17:03:41 +0000
@@ -6,11 +6,9 @@
 import stat
 import shutil
 import tempfile
-import socket
-import urllib2
-
-from StringIO import StringIO
-
+
+import requests
+import responses
 from django.conf import settings
 from django.core.urlresolvers import reverse
 
@@ -50,55 +48,49 @@
     scheme = 'https'
     vhost = 'http://foobar.baz'
 
-    def setup_mock_urlopen(self, func):
-        self.orig_urlopen = urllib2.urlopen
-        urllib2.urlopen = func
-
-    def restore_orig_urlopen(self):
-        urllib2.urlopen = self.orig_urlopen
-
-    def mock_urlopen(self, req, data=None):
-        self.req = req
-        self.assertEqual(5, socket.getdefaulttimeout())
-        return StringIO(self.msg)
-
+    @responses.activate
     def test_plain_remote_req(self):
-        self.setup_mock_urlopen(self.mock_urlopen)
+        responses.add(
+            responses.POST, 'http://%s/readonlydata' % self.host,
+            body=self.msg)
         server = {'host': self.host}
         self.assertEqual(self.msg, _remote_req(**server))
-        self.assertEqual(self.host, self.req.get_host())
-        self.restore_orig_urlopen()
+        # This tests the PreparedRequest, which is before urllib3 inserts a
+        # default Host header.
+        self.assertNotIn('Host', responses.calls[0].request.headers)
 
+    @responses.activate
     def test_https_remote_req(self):
-        self.setup_mock_urlopen(self.mock_urlopen)
+        responses.add(
+            responses.POST, '%s://%s/readonlydata' % (self.scheme, self.host),
+            body=self.msg)
         server = {'host': self.host, 'scheme': self.scheme}
         self.assertEqual(self.msg, _remote_req(**server))
-        self.assertEqual(self.scheme, self.req.get_type())
-        self.assertEqual(self.host, self.req.get_host())
-        self.restore_orig_urlopen()
+        # This tests the PreparedRequest, which is before urllib3 inserts a
+        # default Host header.
+        self.assertNotIn('Host', responses.calls[0].request.headers)
 
+    @responses.activate
     def test_vhost_remote_req(self):
-        self.setup_mock_urlopen(self.mock_urlopen)
+        responses.add(
+            responses.POST, '%s://%s/readonlydata' % (self.scheme, self.host),
+            body=self.msg)
         server = {
             'host': self.host,
             'scheme': self.scheme,
             'virtual_host': self.vhost,
         }
         self.assertEqual(self.msg, _remote_req(**server))
-        headers = {'Host': self.vhost}
-        self.assertEqual(headers, self.req.headers)
-        self.assertEqual(self.scheme, self.req.get_type())
-        self.assertEqual(self.host, self.req.get_host())
-        self.restore_orig_urlopen()
-
-    def test_remote_req_urllib2_error(self):
-        def mock_urlopen(req, data=None):
-            raise urllib2.URLError((-1, 'error'))
-
-        self.setup_mock_urlopen(mock_urlopen)
+        self.assertEqual(
+            self.vhost, responses.calls[0].request.headers['Host'])
+
+    @responses.activate
+    def test_remote_req_error(self):
+        responses.add(
+            responses.POST, 'http://%s/readonlydata' % self.host,
+            body=requests.RequestException('error'))
         server = {'host': self.host}
         self.assertEqual(None, _remote_req(**server))
-        self.restore_orig_urlopen()
 
 
 class ReadOnlyFlagFilesTestCase(ReadOnlyBaseTestCase):
@@ -179,18 +171,6 @@
         assert self.client.login(
             username=account.preferredemail.email, password='password')
 
-    def patch_urlopen(self, mock_urlopen=None):
-        self.reqs = []
-
-        def _mock_urlopen(req, data=None):
-            self.reqs.append(req)
-            return StringIO(json.dumps({}))
-
-        if mock_urlopen is None:
-            mock_urlopen = _mock_urlopen
-
-        self.patch(urllib2, 'urlopen', new=mock_urlopen)
-
     def test_readonly_admin(self):
         self.login_with_staff()
 
@@ -219,13 +199,10 @@
         atts = get_server_atts(servers)
         self.assertEqual(atts, expected)
 
+    @responses.activate
     def test_get_server_atts_data_error(self):
-
-        def mock_loads(data):
-            raise ValueError()
-
-        self.patch(json, 'loads', new=mock_loads)
-        self.patch_urlopen()
+        responses.add(
+            responses.POST, 'http://localhost:8000/readonlydata', body='{')
 
         servers = [{'SERVER_ID': 'localhost', 'SCHEME': 'http',
                     'HOST': 'localhost', 'VIRTUAL_HOST': '', 'PORT': '8000'}]
@@ -235,13 +212,11 @@
         atts = get_server_atts(servers)
         self.assertEqual(atts, expected)
 
+    @responses.activate
     def test_get_server_atts_readonly(self):
-
-        def mock_urlopen(req, data=None):
-            data = {'readonly': True}
-            return StringIO(json.dumps(data))
-
-        self.patch_urlopen(mock_urlopen=mock_urlopen)
+        responses.add(
+            responses.POST, 'http://localhost:8000/readonlydata',
+            json={'readonly': True})
 
         servers = [{'SERVER_ID': 'localhost', 'SCHEME': 'http',
                     'HOST': 'localhost', 'VIRTUAL_HOST': '', 'PORT': '8000'}]
@@ -260,8 +235,10 @@
         self.assertEqual(r.context['appserver'], 'localhost')
         self.assertEqual(r.context['action'], 'set')
 
+    @responses.activate
     def test_readonly_confirm_post(self):
-        self.patch_urlopen()
+        responses.add(
+            responses.POST, 'http://localhost:8000/readonlydata', json={})
         self.login_with_staff()
 
         new_setting = [
@@ -271,15 +248,17 @@
         ]
         with self.settings(APP_SERVERS=new_setting):
             r = self.client.post('/readonly/localhost/set')
-        data = map(lambda x: x.data, self.reqs)
+        data = [call.request.body for call in responses.calls]
         self.assertEqual(data, [
             "action=set&secret=%s" % settings.READONLY_SECRET
         ])
 
         self.assertRedirects(r, '/readonly')
 
+    @responses.activate
     def test_update_server_all_appservers(self):
-        self.patch_urlopen()
+        responses.add(
+            responses.POST, 'http://localhost:8000/readonlydata', json={})
         new_setting = [
             {'SERVER_ID': 'localhost', 'SCHEME': 'http',
              'HOST': 'localhost', 'VIRTUAL_HOST': '', 'PORT': '8000'},
@@ -288,14 +267,16 @@
         ]
         with self.settings(APP_SERVERS=new_setting):
             update_server('set')
-        data = map(lambda x: x.data, self.reqs)
+        data = [call.request.body for call in responses.calls]
         self.assertEqual(data, [
             "action=set&secret=%s" % settings.READONLY_SECRET,
             "action=set&secret=%s" % settings.READONLY_SECRET
         ])
 
+    @responses.activate
     def test_update_server_one_appserver(self):
-        self.patch_urlopen()
+        responses.add(
+            responses.POST, 'http://localhost:8000/readonlydata', json={})
         new_setting = [
             {'SERVER_ID': 'localhost', 'SCHEME': 'http',
              'HOST': 'localhost', 'VIRTUAL_HOST': '', 'PORT': '8000'},
@@ -304,20 +285,22 @@
         ]
         with self.settings(APP_SERVERS=new_setting):
             update_server('set', 'localhost')
-        data = map(lambda x: x.data, self.reqs)
+        data = [call.request.body for call in responses.calls]
         self.assertEqual(data, [
             "action=set&secret=%s" % settings.READONLY_SECRET
         ])
 
+    @responses.activate
     def test_update_server_one_connection(self):
-        self.patch_urlopen()
+        responses.add(
+            responses.POST, 'http://localhost:8000/readonlydata', json={})
         new_setting = [
             {'SERVER_ID': 'localhost', 'SCHEME': 'http',
              'HOST': 'localhost', 'VIRTUAL_HOST': '', 'PORT': '8000'},
         ]
         with self.settings(APP_SERVERS=new_setting):
             update_server('set', 'localhost')
-        data = map(lambda x: x.data, self.reqs)
+        data = [call.request.body for call in responses.calls]
         self.assertEqual(data, [
             "action=set&secret=%s" % settings.READONLY_SECRET,
         ])

=== modified file 'src/identityprovider/tests/test_wsgi.py'
--- src/identityprovider/tests/test_wsgi.py	2015-11-23 15:20:40 +0000
+++ src/identityprovider/tests/test_wsgi.py	2018-01-29 17:03:41 +0000
@@ -7,12 +7,11 @@
 from mock import Mock
 
 from identityprovider.tests.utils import SSOBaseTestCase
-from u1testutils.wsgi_intercept import WSGIInterceptedTestCase
 
 from identityprovider.wsgi import make_app, BzrRevnoMiddleware
 
 
-class WsgiTestCase(SSOBaseTestCase, WSGIInterceptedTestCase):
+class WsgiTestCase(SSOBaseTestCase):
 
     def setUp(self):
         super(WsgiTestCase, self).setUp()

=== modified file 'src/identityprovider/tests/utils.py'
--- src/identityprovider/tests/utils.py	2017-04-19 15:36:34 +0000
+++ src/identityprovider/tests/utils.py	2018-01-29 17:03:41 +0000
@@ -256,6 +256,13 @@
         # loop detection.
         self.client.cookies['C'] = '1'
 
+    def unset_env(self, name):
+        """Temporarily unset an environment variable."""
+        orig_value = os.environ.get(name)
+        if orig_value is not None:
+            self.addCleanup(os.environ.__setitem__, name, orig_value)
+            del os.environ[name]
+
 
 class SSOBaseTestCase(SSOBaseTestCaseMixin, TestCase):