Merge lp:~bjornt/landscape-client/apt-facade-changer-optional into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Use AptFacade in PackageChanger, if USE_APT_FACADE=1 is set.

I've removed the use_apt_facade parameter to run_task_handler() and
moved the check to inside that method. Everything that uses
run_task_handler() should be compatible with AptFacade.

[Thomas Herve (therve)]

[1] You may want to use EnvironSaverHelper to make your os.environ changes safer.

+1!

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Great, +1!

review: Approve

[lp:~bjornt/landscape-client/apt-facade-changer-optional]

408. By Björn Tillenius on 2011-11-17

Use EnvironSaverHelper.

[Björn Tillenius (bjornt)]

On Tue, Nov 15, 2011 at 01:05:26PM -0000, Thomas Herve wrote:
> Review: Approve
>
> [1] You may want to use EnvironSaverHelper to make your os.environ
> changes safer.

Didn't know about EnvironSaverHelper, thanks for pointing it out. I've
change the code to use it, instead of restoring the environment manually.

--
Björn Tillenius | https://launchpad.net/~bjornt

=== modified file 'landscape/package/reporter.py'
--- landscape/package/reporter.py	2011-11-08 10:54:08 +0000
+++ landscape/package/reporter.py	2011-11-17 08:50:28 +0000
@@ -725,9 +725,7 @@
     elif "FAKE_PACKAGE_STORE" in os.environ:
         return run_task_handler(FakeReporter, args)
     else:
-        use_apt_facade = os.environ.get("USE_APT_FACADE", False)
-        return run_task_handler(
-            PackageReporter, args, use_apt_facade=use_apt_facade)
+        return run_task_handler(PackageReporter, args)
 
 
 def find_reporter_command():
 
=== modified file 'landscape/package/taskhandler.py'
--- landscape/package/taskhandler.py	2011-11-04 11:13:58 +0000
+++ landscape/package/taskhandler.py	2011-11-17 08:50:28 +0000
@@ -232,7 +232,7 @@
         return result
 
 
-def run_task_handler(cls, args, reactor=None, use_apt_facade=False):
+def run_task_handler(cls, args, reactor=None):
     # please only pass reactor when you have totally mangled everything with
     # mocker. Otherwise bad things will happen.
     if reactor is None:
@@ -266,7 +266,7 @@
     package_store = cls.package_store_class(config.store_filename)
     # Delay importing of the facades so that we don't
     # import Smart unless we need to.
-    if use_apt_facade:
+    if os.environ.get("USE_APT_FACADE"):
         from landscape.package.facade import AptFacade
         package_facade = AptFacade()
     else:
 
=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2011-11-09 09:36:24 +0000
+++ landscape/package/tests/test_reporter.py	2011-11-17 08:50:28 +0000
@@ -1267,7 +1267,7 @@
         run_task_handler = self.mocker.replace("landscape.package.taskhandler"
                                                ".run_task_handler",
                                                passthrough=False)
-        run_task_handler(PackageReporter, ["ARGS"], use_apt_facade=False)
+        run_task_handler(PackageReporter, ["ARGS"])
         self.mocker.result("RESULT")
         self.mocker.replay()
 
 
=== modified file 'landscape/package/tests/test_taskhandler.py'
--- landscape/package/tests/test_taskhandler.py	2011-11-04 11:13:58 +0000
+++ landscape/package/tests/test_taskhandler.py	2011-11-17 08:50:28 +0000
@@ -9,10 +9,11 @@
 from landscape.package.taskhandler import (
     PackageTaskHandlerConfiguration, PackageTaskHandler, run_task_handler,
     LazyRemoteBroker)
-from landscape.package.facade import SmartFacade
+from landscape.package.facade import AptFacade, SmartFacade
 from landscape.package.store import HashIdStore, PackageStore
-from landscape.package.tests.helpers import SmartFacadeHelper
-from landscape.tests.helpers import LandscapeTest, BrokerServiceHelper
+from landscape.package.tests.helpers import AptFacadeHelper, SmartFacadeHelper
+from landscape.tests.helpers import (
+    LandscapeTest, BrokerServiceHelper, EnvironSaverHelper)
 from landscape.tests.mocker import ANY, ARGS, MATCH
 
 
@@ -48,7 +49,7 @@
 
 class PackageTaskHandlerTest(LandscapeTest):
 
-    helpers = [SmartFacadeHelper, BrokerServiceHelper]
+    helpers = [SmartFacadeHelper, EnvironSaverHelper, BrokerServiceHelper]
 
     def setUp(self):
 
@@ -326,10 +327,11 @@
         self.assertTrue(isinstance(result, Deferred))
         self.assertTrue(result.called)
 
-    def test_run_task_handler(self):
+    def _mock_run_task_handler(self):
         """
-        The L{run_task_handler} function creates and runs the given task
-        handler with the proper arguments.
+        Mock the different parts of run_task_handler(), to ensure it
+        does what it's supposed to do, without actually creating files
+        and starting processes.
         """
         # This is a slightly lengthy one, so bear with me.
 
@@ -379,6 +381,15 @@
         # Okay, the whole playground is set.
         self.mocker.replay()
 
+        return HandlerMock, handler_args
+
+    def test_run_task_handler(self):
+        """
+        The L{run_task_handler} function creates and runs the given task
+        handler with the proper arguments.
+        """
+        HandlerMock, handler_args = self._mock_run_task_handler()
+
         def assert_task_handler(ignored):
 
             store, facade, broker, config = handler_args
@@ -408,6 +419,34 @@
         result = run_task_handler(HandlerMock, ["-c", self.config_filename])
         return result.addCallback(assert_task_handler)
 
+    def test_run_task_handler_use_apt_facade(self):
+        """
+        The L{run_task_handler} creates an AptFacade instead of
+        SmartFacade, if the USE_APT_FACADE environment variable is set.
+        """
+
+        HandlerMock, handler_args = self._mock_run_task_handler()
+        os.environ["USE_APT_FACADE"] = "1"
+
+        def assert_task_handler(ignored):
+
+            store, facade, broker, config = handler_args
+
+            try:
+                self.assertEqual(type(facade), AptFacade)
+            finally:
+                # Put reactor back in place before returning.
+                self.mocker.reset()
+
+        # Set up using AptFacadeHelper, to get a clean root dir, so
+        # creating an AptFacade with a clean dir is much faster than
+        # using / as root. This also prevents test failures due to tests
+        # using AptFacadeHelper not resetting the apt config, which
+        # isn't trivial to do.
+        AptFacadeHelper().set_up(self)
+        result = run_task_handler(HandlerMock, ["-c", self.config_filename])
+        return result.addCallback(assert_task_handler)
+
     def test_run_task_handler_when_already_locked(self):
 
         lock_path(os.path.join(self.data_path, "package", "default.lock"))