Merge lp:~diegosarmentero/ubuntu-sso-client/remove-disconnect-signal into lp:ubuntu-sso-client

[Diego Sarmentero]

- Delete signal removal to keep listening to network state changes (LP: #920591).

Maybe you will have in windows a trace where some tests related to test_webclient might be failing, this is happening also in trunk (and seems to be a timing issue):

https://bugs.launchpad.net/ubuntu-sso-client/+bug/920591

[Roberto Alsina (ralsina)]

+1

review: Approve

[Natalia Bidart (nataliabidart)]

Branch looks mostly good, but I have some suggestions to improve the test readability. ATM, since the fake connect_to_siganl is returning self, it makes difficult to notice that dbus signals are also being faked by the same FakeDbusModule. Also, this is self.DBusException = self very complex to understand... why are you mocking an exception with a FakeDbusModule?

I advice not to mix fakes in this way, since future debugging may take longer and will be difficult to follow. Also, fakes and mocks should have an almost identical API with the object they are faking, so, for example, connect_to_signal should expect a siganl name and a callback, and it should return a different object than self, which has a certain API (has a remove method, for example). The fact that a test (that uses a fake with the correct API) pass, ensures more correctness in the production code.

So, below a proposal to improve the tests:
http://pastebin.ubuntu.com/816828/

review: Needs Fixing

[lp:~diegosarmentero/ubuntu-sso-client/remove-disconnect-signal]

838. By Diego Sarmentero on 2012-01-26

Improving Tests

[Diego Sarmentero (diegosarmentero)]

> Branch looks mostly good, but I have some suggestions to improve the test
> readability. ATM, since the fake connect_to_siganl is returning self, it makes
> difficult to notice that dbus signals are also being faked by the same
> FakeDbusModule. Also, this is self.DBusException = self very complex to
> understand... why are you mocking an exception with a FakeDbusModule?
>
> I advice not to mix fakes in this way, since future debugging may take longer
> and will be difficult to follow. Also, fakes and mocks should have an almost
> identical API with the object they are faking, so, for example,
> connect_to_signal should expect a siganl name and a callback, and it should
> return a different object than self, which has a certain API (has a remove
> method, for example). The fact that a test (that uses a fake with the correct
> API) pass, ensures more correctness in the production code.
>
> So, below a proposal to improve the tests:
> http://pastebin.ubuntu.com/816828/

Thanks for the suggestion, the tests has been improved.

[Natalia Bidart (nataliabidart)]

Looks good!

review: Approve

=== modified file 'ubuntu_sso/networkstate/linux.py'
--- ubuntu_sso/networkstate/linux.py	2011-11-11 19:20:59 +0000
+++ ubuntu_sso/networkstate/linux.py	2012-01-26 12:32:24 +0000
@@ -78,8 +78,6 @@
 
     def call_result_cb(self, state):
         """Return the state thru the result callback."""
-        if self.state_signal:
-            self.state_signal.remove()
         self.result_cb(state)
 
     def got_state(self, state):

=== modified file 'ubuntu_sso/networkstate/tests/test_linux.py'
--- ubuntu_sso/networkstate/tests/test_linux.py	2011-11-11 19:20:59 +0000
+++ ubuntu_sso/networkstate/tests/test_linux.py	2012-01-26 12:32:24 +0000
@@ -4,7 +4,7 @@
 #
 # Author: Alejandro J. Cura <[e-mail]>
 #
-# Copyright 2010 Canonical Ltd.
+# Copyright 2010-2012 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it
 # under the terms of the GNU General Public License version 3, as published
@@ -19,29 +19,35 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 """Tests for the network state detection code."""
 
+from collections import defaultdict
+
 from twisted.internet.defer import inlineCallbacks
+from mocker import ARGS, KWARGS, ANY, MockerTestCase
 
 from ubuntu_sso.tests import TestCase
-from ubuntu_sso.networkstate import (NetworkManagerState,
-                                     ONLINE, OFFLINE, UNKNOWN)
+from ubuntu_sso.networkstate import (
+    linux,
+    NetworkFailException,
+    NetworkManagerState,
+    ONLINE, OFFLINE, UNKNOWN,
+)
 
-from ubuntu_sso.networkstate import NetworkFailException
-from ubuntu_sso.networkstate import linux
 from ubuntu_sso.networkstate.linux import  (is_machine_connected,
-                                            DBUS_UNKNOWN_SERVICE,
-                                            NM_STATE_ASLEEP,
-                                            NM_STATE_ASLEEP_OLD,
-                                            NM_STATE_CONNECTING,
-                                            NM_STATE_CONNECTING_OLD,
-                                            NM_STATE_CONNECTED_OLD,
-                                            NM_STATE_CONNECTED_LOCAL,
-                                            NM_STATE_CONNECTED_SITE,
-                                            NM_STATE_CONNECTED_GLOBAL,
-                                            NM_STATE_DISCONNECTED,
-                                            NM_STATE_DISCONNECTED_OLD,
-                                            NM_STATE_UNKNOWN)
-
-from mocker import ARGS, KWARGS, ANY, MockerTestCase
+    DBUS_UNKNOWN_SERVICE,
+    NM_DBUS_INTERFACE,
+    NM_DBUS_OBJECTPATH,
+    NM_STATE_ASLEEP,
+    NM_STATE_ASLEEP_OLD,
+    NM_STATE_CONNECTING,
+    NM_STATE_CONNECTING_OLD,
+    NM_STATE_CONNECTED_OLD,
+    NM_STATE_CONNECTED_LOCAL,
+    NM_STATE_CONNECTED_SITE,
+    NM_STATE_CONNECTED_GLOBAL,
+    NM_STATE_DISCONNECTED,
+    NM_STATE_DISCONNECTED_OLD,
+    NM_STATE_UNKNOWN,
+)
 
 
 class TestException(Exception):
@@ -66,6 +72,57 @@
         self.call_function(self.connection_state)
 
 
+class FakeDBusMatch(object):
+
+    """Fake a DBus match."""
+
+    def __init__(self, name, callback, interface):
+        self.name = name
+        self.callback = callback
+        self.interface = interface
+        self.removed = False
+
+    def remove(self):
+        """Stop calling the handler function on remove."""
+        self.removed = True
+
+
+class FakeDBusInterface(object):
+
+    """Fake DBus Interface."""
+
+    def __init__(self):
+        self._signals = defaultdict(list)
+
+    # pylint: disable=C0103
+    def Get(self, *args, **kwargs):
+        """Fake Get."""
+    # pylint: enable=C0103
+
+    def connect_to_signal(self, signal_name, handler_function, dbus_interface):
+        """Fake connect_to_signal."""
+        match = FakeDBusMatch(signal_name, handler_function, dbus_interface)
+        self._signals[signal_name].append(match)
+        return match
+
+    def emit_signal(self, signal_name, state):
+        """Emit signal for network state change."""
+        for match in self._signals[signal_name]:
+            match.callback(state)
+
+
+class FakeSystemBus(object):
+
+    """Fake SystemBus."""
+
+    objects = {(NM_DBUS_INTERFACE, NM_DBUS_OBJECTPATH, True): object()}
+
+    def get_object(self, interface, object_path, follow_name_owner_changes):
+        """Fake get_object."""
+        key = (interface, object_path, follow_name_owner_changes)
+        return self.objects[key]
+
+
 class TestConnection(TestCase):
 
     """Test the state of the connection.
@@ -79,6 +136,34 @@
         """Setup the mocker dbus object tree."""
         yield super(TestConnection, self).setUp()
         self.patch(linux, "NetworkManagerState", FakeNetworkManagerState)
+        self.patch(linux.dbus, 'SystemBus', FakeSystemBus)
+        self.nm_interface = FakeDBusInterface()
+        self.patch(linux.dbus, 'Interface', lambda *a: self.nm_interface)
+        self.network_changes = []
+
+    def _listen_network_changes(self, state):
+        """Fake callback function."""
+        self.network_changes.append(state)
+
+    def test_network_state_change(self):
+        """Test the changes in the network connection."""
+        nms = NetworkManagerState(self._listen_network_changes)
+
+        nms.find_online_state()
+
+        self.nm_interface.emit_signal('StateChanged',
+            NM_STATE_CONNECTED_GLOBAL)
+        self.nm_interface.emit_signal('StateChanged', NM_STATE_DISCONNECTED)
+        self.nm_interface.emit_signal('StateChanged',
+            NM_STATE_CONNECTED_GLOBAL)
+
+        self.assertEqual(nms.state_signal.name, "StateChanged")
+        self.assertEqual(nms.state_signal.callback, nms.state_changed)
+        self.assertEqual(nms.state_signal.interface,
+            "org.freedesktop.NetworkManager")
+        self.assertEqual(self.network_changes,
+            [UNKNOWN, ONLINE, OFFLINE, ONLINE])
+        self.assertFalse(nms.state_signal.removed)
 
     @inlineCallbacks
     def test_is_machine_connected_nm_state_connected_old(self):
@@ -216,7 +301,6 @@
         if exc is not None:
             self.expect(self.dbusmock.exceptions.DBusException)
             self.mocker.result(exc)
-        signalmock.remove()
         self.mocker.replay()
 
     # Invalid name "assert[A-Z].*"