Merge lp:~alecu/ubuntuone-client/fix-cmt-usage into lp:ubuntuone-client

[Alejandro J. Cura]

On Windows, use the CredentialsManagementTool in the proper way (LP: #799958)

On Windows, use the CredentialsManagementTool in the proper way (LP: #799958)

[Alejandro J. Cura (alecu)]

To run just the tests for this branch, use a command line similar to the following:

E:\ubuntuone-client\fix-cmt-usage>python c:\python27\scripts\u1trial --reactor=txnp tests\platform\windows\test_ipc.py

[Manuel de la Peña (mandel)]

Grande!

review: Approve

[Natalia Bidart (nataliabidart)]

* As far as I see, there is no need to have an __init__ method for FakeIPCRoot.

* I think this code needs to be deleted since is not used:

    def on_credentials_found_cb(self, app_name, creds, *args, **kwargs):

    def on_credentials_error_cb(self, app_name, *args, **kwargs):

* The rest looks great!

review: Needs Fixing

[Alejandro J. Cura (alecu)]

Hi, thanks for the review!

> * As far as I see, there is no need to have an __init__ method for
> FakeIPCRoot.

The empty method on FakeIPCRoot is needed in order to override the default constructor. I've added a comment to clarify this.

> > * I think this code needs to be deleted since is not used:
>
> def on_credentials_found_cb(self, app_name, creds, *args, **kwargs):
>
> def on_credentials_error_cb(self, app_name, *args, **kwargs):

This is now fixed and pushed.

> * The rest looks great!

thanks!

[Natalia Bidart (nataliabidart)]

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Manuel de la Peña (mandel)]

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

The attempt to merge lp:~alecu/ubuntuone-client/fix-cmt-usage into lp:ubuntuone-client failed. Below is the output from the failed tests.

/usr/bin/gnome-autogen.sh
checking for autoconf >= 2.53...
  testing autoconf2.50... not found.
  testing autoconf... found 2.67
checking for automake >= 1.10...
  testing automake-1.11... found 1.11.1
checking for libtool >= 1.5...
  testing libtoolize... found 2.2.6b
checking for intltool >= 0.30...
  testing intltoolize... found 0.41.1
checking for pkg-config >= 0.14.0...
  testing pkg-config... found 0.25
checking for gtk-doc >= 1.0...
  testing gtkdocize... found 1.17
Checking for required M4 macros...
Checking for forbidden M4 macros...
Processing ./configure.ac
Running libtoolize...
libtoolize: putting auxiliary files in `.'.
libtoolize: copying file `./ltmain.sh'
libtoolize: putting macros in AC_CONFIG_MACRO_DIR, `m4'.
libtoolize: copying file `m4/libtool.m4'
libtoolize: copying file `m4/ltoptions.m4'
libtoolize: copying file `m4/ltsugar.m4'
libtoolize: copying file `m4/ltversion.m4'
libtoolize: copying file `m4/lt~obsolete.m4'
Running intltoolize...
Running gtkdocize...
Running aclocal-1.11...
Running autoconf...
Running autoheader...
Running automake-1.11...
Running ./configure --enable-gtk-doc --enable-debug ...
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking for style of include used by make... GNU
checking for gcc... gcc
checking whether the C compiler works... yes
checking for C compiler default output file name... a.out
checking for suffix of executables...
checking whether we are cross compiling... no
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for gcc option to accept ISO C89... none needed
checking dependency style of gcc... gcc3
checking for library containing strerror... none required
checking for gcc... (cached) gcc
checking whether we are using the GNU C compiler... (cached) yes
checking whether gcc accepts -g... (cached) yes
checking for gcc option to accept ISO C89... (cached) none needed
checking dependency style of gcc... (cached) gcc3
checking build system type... i686-pc-linux-gnu
checking host system type... i686-pc-linux-gnu
checking for a sed that does not truncate output... /bin/sed
checking for grep that handles long lines and -e... /bin/grep
checking for egrep... /bin/grep -E
checking for fgrep... /bin/grep -F
checking for ld used by gcc... /usr/bin/ld
checking if the linker (/usr/bin/ld) is GNU ld... yes
checking for BSD- or MS-compatible name lister (nm)... /usr/bin/nm -B
checking the name lister (/usr/bin/nm -B) interface... BSD nm
checking whether ln -s works... yes
checking the maximum length of command line arguments... 1572864
checking whether the shell understands some XSI constructs... yes
checking whether the shell understands "+="... yes
checking for /usr/bin/ld option to reload object files... -r
checking for objdump... objdump
checking how to recognize dependent librarie...

[Ubuntu One Auto Pilot (otto-pilot)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

=== modified file 'run-tests.bat'
--- run-tests.bat	2011-04-14 11:32:42 +0000
+++ run-tests.bat	2011-06-22 15:39:50 +0000
@@ -52,7 +52,7 @@
 ECHO Python found, executing the tests...
 COPY windows\clientdefs.py ubuntuone\clientdefs.py
 :: execute the tests with a number of ignored linux only modules
-"%PYTHONPATH%\python.exe" "%PYTHONPATH%\Scripts\u1trial" -c tests -p tests\platform\linux
+"%PYTHONPATH%\python.exe" "%PYTHONPATH%\Scripts\u1trial" --reactor=txnp -c tests -p tests\platform\linux %1
 "%PYTHONPATH%\python.exe" "%PYTHONPATH%\Scripts\u1lint"
 :: test for style if we can, if pep8 is not present, move to the end
 IF EXIST "%PYTHONPATH%Scripts\pep8.exe"
@@ -62,4 +62,4 @@
 :: Delete the temp folders
 RMDIR /s /q _trial_temp
 RMDIR /s /q .coverage
-:END
\ No newline at end of file
+:END

=== modified file 'tests/platform/windows/test_ipc.py'
--- tests/platform/windows/test_ipc.py	2011-06-15 15:40:17 +0000
+++ tests/platform/windows/test_ipc.py	2011-06-22 15:39:50 +0000
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 #
-# Author: Manuel de la Pena<[e-mail]>
+# Authors: Manuel de la Pena <[e-mail]>
+#          Alejandro J. Cura <[e-mail]>
 #
 # Copyright 2011 Canonical Ltd.
 #
@@ -27,12 +28,14 @@
     Root,
     RemoteReference)
 
+from ubuntuone.platform.windows import ipc
 from ubuntuone.platform.windows.ipc import (
     AllEventsSender,
     Config,
     Events,
     Folders,
     FileSystem,
+    IPCInterface,
     IPCRoot,
     PublicFiles,
     Shares,
@@ -3172,3 +3175,122 @@
         d.addCallback(test_execution)
         # pylint: enable=E1101
         return d
+
+
+class FakeIPCRoot(object, ipc.Root):
+    """A Fake IPC Root."""
+
+    def __init__(self, *args, **kwargs):
+        """Initialize this fake instance."""
+        # This empty method is needed to override the default constructor
+
+
+class RandomException(Exception):
+    """A random exception."""
+
+
+class FakeCredentialsManagementTool(object):
+    """A Fake CredentialsManagementTool."""
+
+    fake_credentials = {
+        "consumer_key": "sample consumer key",
+        "consumer_secret": "sample consumer secret",
+        "token": "sample token",
+        "token_secret": "sample token secret",
+    }
+    should_fail = False
+
+    def find_credentials(self):
+        """Find the credentials."""
+        if self.should_fail:
+            return defer.fail(RandomException())
+        return defer.succeed(self.fake_credentials)
+
+    def register(self, *args):
+        """Register a new user."""
+        if self.should_fail:
+            return defer.fail(RandomException())
+        return defer.succeed(self.fake_credentials)
+
+
+class FakeCredentialsManagementToolFails(FakeCredentialsManagementTool):
+    """A Fake CredentialsManagementTool that consistently fails."""
+
+    should_fail = True
+
+
+class FakeEventQueue(object):
+    """A fake event queue."""
+
+    def __init__(self):
+        """Initialize this fake instance."""
+        self.pushed_events = []
+
+    def push(self, event_name, **kwargs):
+        """Store the pushed event."""
+        event = (event_name, kwargs)
+        self.pushed_events.append(event)
+
+
+class FakeMain(object):
+    """A fake main."""
+
+    def __init__(self):
+        """Initialize this fake instance."""
+        self.event_q = FakeEventQueue()
+
+
+class IPCInterfaceTestCaseBase(TestCase):
+    """Base tests for IPCInterface."""
+
+    cmt_class = None
+
+    def setUp(self):
+        """Initialize this test instance."""
+        if self.cmt_class is None: return
+        self.patch(ipc, "CredentialsManagementTool", self.cmt_class)
+        self.patch(ipc, "GetUserNameEx", lambda x: "sample_username")
+        self.patch(ipc, "IPCRoot", FakeIPCRoot)
+        no_op = lambda *args: None
+        self.patch(ipc.reactor, "listenPipe", no_op)
+        self.patch(IPCInterface, "network_connected", no_op)
+        self.ipc = IPCInterface(FakeMain())
+
+
+class IPCInterfaceTestCase(IPCInterfaceTestCaseBase):
+    """Tests for the IPCInterface when everything goes dandy."""
+
+    cmt_class = FakeCredentialsManagementTool
+
+    @defer.inlineCallbacks
+    def test_connect(self):
+        """Test the connect method."""
+        expected_token = FakeCredentialsManagementTool.fake_credentials
+        yield self.ipc.connect()
+        token = self.ipc.main.event_q.pushed_events[0][1]["access_token"]
+        self.assertEqual(token, expected_token)
+
+    @defer.inlineCallbacks
+    def test_connect_autoconnecting(self):
+        """Test the connect method when autoconnecting."""
+        expected_token = FakeCredentialsManagementTool.fake_credentials
+        yield self.ipc.connect(autoconnecting=True)
+        token = self.ipc.main.event_q.pushed_events[0][1]["access_token"]
+        self.assertEqual(token, expected_token)
+
+
+class IPCInterfaceFailingTestCase(IPCInterfaceTestCaseBase):
+    """Tests for the IPCInterface on a rainy day."""
+
+    cmt_class = FakeCredentialsManagementToolFails
+
+    @defer.inlineCallbacks
+    def test_connect(self):
+        """Test the connect method fails."""
+        yield self.assertFailure(self.ipc.connect(), ipc.NoAccessToken)
+
+    @defer.inlineCallbacks
+    def test_connect_autoconnecting(self):
+        """Test the connect method when autoconnecting fails."""
+        d = self.ipc.connect(autoconnecting=True)
+        yield self.assertFailure(d, ipc.NoAccessToken)

=== modified file 'ubuntuone/platform/windows/ipc.py'
--- ubuntuone/platform/windows/ipc.py	2011-06-21 09:57:17 +0000
+++ ubuntuone/platform/windows/ipc.py	2011-06-22 15:39:50 +0000
@@ -1,6 +1,7 @@
 # -*- coding: utf-8 -*-
 #
-# Author: Manuel de la Pena<[e-mail]>
+# Authors: Manuel de la Pena <[e-mail]>
+#          Alejandro J. Cura <[e-mail]>
 #
 # Copyright 2011 Canonical Ltd.
 #
@@ -25,7 +26,6 @@
 
 from twisted.internet import defer, reactor
 from twisted.spread.pb import Referenceable, Root, PBServerFactory
-from twisted.python.failure import Failure
 from ubuntuone.syncdaemon.interfaces import IMarker
 from ubuntuone.platform.windows import CredentialsManagementTool
 from ubuntuone.platform.windows.network_manager import NetworkManager
@@ -1057,56 +1057,28 @@
                 return
             token = {'consumer_key': ckey, 'consumer_secret': csecret,
                      'token': key, 'token_secret': secret}
-            self.main.event_q.push('SYS_USER_CONNECT', access_token=token)
         else:
-            token = yield self._request_token(autoconnecting=autoconnecting)
-
-    def on_credentials_found_cb(self, app_name, creds, *args, **kwargs):
-        """Provide the creds."""
-        print 'Creds are %s' % creds
-        logger.info('connect: credential request was successful, '
-                    'pushing SYS_USER_CONNECT.')
-        self.main.event_q.push('SYS_USER_CONNECT', access_token=creds)
-
-    def on_credentials_error_cb(self, app_name, *args, **kwargs):
-        """Deal with the error."""
-        d = self._deferred
-        d.errback(Failure(NoAccessToken()))
-
-    @defer.inlineCallbacks
-    def _register_to_signals(self):
-        """Register to the creds signals."""
-        management_tool = CredentialsManagementTool()
-        self.creds = yield management_tool.get_creds_proxy()
-        self.creds.register_to_credentials_found(self.on_credentials_found_cb)
-        self.creds.register_to_credentials_not_found(
-                                                 self.on_credentials_error_cb)
-        self.creds.register_to_authorization_denied(
-                                                 self.on_credentials_error_cb)
-        self.creds.register_to_credentials_error(self.on_credentials_error_cb)
+            try:
+                token = yield self._request_token(
+                                            autoconnecting=autoconnecting)
+            except Exception, e:
+                raise NoAccessToken(e)
+
+        self.main.event_q.push('SYS_USER_CONNECT', access_token=token)
 
     def _request_token(self, autoconnecting):
         """Request to SSO auth service to fetch the token."""
-        self._deferred = defer.Deferred()
-
-        def error_handler(error):
-            """Default error handler."""
-            logger.error('Handling error from sso %s.', error)
-
         # call ubuntu sso
         try:
             management = CredentialsManagementTool()
-            # add the diff callbacks to be executed
-            self._register_to_signals()
-            # ignore the reply, we get the result via signals
+            # return the deferred, since we are no longer using signals
             if autoconnecting:
                 return management.find_credentials()
             else:
                 return management.register({'window_id': '0'})  # no window ID
         except:
-            logger.exception('connect failed while getting the token')
+            logger.exception('failure while getting the token')
             raise
-        return self._deferred
 
     def disconnect(self):
         """ Push the SYS_USER_DISCONNECT event. """