Merge lp:~nataliabidart/ubuntu-sso-client/no-quit-if-client into lp:ubuntu-sso-client

[Natalia Bidart]

- The service is not shutdown if ref count is not zero, even if the
  shutdown_func was queued (LP: #709200).

- Test failures (caused by a buggy test) fixed (LP: #704941).

[Roberto Alsina (ralsina)]

+1

review: Approve

[Alejandro J. Cura (alecu)]

The branch does what's supposed to do, but I think the timeouts should not be "ignored when they happen" but instead "cancelled when they are no longer needed"
I'll make a new bug for that.

review: Approve

[Alejandro J. Cura (alecu)]

The bug for shutdown timeouts is: #709301

=== modified file 'ubuntu_sso/main.py'
--- ubuntu_sso/main.py	2011-01-11 19:13:19 +0000
+++ ubuntu_sso/main.py	2011-01-28 13:50:13 +0000
@@ -50,7 +50,7 @@
 
 logger = setup_logging("ubuntu_sso.main")
 U1_PING_URL = "https://one.ubuntu.com/oauth/sso-finished-so-get-tokens/"
-TIMEOUT_INTERVAL = 500
+TIMEOUT_INTERVAL = 10000  # 10 seconds
 
 
 class SSOLoginProcessor(Account):
@@ -449,7 +449,6 @@
 
     def _get_ref_count(self):
         """Get value of ref_count."""
-        logger.debug('ref_count is %r.', self._ref_count)
         return self._ref_count
 
     def _set_ref_count(self, new_value):
@@ -464,10 +463,19 @@
             self._ref_count = new_value
 
         if self._ref_count == 0:
-            self.timeout_func(TIMEOUT_INTERVAL, self.shutdown_func)
+            logger.debug('Setting up timer with %r (%r, %r).',
+                         self.timeout_func, TIMEOUT_INTERVAL, self.shutdown)
+            self.timeout_func(TIMEOUT_INTERVAL, self.shutdown)
 
     ref_count = property(fget=_get_ref_count, fset=_set_ref_count)
 
+    def shutdown(self):
+        """If no ongoing requests, call self.shutdown_func."""
+        logger.debug('shutdown!, ref_count is %r.', self._ref_count)
+        if self._ref_count == 0:
+            logger.info('Shutting down, calling %r.', self.shutdown_func)
+            self.shutdown_func()
+
     @dbus.service.signal(DBUS_CREDENTIALS_IFACE, signature='s')
     def AuthorizationDenied(self, app_name):
         """Signal thrown when the user denies the authorization."""

=== modified file 'ubuntu_sso/tests/test_main.py'
--- ubuntu_sso/tests/test_main.py	2011-01-11 19:13:19 +0000
+++ ubuntu_sso/tests/test_main.py	2011-01-28 13:50:13 +0000
@@ -749,30 +749,68 @@
         self.assertIsInstance(self.client, ubuntu_sso.main.dbus.service.Object)
 
 
+class FakeCredentials(object):
+    """A very dummy Credentials object."""
+
+    def __init__(self, *a, **kw):
+        self.find_credentials = lambda *a: defer.succeed(TOKEN)
+        self.clear_credentials = lambda *a: defer.succeed(None)
+        self.store_credentials = lambda *a: defer.succeed(None)
+        self.login = self.register = lambda *a: None
+
+
 class CredentialsManagementRefCountingTestCase(CredentialsManagementTestCase):
     """Tests for the CredentialsManagement ref counting."""
 
+    def setUp(self):
+        super(CredentialsManagementRefCountingTestCase, self).setUp()
+        self.patch(ubuntu_sso.main, 'Credentials', FakeCredentials)
+
     def test_ref_counting(self):
         """Ref counting is in place."""
         self.assertEqual(self.client.ref_count, 0)
 
     def test_find_credentials(self):
         """Keep proper track of on going requests."""
+        d = Deferred()
+
+        def verify(*args):
+            """Make the check."""
+            self.assertEqual(self.client.ref_count, 1)
+            d.callback(True)
+
+        self.patch(self.client, 'CredentialsFound', verify)
         self.client.find_credentials(APP_NAME, self.args)
 
-        self.assertEqual(self.client.ref_count, 1)
+        return d
 
     def test_clear_credentials(self):
         """Keep proper track of on going requests."""
+        d = Deferred()
+
+        def verify(*args):
+            """Make the check."""
+            self.assertEqual(self.client.ref_count, 1)
+            d.callback(True)
+
+        self.patch(self.client, 'CredentialsCleared', verify)
         self.client.clear_credentials(APP_NAME, self.args)
 
-        self.assertEqual(self.client.ref_count, 1)
+        return d
 
     def test_store_credentials(self):
         """Keep proper track of on going requests."""
+        d = Deferred()
+
+        def verify(*args):
+            """Make the check."""
+            self.assertEqual(self.client.ref_count, 1)
+            d.callback(True)
+
+        self.patch(self.client, 'CredentialsStored', verify)
         self.client.store_credentials(APP_NAME, self.args)
 
-        self.assertEqual(self.client.ref_count, 1)
+        return d
 
     def test_register(self):
         """Keep proper track of on going requests."""
@@ -789,10 +827,10 @@
     def test_several_requests(self):
         """Requests can be nested."""
         self.client.login(APP_NAME, self.args)
-        self.client.clear_credentials(APP_NAME, self.args)
-        self.client.find_credentials(APP_NAME, self.args)
-        self.client.register(APP_NAME, self.args)
-        self.client.store_credentials(APP_NAME, self.args)
+        self.client.register(APP_NAME, self.args)
+        self.client.login(APP_NAME, self.args)
+        self.client.register(APP_NAME, self.args)
+        self.client.register(APP_NAME, self.args)
 
         self.assertEqual(self.client.ref_count, 5)
 
@@ -895,19 +933,54 @@
     def test_on_zero_ref_count_shutdown(self):
         """When ref count reaches 0, queue shutdown op."""
         self.client.timeout_func = self._set_called
-        self.client.find_credentials(APP_NAME, self.args)
+        self.client.login(APP_NAME, self.args)
         self.client.CredentialsFound(APP_NAME, TOKEN)
 
         self.assertEqual(self._called,
-                         ((TIMEOUT_INTERVAL, self.client.shutdown_func), {}))
+                         ((TIMEOUT_INTERVAL, self.client.shutdown), {}))
 
     def test_on_non_zero_ref_count_do_not_shutdown(self):
         """If ref count is not 0, do not queue shutdown op."""
         self.client.timeout_func = self._set_called
-        self.client.find_credentials(APP_NAME, self.args)
+        self.client.login(APP_NAME, self.args)
 
         self.assertEqual(self._called, False)
 
+    def test_on_non_zero_ref_count_after_zero_do_not_shutdown(self):
+        """If the shutdown was queued, do not quit if counter is not zero."""
+
+        def fake_timeout_func(interval, func):
+            """Start a new request when the timer is started."""
+            self.client.register(APP_NAME, self.args)
+            assert self.client.ref_count > 0
+            func()
+
+        self.client.timeout_func = fake_timeout_func
+        self.client.shutdown_func = self._set_called
+
+        self.client.login(APP_NAME, self.args)
+        self.client.CredentialsFound(APP_NAME, TOKEN)
+        # counter reached 0, timeout_func was called
+
+        self.assertEqual(self._called, False, 'shutdown_func was not called')
+
+    def test_zero_ref_count_after_zero_do_shutdown(self):
+        """If the shutdown was queued, do quit if counter is zero."""
+
+        def fake_timeout_func(interval, func):
+            """Start a new request when the timer is started."""
+            assert self.client.ref_count == 0
+            func()
+
+        self.client.timeout_func = fake_timeout_func
+        self.client.shutdown_func = self._set_called
+
+        self.client.login(APP_NAME, self.args)
+        self.client.CredentialsFound(APP_NAME, TOKEN)
+        # counter reached 0, timeout_func was called
+
+        self.assertEqual(self._called, ((), {}), 'shutdown_func was called')
+
 
 class CredentialsManagementFindTestCase(CredentialsManagementTestCase):
     """Tests for the CredentialsManagement find method."""