Merge lp:~free.ekanayaka/landscape-client/config-failure into lp:~landscape/landscape-client/trunk

[Free Ekanayaka]

A slightly tricky change, apparently it's not possible to call sys.exit inside a running callback, so I simply changed it to run after reactor.run() returns.

[Sidnei da Silva (sidnei)]

[1] I'm a little bit worried about a scoping problem with 'exit_with_error' being assigned a value *after* the 'got_error' function is defined. It might be just because scoping is trickier in Javascript. I would just move the assignment before the function or pass the variable as an argument to the function instead.

[2] The setup() function has a similar code path. I suspect it can stay as it is because it's not used as a callback? Just for clarification.

Other than that, looks good. +1.

review: Approve

[Sidnei da Silva (sidnei)]

Oh, some small cleanups too:

=== pep8 ===

landscape/tests/test_configuration.py:1493:1: E302 expected 2 blank lines, found 3
landscape/tests/test_configuration.py:1529:9: E301 expected 1 blank line, found 2
landscape/tests/test_configuration.py:1579:9: E301 expected 1 blank line, found 2
landscape/configuration.py:496:5: E301 expected 1 blank line, found 2

=== pyflakes ===

landscape/tests/test_configuration.py:6: 'reactor' imported but unused

[Free Ekanayaka (free.ekanayaka)]

Thanks for the careful review, Sidnei.

I fixed all the lints but two, as they are due to comments preceding function definitions.

[1]

Good point, I moved exit_with_error at the beginning of the main function.

[2]

Right, that's not an issue because we're not running that code inside a callback.

[lp:~free.ekanayaka/landscape-client/config-failure]

243. By Free Ekanayaka on 2010-05-10

Fix Sidnei's review comment [1] and lints

[Muharem Hrnjadovic (al-maisan)]

Looks good, +1

review: Approve

=== modified file 'landscape/broker/client.py'
--- landscape/broker/client.py	2010-04-23 11:37:25 +0000
+++ landscape/broker/client.py	2010-05-10 15:05:39 +0000
@@ -178,4 +178,6 @@
 
     def exit(self):
         """Stop the reactor and exit the process."""
-        self.reactor.stop()
+        # Stop with a short delay to give a chance to reply to the
+        # caller over AMP.
+        self.reactor.call_later(0.1, self.reactor.stop)

=== modified file 'landscape/broker/server.py'
--- landscape/broker/server.py	2010-04-22 15:44:38 +0000
+++ landscape/broker/server.py	2010-05-10 15:05:39 +0000
@@ -52,9 +52,6 @@
 
         reactor.call_on("message", self.broadcast_message)
         reactor.call_on("impending-exchange", self.impending_exchange)
-        reactor.call_on("exchange-failed", self.exchange_failed)
-        reactor.call_on("registration-done", self.registration_done)
-        reactor.call_on("registration-failed", self.registration_failed)
         reactor.call_on("message-type-acceptance-changed",
                         self.message_type_acceptance_changed)
         reactor.call_on("server-uuid-changed", self.server_uuid_changed)
@@ -201,18 +198,6 @@
     def impending_exchange(self):
         """Broadcast an C{impending-exchange} event to the clients."""
 
-    @event
-    def exchange_failed(self):
-        """Broadcast a C{exchange-failed} event to the clients."""
-
-    @event
-    def registration_done(self):
-        """Broadcast a C{registration-done} event to the clients."""
-
-    @event
-    def registration_failed(self):
-        """Broadcast a C{registration-failed} event to the clients."""
-
     def listen_events(self, event_types):
         """
         Return a C{Deferred} that fires when the first event occurs among the

=== modified file 'landscape/broker/tests/test_client.py'
--- landscape/broker/tests/test_client.py	2010-03-19 09:34:15 +0000
+++ landscape/broker/tests/test_client.py	2010-05-10 15:05:39 +0000
@@ -283,7 +283,8 @@
         """
         The L{BrokerClient.exit} method causes the reactor to be stopped.
         """
-        self.client.reactor = self.mocker.mock()
+        self.client.reactor.stop = self.mocker.mock()
         self.client.reactor.stop()
         self.mocker.replay()
         self.client.exit()
+        self.client.reactor.advance(0.1)

=== modified file 'landscape/broker/tests/test_server.py'
--- landscape/broker/tests/test_server.py	2010-04-22 15:44:38 +0000
+++ landscape/broker/tests/test_server.py	2010-05-10 15:05:39 +0000
@@ -316,39 +316,6 @@
         self.client.add(plugin)
         return self.assertSuccess(self.broker.impending_exchange(), [[None]])
 
-    def test_exchange_failed(self):
-        """
-        The L{BrokerServer.exchange_failed} method broadcasts an
-        C{exchange-failed} event to all connected clients.
-        """
-        callback = self.mocker.mock()
-        callback()
-        self.mocker.replay()
-        self.client_reactor.call_on("exchange-failed", callback)
-        return self.assertSuccess(self.broker.exchange_failed(), [[None]])
-
-    def test_registration_done(self):
-        """
-        The L{BrokerServer.registration_done} method broadcasts a
-        C{registration-done} event to all connected clients.
-        """
-        callback = self.mocker.mock()
-        callback()
-        self.mocker.replay()
-        self.client_reactor.call_on("registration-done", callback)
-        return self.assertSuccess(self.broker.registration_done(), [[None]])
-
-    def test_registration_failed(self):
-        """
-        The L{BrokerServer.registration_failed} method broadcasts a
-        C{registration-failed} event to all connected clients.
-        """
-        callback = self.mocker.mock()
-        callback()
-        self.mocker.replay()
-        self.client_reactor.call_on("registration-failed", callback)
-        return self.assertSuccess(self.broker.registration_failed(), [[None]])
-
     def test_broker_started(self):
         """
         The L{BrokerServer.broker_started} method broadcasts a
@@ -470,28 +437,6 @@
         self.mocker.replay()
         self.reactor.fire("impending-exchange")
 
-    def test_exchange_failed(self):
-        """
-        When an C{exchange-failed} event is fired by the reactor, the
-        broker broadcasts it to its clients.
-        """
-        self.client.fire_event = self.mocker.mock()
-        self.client.fire_event("exchange-failed")
-        self.mocker.result(succeed(None))
-        self.mocker.replay()
-        self.reactor.fire("exchange-failed")
-
-    def test_registration_done(self):
-        """
-        When a C{registration-done} event is fired by the reactor, the
-        broker broadcasts it to its clients.
-        """
-        self.client.fire_event = self.mocker.mock()
-        self.client.fire_event("registration-done")
-        self.mocker.result(succeed(None))
-        self.mocker.replay()
-        self.reactor.fire("registration-done")
-
     def test_message_type_acceptance_changed(self):
         """
         When a C{message-type-acceptance-changed} event is fired by the

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2010-04-22 16:02:14 +0000
+++ landscape/configuration.py	2010-05-10 15:05:39 +0000
@@ -489,16 +489,14 @@
         will happen.
     """
     reactor = TwistedReactor()
+    exit_with_error = []
 
     # XXX: many of these reactor.stop() calls should also specify a non-0 exit
     # code, unless ok-no-register is passed.
 
     def stop():
         connector.disconnect()
-        # For some obscure reason our TwistedReactor.stop method calls
-        # reactor.crash() instead of reactor.stop(), which doesn't work
-        # here. Maybe TwistedReactor.stop should simply use reactor.stop().
-        reactor.call_later(0, reactor._reactor.stop)
+        reactor.stop()
 
     def failure():
         print_text("Invalid account name or "
@@ -545,7 +543,6 @@
             remote.register().addErrback(handle_registration_errors)]
         # We consume errors here to ignore errors after the first one.
         # catch_all will be called for the very first deferred that fails.
-
         results = gather_results(deferreds, consume_errors=True)
         return results.addErrback(catch_all)
 
@@ -554,10 +551,9 @@
                    "client.", error=True)
         print_text("This machine will be registered with the provided "
                    "details when the client runs.", error=True)
-        exit_code = 2
-        if config.ok_no_register:
-            exit_code = 0
-        sys.exit(exit_code)
+        if not config.ok_no_register:
+            exit_with_error.append(2)
+        stop()
 
     connector = RemoteBrokerConnector(reactor, config)
     result = connector.connect(max_retries=0, quiet=True)
@@ -566,6 +562,9 @@
 
     reactor.run()
 
+    if exit_with_error:
+        sys.exit(exit_with_error[0])
+
     return result
 
 

=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2010-04-28 19:27:18 +0000
+++ landscape/tests/test_configuration.py	2010-05-10 15:05:39 +0000
@@ -3,7 +3,6 @@
 from ConfigParser import ConfigParser
 
 from twisted.internet.defer import succeed, fail
-from twisted.internet import reactor
 
 from landscape.reactor import TwistedReactor
 from landscape.lib.fetch import HTTPCodeError, PyCurlError
@@ -1492,12 +1491,6 @@
 
 class RegisterFunctionTest(LandscapeTest):
 
-    # Due to the way these tests run, the run() method on the reactor is called
-    # *before* any of the remote methods (reload, register) are called, because
-    # these tests "hold" the reactor until after the tests runs, then the
-    # reactor is given back control of the process, *then* all the remote calls
-    # in the dbus queue are fired.
-
     helpers = [BrokerServiceHelper]
 
     def test_register_success(self):
@@ -1534,7 +1527,7 @@
         # The deferred callback finally prints out this message.
         print_text_mock("System successfully registered.")
 
-        reactor_mock.call_later(0, reactor.stop)
+        reactor_mock.stop()
 
         # Nothing else is printed!
         print_text_mock(ANY)
@@ -1585,7 +1578,7 @@
         print_text_mock("Invalid account name or registration password.",
                         error=True)
 
-        reactor_mock.call_later(0, reactor.stop)
+        reactor_mock.stop()
 
         # Nothing else is printed!
         print_text_mock(ANY)
@@ -1636,7 +1629,7 @@
                         error=True)
 
 
-        reactor_mock.call_later(0, reactor.stop)
+        reactor_mock.stop()
 
         # Nothing else is printed!
         print_text_mock(ANY)
@@ -1700,8 +1693,15 @@
         # This will make the RemoteBrokerConnector.connect call fail
         print_text_mock = self.mocker.replace(print_text)
         time_mock = self.mocker.replace("time")
+        sys_mock = self.mocker.replace("sys")
         reactor_mock = self.mocker.patch(TwistedReactor)
 
+        connector_factory = self.mocker.replace(
+            "landscape.broker.amp.RemoteBrokerConnector", passthrough=False)
+        connector = connector_factory(ANY, ANY)
+        connector.connect(max_retries=0, quiet=True)
+        self.mocker.result(fail(ZeroDivisionError))
+
         print_text_mock(ARGS)
         time_mock.sleep(ANY)
         reactor_mock.run()
@@ -1713,15 +1713,14 @@
         print_text_mock(CONTAINS("This machine will be registered"),
                         error=True)
 
+        sys_mock.exit(2)
+        connector.disconnect()
+        reactor_mock.stop()
+
         self.mocker.replay()
 
-        def assert_exit_code(system_exit):
-            self.assertEquals(system_exit.code, 2)
-
         config = get_config(self, ["-a", "accountname", "--silent"])
-        result = register(config)
-        self.assertFailure(result, SystemExit)
-        return result.addCallback(assert_exit_code)
+        return register(config)
 
     def test_register_bus_connection_failure_ok_no_register(self):
         """
@@ -1735,6 +1734,7 @@
         print_text_mock(ARGS)
         time_mock.sleep(ANY)
         reactor_mock.run()
+        reactor_mock.stop()
 
         print_text_mock(
             CONTAINS("There was an error communicating with the "
@@ -1745,14 +1745,9 @@
 
         self.mocker.replay()
 
-        def assert_exit_code(system_exit):
-            self.assertEquals(system_exit.code, 0)
-
         config = get_config(self, ["-a", "accountname", "--silent",
                                    "--ok-no-register"])
-        result = register(config)
-        self.assertFailure(result, SystemExit)
-        return result.addCallback(assert_exit_code)
+        return self.assertSuccess(register(config))
 
 
 class RegisterFunctionNoServiceTest(LandscapeTest):
@@ -1801,7 +1796,7 @@
 
         # WHOAH DUDE. This waits for callLater(0, reactor.stop).
         connector.disconnect()
-        reactor_mock.call_later(0, reactor.stop)
+        reactor_mock.stop()
 
         self.mocker.replay()