Merge lp:~alecu/ubuntu-sso-client/post-restful-cleanups into lp:ubuntu-sso-client

[Alejandro J. Cura]

- Replace urllib2.Request with common web-client (LP: #884972).
- Replace urllib2.urlopen with common web-client (LP: #884975).
- Fix tests failing in sso with test_webclient (LP: #920591).

- Replace urllib2.Request with common web-client (LP: #884972).
- Replace urllib2.urlopen with common web-client (LP: #884975).
- Fix tests failing in sso with test_webclient (LP: #920591).

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

[Manuel de la Peña (mandel)]

Great work!

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~alecu/ubuntu-sso-client/post-restful-cleanups into lp:ubuntu-sso-client failed. Below is the output from the failed tests.

*** Running test suite for ubuntu_sso ***
ubuntu_sso.tests.test_account
  AccountTestCase
    test_generate_captcha ... [OK]
    test_is_not_validated ... [OK]
    test_is_not_validated_empty_result ... [OK]
    test_is_validated ... [OK]
    test_login_if_http_error ... [OK]
    test_login_if_no_error ... [OK]
    test_register_user_checks_valid_email ... [OK]
    test_register_user_checks_valid_password ... [OK]
    test_register_user_if_status_error ... [OK]
    test_register_user_if_status_error_with_string_message ... [OK]
    test_register_user_if_status_ok ... [OK]
    test_register_user_if_status_unknown ... [OK]
    test_request_password_reset_token_if_http_error ... [OK]
    test_request_password_reset_token_if_status_ok ... [OK]
    test_request_password_reset_token_if_status_unknown ... [OK]
    test_set_new_password_if_http_error ... [OK]
    test_set_new_password_if_status_ok ... [OK]
    test_set_new_password_if_status_unknown ... [OK]
    test_validate_email_if_status_error ... [OK]
    test_validate_email_if_status_error_with_string_message ... [OK]
    test_validate_email_if_status_ok ... [OK]
    test_validate_email_if_status_unknown ... [OK]
  EnvironOverridesTestCase
    test_no_override_service_url ... [OK]
    test_override_service_url ... [OK]
    test_service_url_as_parameter ... [OK]
twisted.trial.unittest
  TestCase
    runTest ... [OK]
ubuntu_sso.tests.test_account
  TimestampedAuthorizerTestCase
    test_authorize_request_includes_timestamp ... [OK]
ubuntu_sso.tests.test_credentials
  BasicTestCase
    runTest ... [OK]
  ClearCredentialsTestCase
    test_clear_credentials ... [OK]
    test_keyring_failure ... [OK]
  CredentialsAuthDeniedTestCase
    test_auth_denial_cb ... [OK]
  CredentialsCallbacksTestCase
    test_callbacks_are_stored ... [OK]
    test_callbacks_default_to_no_op ... [OK]
    test_creation_parameters_are_stored ... ...

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~alecu/ubuntu-sso-client/post-restful-cleanups into lp:ubuntu-sso-client failed. Below is the output from the failed tests.

*** Running test suite for ubuntu_sso ***
ubuntu_sso.tests.test_account
  AccountTestCase
    test_generate_captcha ... [OK]
    test_is_not_validated ... [OK]
    test_is_not_validated_empty_result ... [OK]
    test_is_validated ... [OK]
    test_login_if_http_error ... [OK]
    test_login_if_no_error ... [OK]
    test_register_user_checks_valid_email ... [OK]
    test_register_user_checks_valid_password ... [OK]
    test_register_user_if_status_error ... [OK]
    test_register_user_if_status_error_with_string_message ... [OK]
    test_register_user_if_status_ok ... [OK]
    test_register_user_if_status_unknown ... [OK]
    test_request_password_reset_token_if_http_error ... [OK]
    test_request_password_reset_token_if_status_ok ... [OK]
    test_request_password_reset_token_if_status_unknown ... [OK]
    test_set_new_password_if_http_error ... [OK]
    test_set_new_password_if_status_ok ... [OK]
    test_set_new_password_if_status_unknown ... [OK]
    test_validate_email_if_status_error ... [OK]
    test_validate_email_if_status_error_with_string_message ... [OK]
    test_validate_email_if_status_ok ... [OK]
    test_validate_email_if_status_unknown ... [OK]
  EnvironOverridesTestCase
    test_no_override_service_url ... [OK]
    test_override_service_url ... [OK]
    test_service_url_as_parameter ... [OK]
twisted.trial.unittest
  TestCase
    runTest ... [OK]
ubuntu_sso.tests.test_account
  TimestampedAuthorizerTestCase
    test_authorize_request_includes_timestamp ... [OK]
ubuntu_sso.tests.test_credentials
  BasicTestCase
    runTest ... [OK]
  ClearCredentialsTestCase
    test_clear_credentials ... [OK]
    test_keyring_failure ... [OK]
  CredentialsAuthDeniedTestCase
    test_auth_denial_cb ... [OK]
  CredentialsCallbacksTestCase
    test_callbacks_are_stored ... [OK]
    test_callbacks_default_to_no_op ... [OK]
    test_creation_parameters_are_stored ... ...

[lp:~alecu/ubuntu-sso-client/post-restful-cleanups]

872. By Alejandro J. Cura on 2012-02-03

fix xvfb-run command line

=== modified file 'run-tests'
--- run-tests	2012-02-02 18:09:21 +0000
+++ run-tests	2012-02-03 17:25:21 +0000
@@ -38,20 +38,24 @@
         echo "Please install the 'pep8' package."
     fi
 }
-
 unset GTK_MODULES
 
+XVFB=$(which xvfb-run)
+if [ $XVFB ]; then
+    XVFB_CMDLINE="$XVFB -a"
+fi
+
 echo "*** Running test suite for ""$MODULE"" ***"
-`which xvfb-run` u1trial --reactor=gi -p "$QT_TESTS_PATH, $GTK_TESTS_PATH" -i "test_windows.py" "$MODULE"
+$XVFB_CMDLINE u1trial --reactor=gi -p "$QT_TESTS_PATH, $GTK_TESTS_PATH" -i "test_windows.py" "$MODULE"
 rm -rf _trial_temp
 
 echo "*** Running GTK test suite ***"
-`which xvfb-run` u1trial --reactor=gi --gui "$GTK_TESTS_PATH"
+$XVFB_CMDLINE u1trial --reactor=gi --gui "$GTK_TESTS_PATH"
 rm -rf _trial_temp
 
 echo "*** Running QT test suite ***"
 ./setup.py build
-USE_QT_MAINLOOP=True `which xvfb-run` u1trial --reactor=qt4 --gui "$QT_TESTS_PATH"
+USE_QT_MAINLOOP=True $XVFB_CMDLINE u1trial --reactor=qt4 --gui "$QT_TESTS_PATH"
 rm -rf _trial_temp
 rm -rf build
 

=== modified file 'ubuntu_sso/account.py'
--- ubuntu_sso/account.py	2012-01-26 18:52:23 +0000
+++ ubuntu_sso/account.py	2012-02-03 17:25:21 +0000
@@ -134,7 +134,10 @@
         logger.debug('generate_captcha: requesting captcha, filename: %r',
                      filename)
         restful_client = restful.RestfulClient(self.service_url)
-        captcha = yield restful_client.restcall(u"captchas.new")
+        try:
+            captcha = yield restful_client.restcall(u"captchas.new")
+        finally:
+            restful_client.shutdown()
 
         # download captcha and save to 'filename'
         logger.debug('generate_captcha: server answered: %r', captcha)
@@ -147,6 +150,8 @@
             msg = 'generate_captcha crashed while downloading the image.'
             logger.exception(msg)
             raise
+        finally:
+            wc.shutdown()
 
         defer.returnValue(captcha['captcha_id'])
 
@@ -158,19 +163,22 @@
                      'displayname: %r, captcha_id: %r, captcha_solution: %r',
                      email, displayname, captcha_id, captcha_solution)
         restful_client = restful.RestfulClient(self.service_url)
-        if not self._valid_email(email):
-            logger.error('register_user: InvalidEmailError for email: %r',
-                         email)
-            raise InvalidEmailError()
-        if not self._valid_password(password):
-            logger.error('register_user: InvalidPasswordError')
-            raise InvalidPasswordError()
+        try:
+            if not self._valid_email(email):
+                logger.error('register_user: InvalidEmailError for email: %r',
+                             email)
+                raise InvalidEmailError()
+            if not self._valid_password(password):
+                logger.error('register_user: InvalidPasswordError')
+                raise InvalidPasswordError()
 
-        result = yield restful_client.restcall(u"registration.register",
-                    email=email, password=password,
-                    displayname=displayname,
-                    captcha_id=captcha_id,
-                    captcha_solution=captcha_solution)
+            result = yield restful_client.restcall(u"registration.register",
+                        email=email, password=password,
+                        displayname=displayname,
+                        captcha_id=captcha_id,
+                        captcha_solution=captcha_solution)
+        finally:
+            restful_client.shutdown()
         logger.info('register_user: email: %r result: %r', email, result)
 
         if result['status'].lower() == SSO_STATUS_ERROR:
@@ -196,6 +204,8 @@
         except WebClientError:
             logger.exception('login failed with:')
             raise AuthenticationError()
+        finally:
+            restful_client.shutdown()
 
         logger.debug('login: authentication successful! consumer_key: %r, ' \
                      'token_name: %r', credentials['consumer_key'], token_name)
@@ -207,7 +217,10 @@
         logger.debug('is_validated: requesting accounts.me() info.')
         restful_client = restful.RestfulClient(self.service_url,
                                                oauth_credentials=token)
-        me_info = yield restful_client.restcall(u"accounts.me")
+        try:
+            me_info = yield restful_client.restcall(u"accounts.me")
+        finally:
+            restful_client.shutdown()
         key = 'preferred_email'
         result = key in me_info and me_info[key] != None
 
@@ -225,8 +238,11 @@
                                        token_name=token_name)
         restful_client = restful.RestfulClient(self.service_url,
                                                oauth_credentials=credentials)
-        result = yield restful_client.restcall(u"accounts.validate_email",
-                                               email_token=email_token)
+        try:
+            result = yield restful_client.restcall(u"accounts.validate_email",
+                                                   email_token=email_token)
+        finally:
+            restful_client.shutdown()
         logger.info('validate_email: email: %r result: %r', email, result)
         if 'errors' in result:
             errorsdict = self._format_webservice_errors(result['errors'])
@@ -246,6 +262,8 @@
         except WebClientError, e:
             logger.exception('request_password_reset_token failed with:')
             raise ResetPasswordTokenError(e[1].split('\n')[0])
+        finally:
+            restful_client.shutdown()
 
         if result['status'].lower() == SSO_STATUS_OK:
             defer.returnValue(email)
@@ -270,6 +288,8 @@
         except WebClientError, e:
             logger.exception('set_new_password failed with:')
             raise NewPasswordError(e[1].split('\n')[0])
+        finally:
+            restful_client.shutdown()
 
         if result['status'].lower() == SSO_STATUS_OK:
             defer.returnValue(email)

=== modified file 'ubuntu_sso/credentials.py'
--- ubuntu_sso/credentials.py	2012-01-10 17:28:08 +0000
+++ ubuntu_sso/credentials.py	2012-02-03 17:25:21 +0000
@@ -39,15 +39,15 @@
 
 import sys
 import traceback
-import urllib2
 
 from functools import wraps
 
 from twisted.internet import defer
 
-from ubuntu_sso import NO_OP, utils
+from ubuntu_sso import NO_OP
 from ubuntu_sso.keyring import Keyring
 from ubuntu_sso.logger import setup_logging
+from ubuntu_sso.utils import webclient
 
 logger = setup_logging('ubuntu_sso.credentials')
 
@@ -216,6 +216,7 @@
         self.denial_cb(app_name)
 
     @handle_failures(msg='Problem opening the ping_url')
+    @defer.inlineCallbacks
     def _ping_url(self, app_name, email, credentials):
         """Ping the self.ping_url with the email attached.
 
@@ -235,16 +236,12 @@
                          'appending email (%r).', self.ping_url, email)
             url = self.ping_url + email
 
-        headers = utils.oauth_headers(url, credentials)
-        request = urllib2.Request(url, headers=headers)
-        logger.debug('Opening the url %r with urllib2.urlopen.',
-                     request.get_full_url())
-        # This code is blocking, we should change this.
-        # I've tried with deferToThread an twisted.web.client.getPage
-        # but the returned deferred will never be fired (nataliabidart).
-        response = urllib2.urlopen(request)
-        logger.debug('Url opened. Response: %s.', response.code)
-        return defer.succeed(response)
+        wc = webclient.webclient_factory()
+        try:
+            response = yield wc.request(url, oauth_credentials=credentials)
+            defer.returnValue(response)
+        finally:
+            wc.shutdown()
 
     @handle_exceptions(msg='Problem opening the Ubuntu SSO user interface')
     def _show_ui(self, login_only):

=== modified file 'ubuntu_sso/tests/test_account.py'
--- ubuntu_sso/tests/test_account.py	2012-01-26 18:52:23 +0000
+++ ubuntu_sso/tests/test_account.py	2012-02-03 17:25:21 +0000
@@ -82,16 +82,30 @@
 class FakeWebClient(object):
     """A fake webclient."""
 
+    def __init__(self):
+        self.started = True
+
     def request(self, url):
         """Do a fake request, return a fake Response."""
         return FakeWebClientResponse()
 
+    def shutdown(self):
+        """Turn off this webclient."""
+        self.started = False
+
 
 class FakeRestfulClient(object):
     """A fake restfulclient."""
 
     preferred_email = EMAIL
 
+    def __init__(self):
+        self.started = True
+
+    def shutdown(self):
+        """Stop this restfulclient."""
+        self.started = False
+
     def fake_captchas_new(self):
         """Return a local fake captcha."""
         return FAKE_NEW_CAPTCHA
@@ -231,6 +245,11 @@
         self.frc = FakeRestfulClient()
         self.patch(account.restful, "RestfulClient",
                    lambda *args, **kwargs: self.frc)
+        self.addCleanup(self.verify_frc_shutdown)
+
+    def verify_frc_shutdown(self):
+        """Verify that the FakeRestfulClient was stopped."""
+        assert self.frc.started == False, "Restfulclient must be shut down."
 
     @defer.inlineCallbacks
     def tearDown(self):
@@ -244,7 +263,8 @@
         filename = self.mktemp()
         self.addCleanup(lambda: os.remove(filename)
                                 if os.path.exists(filename) else None)
-        self.patch(account.webclient, "webclient_factory", FakeWebClient)
+        wc = FakeWebClient()
+        self.patch(account.webclient, "webclient_factory", lambda: wc)
         captcha_id = yield self.processor.generate_captcha(filename)
         self.assertEqual(CAPTCHA_ID, captcha_id, 'captcha id must be correct.')
         self.assertTrue(os.path.isfile(filename), '%s must exist.' % filename)
@@ -254,6 +274,7 @@
         with open(filename) as f:
             actual = f.read()
         self.assertEqual(expected, actual, 'captcha image must be correct.')
+        self.assertFalse(wc.started, "Webclient must be shut down.")
 
     @defer.inlineCallbacks
     def test_register_user_checks_valid_email(self):

=== modified file 'ubuntu_sso/tests/test_credentials.py'
--- ubuntu_sso/tests/test_credentials.py	2011-12-21 21:38:15 +0000
+++ ubuntu_sso/tests/test_credentials.py	2012-02-03 17:25:21 +0000
@@ -16,12 +16,9 @@
 """Tests for the Credentials module."""
 
 import logging
-import os
-import time
-import urllib2
 
 from twisted.internet import defer
-from twisted.trial.unittest import TestCase, FailTest
+from twisted.trial.unittest import TestCase
 from ubuntuone.devtools.handlers import MementoHandler
 
 from ubuntu_sso import credentials
@@ -293,8 +290,11 @@
         self.patch(credentials.Keyring, 'get_credentials',
                    lambda kr, app: defer.succeed(TOKEN))
         error = 'Bla'
-        self.patch(credentials.urllib2, 'urlopen',
-                   lambda *a, **kw: self.fail(error))
+        exception = SampleMiscException(error)
+        failing_request = lambda *args, **kwargs: defer.fail(exception)
+        self.patch(credentials.webclient.webclient_module().WebClient,
+                   "request", failing_request)
+
         self._cred_cleared = False
         self.patch(self.obj, 'clear_credentials',
                    lambda: setattr(self, '_cred_cleared', True))
@@ -303,14 +303,15 @@
 
         # error cb called correctly
         msg = 'Problem opening the ping_url'
-        self.assert_error_cb_called(msg=msg, detailed_error=FailTest(error))
+        self.assert_error_cb_called(msg=msg, detailed_error=exception)
         self.assertEqual(result, None)
 
         # credentials cleared
         self.assertTrue(self._cred_cleared)
 
         # exception logged
-        self.assertTrue(self.memento.check_exception(FailTest, error))
+        self.assertTrue(self.memento.check_exception(SampleMiscException,
+                                                     error))
 
     @defer.inlineCallbacks
     def test_pings_url(self):
@@ -356,31 +357,35 @@
         self.assertEqual(self._called, (('denial', APP_NAME), {}))
 
 
+class FakeWebclient(object):
+    """A fake webclient."""
+
+    def __init__(self):
+        self.called = []
+
+    def request(self, *args, **kwargs):
+        """Save a webclient request."""
+        self.called.append((args, kwargs))
+        return "response"
+
+    def shutdown(self):
+        """Shutdown this fake webclient."""
+
+
 class PingUrlTestCase(CredentialsTestCase):
     """Test suite for the URL pinging."""
 
     @defer.inlineCallbacks
     def setUp(self):
         yield super(PingUrlTestCase, self).setUp()
-        self._request = None
-
-        def faked_urlopen(request):
-            """Fake urlopener."""
-            self._request = request
-            response = urllib2.addinfourl(fp=open(os.path.devnull),
-                                          headers=request.headers,
-                                          url=request.get_full_url(),
-                                          code=200)
-            return response
-
-        self.patch(credentials.urllib2, 'urlopen', faked_urlopen)
-        self.patch(credentials.utils.timestamp_checker, "get_faithful_time",
-                   time.time)
+        self.wc = FakeWebclient()
+        self.patch(credentials.webclient, "webclient_factory",
+                   lambda *args: self.wc)
 
     @defer.inlineCallbacks
     def test_ping_url_if_url_is_none(self):
         """self.ping_url is opened."""
-        self.patch(credentials.urllib2, 'urlopen', self.fail)
+        self.patch(self.wc, 'request', lambda *args, **kwargs: defer.fail())
         self.obj.ping_url = None
         yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
                                  credentials=TOKEN)
@@ -392,77 +397,78 @@
         yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
                                  credentials=TOKEN)
 
-        self.assertIsInstance(self._request, credentials.urllib2.Request)
-        self.assertEqual(self._request.get_full_url(),
-                         self.obj.ping_url + EMAIL)
+        args, _kwargs = self.wc.called[0]
+        self.assertEqual(args[0], self.obj.ping_url + EMAIL)
 
     @defer.inlineCallbacks
     def test_request_is_signed_with_credentials(self):
         """The http request to self.ping_url is signed with the credentials."""
-
-        def fake_it(*a, **kw):
-            """Fake oauth_headers."""
-            self._set_called(*a, **kw)
-            return {}
-
-        self.patch(credentials.utils, 'oauth_headers', fake_it)
-        result = yield self.obj._ping_url(APP_NAME, EMAIL, TOKEN)
-
-        self.assertEqual(self._called,
-                         ((self.obj.ping_url + EMAIL, TOKEN), {}))
-        self.assertEqual(result.code, 200)
+        yield self.obj._ping_url(APP_NAME, EMAIL, TOKEN)
+
+        _args, kwargs = self.wc.called[0]
+        self.assertEqual(kwargs["oauth_credentials"], TOKEN)
 
     @defer.inlineCallbacks
     def test_ping_url_error(self):
         """Exception is handled if ping fails."""
         error = 'Blu'
-        self.patch(credentials.urllib2, 'urlopen', lambda r: self.fail(error))
-
+        exception = SampleMiscException(error)
+        self.patch(self.wc, "request",
+                   lambda *args, **kwargs: defer.fail(exception))
         yield self.obj._ping_url(APP_NAME, EMAIL, TOKEN)
 
         msg = 'Problem opening the ping_url'
-        self.assert_error_cb_called(msg=msg, detailed_error=FailTest(error))
-        self.assertTrue(self.memento.check_exception(FailTest, error))
+        self.assert_error_cb_called(msg=msg, detailed_error=exception)
+        self.assertTrue(self.memento.check_exception(SampleMiscException,
+                                                     error))
 
     @defer.inlineCallbacks
     def test_ping_url_formatting(self):
         """The email is added as the first formatting argument."""
         self.obj.ping_url = u'http://example.com/{email}/something/else'
-        result = yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
+        yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
                                           credentials=TOKEN)
 
         expected = self.obj.ping_url.format(email=EMAIL)
-        self.assertEqual(expected, result.url)
+        assert len(self.wc.called) > 0
+        args, _kwargs = self.wc.called[0]
+        self.assertEqual(expected, args[0])
 
     @defer.inlineCallbacks
     def test_ping_url_formatting_with_query_params(self):
         """The email is added as the first formatting argument."""
         self.obj.ping_url = u'http://example.com/{email}?something=else'
-        result = yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
-                                          credentials=TOKEN)
+        yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
+                                 credentials=TOKEN)
 
         expected = self.obj.ping_url.format(email=EMAIL)
-        self.assertEqual(expected, result.url)
+        assert len(self.wc.called) > 0
+        args, _kwargs = self.wc.called[0]
+        self.assertEqual(expected, args[0])
 
     @defer.inlineCallbacks
     def test_ping_url_formatting_no_email_kwarg(self):
         """The email is added as the first formatting argument."""
         self.obj.ping_url = u'http://example.com/{0}/yadda/?something=else'
-        result = yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
-                                          credentials=TOKEN)
+        yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
+                                 credentials=TOKEN)
 
         expected = self.obj.ping_url.format(EMAIL)
-        self.assertEqual(expected, result.url)
+        assert len(self.wc.called) > 0
+        args, _kwargs = self.wc.called[0]
+        self.assertEqual(expected, args[0])
 
     @defer.inlineCallbacks
     def test_ping_url_formatting_no_format(self):
         """The email is appended if formatting could not be accomplished."""
         self.obj.ping_url = u'http://example.com/yadda/'
-        result = yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
-                                          credentials=TOKEN)
+        yield self.obj._ping_url(app_name=APP_NAME, email=EMAIL,
+                                 credentials=TOKEN)
 
         expected = self.obj.ping_url + EMAIL
-        self.assertEqual(expected, result.url)
+        assert len(self.wc.called) > 0
+        args, _kwargs = self.wc.called[0]
+        self.assertEqual(expected, args[0])
 
 
 class FindCredentialsTestCase(CredentialsTestCase):

=== modified file 'ubuntu_sso/utils/webclient/common.py'
--- ubuntu_sso/utils/webclient/common.py	2012-01-27 20:05:41 +0000
+++ ubuntu_sso/utils/webclient/common.py	2012-02-03 17:25:21 +0000
@@ -90,7 +90,7 @@
     def get_timestamp_checker(cls):
         """Get the timestamp checker for this class of webclient."""
         if cls.timestamp_checker is None:
-            cls.timestamp_checker = TimestampChecker(cls())
+            cls.timestamp_checker = TimestampChecker(cls)
         return cls.timestamp_checker
 
     def get_timestamp(self):

=== modified file 'ubuntu_sso/utils/webclient/restful.py'
--- ubuntu_sso/utils/webclient/restful.py	2012-01-25 15:02:50 +0000
+++ ubuntu_sso/utils/webclient/restful.py	2012-02-03 17:25:21 +0000
@@ -59,3 +59,7 @@
                                               post_content=encoded_args,
                                               extra_headers=POST_HEADERS)
         defer.returnValue(json.loads(result.content))
+
+    def shutdown(self):
+        """Stop the webclient used by this class."""
+        self.webclient.shutdown()

=== modified file 'ubuntu_sso/utils/webclient/tests/__init__.py'
--- ubuntu_sso/utils/webclient/tests/__init__.py	2011-12-15 16:16:37 +0000
+++ ubuntu_sso/utils/webclient/tests/__init__.py	2012-02-03 17:25:21 +0000
@@ -15,3 +15,63 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 """Tests for the proxy-aware webclient."""
+
+from twisted.application import internet, service
+from twisted.web import http, server
+
+
+class SaveHTTPChannel(http.HTTPChannel):
+    """A save protocol to be used in tests."""
+
+    protocolInstance = None
+
+    # pylint: disable=C0103
+    def connectionMade(self):
+        """Keep track of the given protocol."""
+        SaveHTTPChannel.protocolInstance = self
+        http.HTTPChannel.connectionMade(self)
+
+
+class SaveSite(server.Site):
+    """A site that let us know when it closed."""
+
+    protocol = SaveHTTPChannel
+
+    def __init__(self, *args, **kwargs):
+        """Create a new instance."""
+        server.Site.__init__(self, *args, **kwargs)
+        # we disable the timeout in the tests, we will deal with it manually.
+        # pylint: disable=C0103
+        self.timeOut = None
+
+
+class BaseMockWebServer(object):
+    """A mock webserver for testing"""
+
+    def __init__(self):
+        """Start up this instance."""
+        self.root = self.get_root_resource()
+        self.site = SaveSite(self.root)
+        application = service.Application('web')
+        self.service_collection = service.IServiceCollection(application)
+        #pylint: disable=E1101
+        self.tcpserver = internet.TCPServer(0, self.site)
+        self.tcpserver.setServiceParent(self.service_collection)
+        self.service_collection.startService()
+
+    def get_root_resource(self):
+        """Get the root resource with all the children."""
+        raise NotImplementedError
+
+    def get_iri(self):
+        """Build the iri for this mock server."""
+        #pylint: disable=W0212
+        port_num = self.tcpserver._port.getHost().port
+        return u"http://127.0.0.1:%d/" % port_num
+
+    def stop(self):
+        """Shut it down."""
+        #pylint: disable=E1101
+        if self.site.protocol.protocolInstance:
+            self.site.protocol.protocolInstance.timeoutConnection()
+        return self.service_collection.stopService()

=== modified file 'ubuntu_sso/utils/webclient/tests/test_restful.py'
--- ubuntu_sso/utils/webclient/tests/test_restful.py	2012-01-25 15:02:50 +0000
+++ ubuntu_sso/utils/webclient/tests/test_restful.py	2012-02-03 17:25:21 +0000
@@ -43,12 +43,17 @@
         self.return_value = SAMPLE_RESPONSE
         self.called = []
         self.init_kwargs = kwargs
+        self.running = True
 
     def request(self, iri, *args, **kwargs):
         """Return a deferred that will be fired with a Response object."""
         self.called.append((iri, args, kwargs))
         return defer.succeed(Response(self.return_value))
 
+    def shutdown(self):
+        """Stop this fake webclient."""
+        self.running = False
+
 
 class BaseTestCase(TestCase):
     """The base for the Restful Client testcases."""
@@ -75,11 +80,18 @@
         """Initialize this testcase."""
         yield super(RestfulClientTestCase, self).setUp()
         self.rc = restful.RestfulClient(SAMPLE_SERVICE_IRI)
+        self.addCleanup(self.rc.shutdown)
 
     def test_has_a_webclient(self):
         """The RC has a webclient."""
         self.assertEqual(self.rc.webclient, self.wc)
 
+    def test_shutsdown_the_webclient(self):
+        """Calling shutdown on the restful shuts down the webclient too."""
+        self.rc.shutdown()
+        # pylint: disable=E1101
+        self.assertFalse(self.rc.webclient.running, "The webclient is stopped")
+
     @defer.inlineCallbacks
     def test_can_make_calls(self):
         """The RC can make webcalls."""

=== modified file 'ubuntu_sso/utils/webclient/tests/test_timestamp.py'
--- ubuntu_sso/utils/webclient/tests/test_timestamp.py	2012-01-19 00:39:05 +0000
+++ ubuntu_sso/utils/webclient/tests/test_timestamp.py	2012-02-03 17:25:21 +0000
@@ -15,12 +15,12 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 """Tests for the timestamp sync classes."""
 
-from twisted.application import internet, service
 from twisted.internet import defer
 from twisted.trial.unittest import TestCase
-from twisted.web import server, resource
+from twisted.web import resource
 
-from ubuntu_sso.utils.webclient import timestamp, webclient_factory
+from ubuntu_sso.utils.webclient import timestamp, webclient_module
+from ubuntu_sso.utils.webclient.tests import BaseMockWebServer
 
 
 class FakedError(Exception):
@@ -46,61 +46,45 @@
         return ""
 
 
-class MockWebServer(object):
+class MockWebServer(BaseMockWebServer):
     """A mock webserver for testing."""
 
-    # pylint: disable=E1101
-    def __init__(self):
-        """Start up this instance."""
-        self.root = RootResource()
-        site = server.Site(self.root)
-        application = service.Application('web')
-        self.service_collection = service.IServiceCollection(application)
-        self.tcpserver = internet.TCPServer(0, site)
-        self.tcpserver.setServiceParent(self.service_collection)
-        self.service_collection.startService()
-
-    def get_iri(self):
-        """Build the url for this mock server."""
-        # pylint: disable=W0212
-        port_num = self.tcpserver._port.getHost().port
-        return u"http://localhost:%d/" % port_num
-
-    def stop(self):
-        """Shut it down."""
-        self.service_collection.stopService()
+    def get_root_resource(self):
+        """Get the root resource with all the children."""
+        return RootResource()
 
 
 class TimestampCheckerTestCase(TestCase):
     """Tests for the timestamp checker."""
 
+    timeout = 1
+
     @defer.inlineCallbacks
     def setUp(self):
         yield super(TimestampCheckerTestCase, self).setUp()
         self.ws = MockWebServer()
         self.addCleanup(self.ws.stop)
-        self.wc = webclient_factory()
-        self.addCleanup(self.wc.shutdown)
+        self.webclient_class = webclient_module().WebClient
         self.patch(timestamp.TimestampChecker, "SERVER_IRI", self.ws.get_iri())
 
     @defer.inlineCallbacks
     def test_returned_value_is_int(self):
         """The returned value is an integer."""
-        checker = timestamp.TimestampChecker(self.wc)
+        checker = timestamp.TimestampChecker(self.webclient_class)
         result = yield checker.get_faithful_time()
         self.assertEqual(type(result), int)
 
     @defer.inlineCallbacks
     def test_first_call_does_head(self):
         """The first call gets the clock from our web."""
-        checker = timestamp.TimestampChecker(self.wc)
+        checker = timestamp.TimestampChecker(self.webclient_class)
         yield checker.get_faithful_time()
         self.assertEqual(self.ws.root.count, 1)
 
     @defer.inlineCallbacks
     def test_second_call_is_cached(self):
         """For the second call, the time is cached."""
-        checker = timestamp.TimestampChecker(self.wc)
+        checker = timestamp.TimestampChecker(self.webclient_class)
         yield checker.get_faithful_time()
         yield checker.get_faithful_time()
         self.assertEqual(self.ws.root.count, 1)
@@ -110,7 +94,7 @@
         """After some time, the cache expires."""
         fake_timestamp = 1
         self.patch(timestamp.time, "time", lambda: fake_timestamp)
-        checker = timestamp.TimestampChecker(self.wc)
+        checker = timestamp.TimestampChecker(self.webclient_class)
         yield checker.get_faithful_time()
         fake_timestamp += timestamp.TimestampChecker.CHECKING_INTERVAL
         yield checker.get_faithful_time()
@@ -121,7 +105,7 @@
         """When server can't be reached, the skew is not updated."""
         fake_timestamp = 1
         self.patch(timestamp.time, "time", lambda: fake_timestamp)
-        checker = timestamp.TimestampChecker(self.wc)
+        checker = timestamp.TimestampChecker(self.webclient_class)
         failing_get_server_time = lambda _: defer.fail(FakedError())
         self.patch(checker, "get_server_time", failing_get_server_time)
         yield checker.get_faithful_time()
@@ -132,7 +116,7 @@
     @defer.inlineCallbacks
     def test_server_date_sends_nocache_headers(self):
         """Getting the server date sends the no-cache headers."""
-        checker = timestamp.TimestampChecker(self.wc)
+        checker = timestamp.TimestampChecker(self.webclient_class)
         yield checker.get_server_date_header(self.ws.get_iri())
         self.assertEqual(len(self.ws.root.request_headers), 1)
         headers = self.ws.root.request_headers[0]

=== modified file 'ubuntu_sso/utils/webclient/tests/test_webclient.py'
--- ubuntu_sso/utils/webclient/tests/test_webclient.py	2012-02-01 19:21:28 +0000
+++ ubuntu_sso/utils/webclient/tests/test_webclient.py	2012-02-03 17:25:21 +0000
@@ -19,10 +19,9 @@
 import sys
 import urllib
 
-from twisted.application import internet, service
 from twisted.cred import checkers, portal
 from twisted.internet import defer
-from twisted.web import http, resource, server, guard
+from twisted.web import guard, http, resource
 from urllib2 import urlparse
 
 from ubuntuone.devtools.testcases import TestCase
@@ -30,6 +29,7 @@
 
 from ubuntu_sso.utils import webclient
 from ubuntu_sso.utils.webclient.common import BaseWebClient, HeaderDict
+from ubuntu_sso.utils.webclient.tests import BaseMockWebServer
 
 ANY_VALUE = object()
 SAMPLE_KEY = "result"
@@ -148,34 +148,11 @@
         return "ERROR: Expected OAuth header not present."
 
 
-class SaveHTTPChannel(http.HTTPChannel):
-    """A save protocol to be used in tests."""
-
-    protocolInstance = None
-
-    def connectionMade(self):
-        """Keep track of the given protocol."""
-        SaveHTTPChannel.protocolInstance = self
-        http.HTTPChannel.connectionMade(self)
-
-
-class SaveSite(server.Site):
-    """A site that let us know when it closed."""
-
-    protocol = SaveHTTPChannel
-
-    def __init__(self, *args, **kwargs):
-        """Create a new instance."""
-        server.Site.__init__(self, *args, **kwargs)
-        # we disable the timeout in the tests, we will deal with it manually.
-        self.timeOut = None
-
-
-class MockWebServer(object):
-    """A mock webserver for testing"""
-
-    def __init__(self):
-        """Start up this instance."""
+class MockWebServer(BaseMockWebServer):
+    """A mock webserver for the webclient tests."""
+
+    def get_root_resource(self):
+        """Get the root resource with all the children."""
         root = resource.Resource()
         root.putChild(SIMPLERESOURCE, SimpleResource())
         root.putChild(POSTABLERESOURECE, PostableResource())
@@ -197,27 +174,7 @@
         guarded_resource = guard.HTTPAuthSessionWrapper(test_portal,
                                                         [cred_factory])
         root.putChild(GUARDED, guarded_resource)
-
-        self.site = SaveSite(root)
-        application = service.Application('web')
-        self.service_collection = service.IServiceCollection(application)
-        #pylint: disable=E1101
-        self.tcpserver = internet.TCPServer(0, self.site)
-        self.tcpserver.setServiceParent(self.service_collection)
-        self.service_collection.startService()
-
-    def get_iri(self):
-        """Build the iri for this mock server."""
-        #pylint: disable=W0212
-        port_num = self.tcpserver._port.getHost().port
-        return u"http://127.0.0.1:%d/" % port_num
-
-    def stop(self):
-        """Shut it down."""
-        #pylint: disable=E1101
-        if self.site.protocol.protocolInstance:
-            self.site.protocol.protocolInstance.timeoutConnection()
-        return self.service_collection.stopService()
+        return root
 
 
 class FakeReactor(object):
@@ -261,7 +218,7 @@
 class WebClientTestCase(TestCase):
     """Test for the webclient."""
 
-    timeout = 8
+    timeout = 1
 
     @defer.inlineCallbacks
     def setUp(self):
@@ -271,8 +228,6 @@
         self.base_iri = self.ws.get_iri()
         self.wc = webclient.webclient_factory()
         self.addCleanup(self.wc.shutdown)
-        # pylint: disable=W0511
-        # TODO: skewed timestamp correction
 
     @defer.inlineCallbacks
     def test_request_takes_an_iri(self):
@@ -343,6 +298,8 @@
     @defer.inlineCallbacks
     def test_request_is_oauth_signed(self):
         """The request is oauth signed."""
+        tsc = self.wc.get_timestamp_checker()
+        self.patch(tsc, "get_faithful_time", lambda: defer.succeed(1))
         result = yield self.wc.request(self.base_iri + OAUTHRESOURCE,
                                        oauth_credentials=SAMPLE_CREDENTIALS)
         self.assertEqual(SAMPLE_RESOURCE, result.content)
@@ -366,15 +323,14 @@
     @defer.inlineCallbacks
     def test_returned_content_are_bytes(self):
         """The returned content are bytes."""
+        tsc = self.wc.get_timestamp_checker()
+        self.patch(tsc, "get_faithful_time", lambda: defer.succeed(1))
         result = yield self.wc.request(self.base_iri + OAUTHRESOURCE,
                                        oauth_credentials=SAMPLE_CREDENTIALS)
         self.assertTrue(isinstance(result.content, bytes),
                         "The type of %r must be bytes" % result.content)
 
 
-WebClientTestCase.skip = 'Tests failing, see bug #920591.'
-
-
 class TimestampCheckerTestCase(TestCase):
     """Tests for the timestampchecker classmethod."""
 
@@ -388,12 +344,7 @@
     def test_timestamp_checker_has_the_same_class_as_the_creator(self):
         """The TimestampChecker has the same class."""
         tsc = self.wc.get_timestamp_checker()
-        self.assertEqual(tsc.webclient.__class__, self.wc.__class__)
-
-    def test_timestamp_checker_uses_different_webclient_than_the_creator(self):
-        """The TimestampChecker uses a different webclient than the creator."""
-        tsc = self.wc.get_timestamp_checker()
-        self.assertNotEqual(tsc.webclient, self.wc)
+        self.assertEqual(tsc.webclient_class, self.wc.__class__)
 
     def test_timestamp_checker_is_the_same_for_all_webclients(self):
         """The TimestampChecker is the same for all webclients."""

=== modified file 'ubuntu_sso/utils/webclient/timestamp.py'
--- ubuntu_sso/utils/webclient/timestamp.py	2012-01-25 15:02:50 +0000
+++ ubuntu_sso/utils/webclient/timestamp.py	2012-02-03 17:25:21 +0000
@@ -33,18 +33,22 @@
     ERROR_INTERVAL = 30  # in seconds
     SERVER_IRI = u"http://one.ubuntu.com/api/time"
 
-    def __init__(self, webclient):
+    def __init__(self, webclient_class):
         """Initialize this instance."""
         self.next_check = time.time()
         self.skew = 0
-        self.webclient = webclient
+        self.webclient_class = webclient_class
 
     @defer.inlineCallbacks
     def get_server_date_header(self, server_iri):
         """Get the server date using twisted webclient."""
-        response = yield self.webclient.request(server_iri, method="HEAD",
-                                                extra_headers=NOCACHE_HEADERS)
-        defer.returnValue(response.headers["Date"][0])
+        webclient = self.webclient_class()
+        try:
+            response = yield webclient.request(server_iri, method="HEAD",
+                                               extra_headers=NOCACHE_HEADERS)
+            defer.returnValue(response.headers["Date"][0])
+        finally:
+            webclient.shutdown()
 
     @defer.inlineCallbacks
     def get_server_time(self):