Merge lp:~shanepatrickfagan/ubuntu-sso-client/nm-state-bug-fix into lp:ubuntu-sso-client

[Shane Fagan]

Fixes the network manager states change in 11.10. I had to change stuff around to get it working and I dont know if I approached it entirely correctly but it is working on 11.10 and 11.04 for me. To test just do a sudo python setup.py and kill the cp and see if it connects correctly.

[lp:~shanepatrickfagan/ubuntu-sso-client/nm-state-bug-fix]

719. By Shane Fagan on 2011-06-01

took away that weird code I wrote

[Natalia Bidart (nataliabidart)]

The code makes sense, though in order to get the Approve we need the following:

* unit tests for both branches of the newly added if
* self.NM_STATE_UNKNOWN should be defined for both versions of the protocol
* the protocol version number should not be a literal string but a constant

review: Needs Fixing

[lp:~shanepatrickfagan/ubuntu-sso-client/nm-state-bug-fix]

720. By Shane Fagan on 2011-06-01

fixed a small oversight

[Natalia Bidart (nataliabidart)]

Also, test suite is not passing OK:

nessita@dali:~/canonical/ussoc/review_nm-state-bug-fix$ ./run-tests
Running test suite for ubuntu_sso
Xlib: extension "RANDR" missing on display ":99".
Traceback (most recent call last):
  File "/usr/bin/u1trial", line 269, in <module>
    main()
  File "/usr/bin/u1trial", line 265, in main
    TestRunner(force_gc=options.force_gc).run(testpath, options)
  File "/usr/bin/u1trial", line 182, in run
    options.ignored_modules, options.ignored_paths)
  File "/usr/bin/u1trial", line 161, in _collect_tests
    module_suite = self._load_unittest(filepath)
  File "/usr/bin/u1trial", line 89, in _load_unittest
    module = __import__(modpath, None, None, [""])
  File "/home/nessita/canonical/ussoc/review_nm-state-bug-fix/ubuntu_sso/networkstate/tests/test_linux.py", line 24, in <module>
    from ubuntu_sso.networkstate.linux import (DBUS_UNKNOWN_SERVICE,
ImportError: cannot import name NM_STATE_DISCONNECTED

review: Needs Fixing

[dobey (dobey)]

47 + if str(nm_proxy.Get(NM_DBUS_INTERFACE,
48 + "Version")) >= "0.8.9997":

This doesn't work. Python doesn't compare these two values as tuples of integers here. Instead, it compares them as strings. Which means, any single character in the ASCII charset that comes after the character '0' is going to return True. Also, you should use a lower number for the check, like 0.8.99 perhaps.

review: Needs Fixing

[Shane Fagan (shanepatrickfagan)]

@dobey well the way I did it originally I did .digit and compared them as ints would that work better?

[Shane Fagan (shanepatrickfagan)]

Oh and that version was the one that brought the changes in Ubuntu I dont know if the earlier versions would work in the same way so I thought better be safe than sorry about the version number.

[dobey (dobey)]

You should check the upstream history to see when the API change was made, and what the version number was at that time, and use that version number (or the next one after, as it may have only been bumped at release time).

And for comparison, as alecu said in IRC, you need to get the string split into a tuple of integers (and strings, if there are any non-numeric characters in the version like a/b/pre/etc...), check that the length of each tuple is >= 2, and then compare the tuples. Also need to ensure that the int() values are actually correct, and not just the ASCII numeric value (ie "0" becomes 0).

[lp:~shanepatrickfagan/ubuntu-sso-client/nm-state-bug-fix]

721. By Shane Fagan on 2011-06-02

made the code a lot cleaner and did the suggestions that the guys said but still no tests. Will do them next

722. By Shane Fagan on 2011-06-16

fixed nearly all of the tests and added methods for everything

[dobey (dobey)]

A much better fix for this was proposed by mterry and approved. Rejecting this branch now.

review: Disapprove

Unmerged revisions

722. By Shane Fagan on 2011-06-16

fixed nearly all of the tests and added methods for everything

721. By Shane Fagan on 2011-06-02

made the code a lot cleaner and did the suggestions that the guys said but still no tests. Will do them next

720. By Shane Fagan on 2011-06-01

fixed a small oversight

719. By Shane Fagan on 2011-06-01

took away that weird code I wrote

718. By Shane Fagan on 2011-06-01

shortened the long line since it would probably break pep8

717. By Shane Fagan on 2011-06-01

added some code for detecting the connection in 11.10, it works but needs testing on 11.04 or below. Its a few little changes but all are needed to work I think.

=== modified file 'ubuntu_sso/networkstate/linux.py'
--- ubuntu_sso/networkstate/linux.py	2011-03-02 10:55:12 +0000
+++ ubuntu_sso/networkstate/linux.py	2011-06-16 14:09:34 +0000
@@ -33,13 +33,6 @@
     UNKNOWN: "unknown",
 }
 
-# Internal NetworkManager State constants
-NM_STATE_UNKNOWN = 0
-NM_STATE_ASLEEP = 1
-NM_STATE_CONNECTING = 2
-NM_STATE_CONNECTED = 3
-NM_STATE_DISCONNECTED = 4
-
 NM_DBUS_INTERFACE = "org.freedesktop.NetworkManager"
 NM_DBUS_OBJECTPATH = "/org/freedesktop/NetworkManager"
 DBUS_UNKNOWN_SERVICE = "org.freedesktop.DBus.Error.ServiceUnknown"
@@ -62,9 +55,9 @@
 
     def got_state(self, state):
         """Called by DBus when the state is retrieved from NM."""
-        if state == NM_STATE_CONNECTED:
+        if state == self.nm_state_connected:
             self.call_result_cb(ONLINE)
-        elif state == NM_STATE_CONNECTING:
+        elif state == self.nm_state_connecting:
             logger.debug("Currently connecting, waiting for signal")
         else:
             self.call_result_cb(OFFLINE)
@@ -74,23 +67,60 @@
         if isinstance(error, self.dbus.exceptions.DBusException) and \
                 error.get_dbus_name() == DBUS_UNKNOWN_SERVICE:
             logger.debug("Network Manager not present")
-            self.call_result_cb(UNKNOWN)
+            self.call_result_cb(self.nm_state_unknown)
         else:
             logger.error("Error contacting NetworkManager: %s" % \
                              str(error))
-            self.call_result_cb(UNKNOWN)
+            self.call_result_cb(self.nm_state_unknown)
 
     def state_changed(self, state):
         """Called when a signal is emmited by Network Manager."""
-        if int(state) == NM_STATE_CONNECTED:
+        if int(state) == self.nm_state_connected:
             self.call_result_cb(ONLINE)
-        elif int(state) == NM_STATE_DISCONNECTED:
+        elif int(state) == self.nm_state_disconnected:
             self.call_result_cb(OFFLINE)
         else:
             logger.debug("Not yet connected: continuing to wait")
 
     def find_online_state(self):
         """Get the network state and return it thru the set callback."""
+        sysbus = self.dbus.SystemBus()
+        nm_proxy = sysbus.get_object(NM_DBUS_INTERFACE,
+                                         NM_DBUS_OBJECTPATH,
+                                         follow_name_owner_changes=True)
+        nm_proxy.Get(NM_DBUS_INTERFACE,
+                    "Version", reply_handler=self.got_version,
+                    error_handler=self.got_error)
+
+    def got_version(self, version):
+        """Parse the version and pass what state version to use in set_state_values."""
+        x = str(version).split(".")
+        new_state_version = False
+        if int(x[0]) == 0 and int(x[1]) <= 8:
+            if int(x[1]) == 8 and x[2].startswith("99"):
+                    new_state_version = True
+        else:
+            new_state_version = True
+        self.set_state_values(new_state_version)
+
+    def set_state_values(self, new_state_version):
+        """Set the state values."""
+        if new_state_version == True:
+            self.nm_state_asleep = 10
+            self.nm_state_disconnecting = 20
+            self.nm_state_disconnected = 30
+            self.nm_state_connecting = 40
+            self.nm_state_connected = 70
+        else:
+            self.nm_state_asleep = 1
+            self.nm_state_connecting = 2
+            self.nm_state_connected = 3
+            self.nm_state_disconnected = 4
+        self.nm_state_unknown = 0
+        self.detect_online_state()
+
+    def detect_online_state(self):
+        """Detect the online state."""
         try:
             sysbus = self.dbus.SystemBus()
             nm_proxy = sysbus.get_object(NM_DBUS_INTERFACE,

=== modified file 'ubuntu_sso/networkstate/tests/test_linux.py'
--- ubuntu_sso/networkstate/tests/test_linux.py	2011-06-15 19:19:19 +0000
+++ ubuntu_sso/networkstate/tests/test_linux.py	2011-06-16 14:09:34 +0000
@@ -19,12 +19,11 @@
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
 """Tests for the network state detection code."""
 
+from twisted.trial.unittest import TestCase
+
 from ubuntu_sso.networkstate import (NetworkManagerState,
                                      ONLINE, OFFLINE, UNKNOWN)
-from ubuntu_sso.networkstate.linux import  (DBUS_UNKNOWN_SERVICE,
-                                            NM_STATE_DISCONNECTED,
-                                            NM_STATE_CONNECTING,
-                                            NM_STATE_CONNECTED)
+from ubuntu_sso.networkstate.linux import  (DBUS_UNKNOWN_SERVICE)
 
 from mocker import ARGS, KWARGS, ANY, MockerTestCase
 
@@ -47,6 +46,7 @@
     """Base test case for NM state tests."""
 
     def setUp(self):
+<<<<<<< TREE
         """Setup the mocker dbus object tree."""
         super(NetworkManagerBaseTestCase, self).setUp()
 
@@ -76,6 +76,30 @@
             self.mocker.result(exc)
         signalmock.remove()
         self.mocker.replay()
+=======
+         """Setup the mocker dbus object tree."""
+         self.dbusmock = self.mocker.mock()
+         self.dbusmock.SystemBus()
+         sysbusmock = self.mocker.mock()
+         self.mocker.result(sysbusmock)
+
+         sysbusmock.get_object(ARGS, KWARGS)
+         proxymock = self.mocker.mock()
+         self.mocker.result(proxymock)
+
+         self.dbusmock.Interface(proxymock, ANY)
+         ifmock = self.mocker.mock()
+         self.mocker.result(ifmock)
+
+         ifmock.connect_to_signal(ARGS, KWARGS)
+         signalmock = self.mocker.mock()
+         self.mocker.result(signalmock)
+
+         proxymock.Get(ARGS, KWARGS)
+         signalmock.remove()
+
+         self.mocker.replay()
+>>>>>>> MERGE-SOURCE
 
     # Invalid name "assert[A-Z].*"
     # pylint: disable=C0103
@@ -125,24 +149,130 @@
 
     def test_nm_online(self):
         """Check the connected case."""
+<<<<<<< TREE
         self.check_nm_state(self.assertOnline, NM_STATE_CONNECTED)
+=======
+
+        def got_state_cb(state):
+            """State was given."""
+            self.assertEquals(state, ONLINE)
+
+        nms = NetworkManagerState(got_state_cb, self.dbusmock)
+        nms.set_state_values(True)
+        nms.got_state(nms.nm_state_connected)
+>>>>>>> MERGE-SOURCE
 
     def test_nm_offline(self):
         """Check the disconnected case."""
+<<<<<<< TREE
         self.check_nm_state(self.assertOffline, NM_STATE_DISCONNECTED)
+=======
+
+        def got_state_cb(state):
+            """State was given."""
+            self.assertEquals(state, OFFLINE)
+
+        nms = NetworkManagerState(got_state_cb, self.dbusmock)
+        nms.set_state_values(True)
+        nms.got_state(nms.nm_state_disconnected)
+>>>>>>> MERGE-SOURCE
 
     def test_nm_connecting_then_online(self):
         """Check the waiting for connection case."""
+<<<<<<< TREE
         self.check_nm_state_change(self.assertOnline,
                                    NM_STATE_CONNECTING, NM_STATE_CONNECTED)
+=======
+
+        def got_state_cb(state):
+            """State was given."""
+            self.assertEquals(state, ONLINE)
+
+        nms = NetworkManagerState(got_state_cb, self.dbusmock)
+        nms.set_state_values(True)
+        nms.got_state(nms.nm_state_connecting)
+        nms.state_changed(nms.nm_state_connected)
+>>>>>>> MERGE-SOURCE
 
     def test_nm_connecting_then_offline(self):
         """Check the waiting but fail case."""
+<<<<<<< TREE
         self.check_nm_state_change(self.assertOffline,
                                    NM_STATE_CONNECTING, NM_STATE_DISCONNECTED)
 
 
 class NetworkManagerStateErrorsTestCase(NetworkManagerBaseTestCase):
+=======
+
+        def got_state_cb(state):
+            """State was given."""
+            self.assertEquals(state, OFFLINE)
+
+        nms = NetworkManagerState(got_state_cb, self.dbusmock)
+        nms.set_state_values(True)
+        nms.got_state(nms.nm_state_connecting)
+        nms.state_changed(nms.nm_state_disconnected)
+
+
+class CheckVersionTestCase(TestCase):
+    """Check the version tests."""
+
+    def test_got_version(self):
+        """Check that got version checks for the right version."""
+        self.new_state_version = None
+
+        def check_return_value(state_version):
+            """Test true return."""
+            self.new_state_version = state_version
+
+        def check_got_version(version, expected):
+            nms.got_version(version)
+            self.assertEquals(self.new_state_version, expected)
+
+        nms = NetworkManagerState(None, None)
+        self.patch(nms, "set_state_values", check_return_value)
+        versions = ["0.8.9997", "0.9", "1.0", "1.12.4215"]
+        for x in versions:
+            check_got_version(x, True)
+        versions = ["0.8.8713", "0.3.2141", "0.1.3124", "0.5.5125"]
+        for x in versions:
+	    check_got_version(x, False)
+
+class CheckSetValuesTestCase(TestCase):
+    """Check that the values are set."""
+
+    def test_set_state(self):
+        """Test that the states are actually set."""
+        def check_set_values(nms):
+            if nms.nm_state_asleep == 10 and \
+                     nms.nm_state_disconnected == 30 and \
+                     nms.nm_state_connecting == 40 and \
+                     nms.nm_state_connected == 70 and \
+                     nms.nm_state_unknown == 0:
+                return True
+            else:
+                return False
+        nms = NetworkManagerState(None, None)
+        nms.set_state_values(True)
+        self.assertEquals(check_set_values(nms), True)
+        nms.set_state_values(False)
+        self.assertEquals(check_set_values(nms), False)
+
+
+class CheckDetectOnlineStateMethodCall(TestCase):
+    """Check detect online state method call."""
+
+    def test_detect_online_state_call(self):
+        self.is_called = False
+        def check_method_call():
+            self.is_called = True
+        nms = NetworkManagerState(None, None)
+        self.patch(nms, "detect_online_state", check_method_call)
+        nms.detect_online_state()
+        self.assertEquals(self.is_called, True)
+
+class NetworkManagerStateErrorsTestCase(MockerTestCase):
+>>>>>>> MERGE-SOURCE
     """Test NetworkManager state retrieval code."""
 
     def mock_except_while_getting_proxy(self, exc):
@@ -154,8 +284,20 @@
 
     def test_nm_not_running(self):
         """Check the case when NM is not running."""
+<<<<<<< TREE
         self.connect_proxy(TestNmNotAvailableException)
         self.check_nm_error(self.assertUnknown, TestNmNotAvailableException())
+=======
+
+        def got_state_cb(state):
+            """State was given."""
+            self.assertEquals(state, nms.nm_state_unknown)
+
+        self.mock_dbus_error_while_getting_state(TestNmNotAvailableException)
+        nms = NetworkManagerState(got_state_cb, self.dbusmock)
+        nms.set_state_values(True)
+        nms.got_error(TestNmNotAvailableException())
+>>>>>>> MERGE-SOURCE
 
     def test_dbus_problem(self):
         """Check the case when DBus throws some other exception."""