Merge lp:~chad.smith/landscape-client/mocker-to-mock-package-reporter into lp:~landscape/landscape-client/trunk

[Chad Smith]

Phase 1, replace basic logging mocker w/ mock patches.

This file has significant mocker use. It is separated into parts to aid reviews.

Phase 1, replace basic logging mocker w/ mock patches.

This file has significant mocker use. It is separated into parts to aid reviews.

Testing instructions:
trial landscape.package.tests.test_reporter

[Adam Collard (adam-collard)]

some nits, +1

review: Approve

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 884
Branch: lp:~chad.smith/landscape-client/mocker-to-mock-package-reporter
Jenkins: https://ci.lscape.net/job/latch-test/5092/

review: Needs Fixing (test results)

[lp:~chad.smith/landscape-client/mocker-to-mock-package-reporter]

885. By Chad Smith on 2016-06-16

address sparkiegeek's review: isn't it ironic ...

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[Chris Glass (tribaal)]

+1

review: Approve

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 885
Branch: lp:~chad.smith/landscape-client/mocker-to-mock-package-reporter
Jenkins: https://ci.lscape.net/job/latch-test/5098/

review: Approve (test results)

=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2015-01-12 13:18:15 +0000
+++ landscape/package/tests/test_reporter.py	2016-06-16 16:26:14 +0000
@@ -23,6 +23,7 @@
 from landscape.tests.helpers import (
     LandscapeTest, BrokerServiceHelper, EnvironSaverHelper)
 from landscape.tests.mocker import ANY
+from mock import patch, call
 from landscape.reactor import FakeReactor
 
 SAMPLE_LSB_RELEASE = "DISTRIB_CODENAME=codename\n"
@@ -265,7 +266,8 @@
         deferred = self.reporter.handle_tasks()
         return deferred.addCallback(got_result)
 
-    def test_fetch_hash_id_db(self):
+    @patch("logging.info", return_value=None)
+    def test_fetch_hash_id_db(self, logging_mock):
 
         # Assume package_hash_id_url is set
         self.config.data_path = self.makeDir()
@@ -289,11 +291,6 @@
         fetch_async_result.callback("hash-ids")
         self.mocker.result(fetch_async_result)
 
-        # The download should be properly logged
-        logging_mock = self.mocker.replace("logging.info")
-        logging_mock("Downloaded hash=>id database from %s" % hash_id_db_url)
-        self.mocker.result(None)
-
         # We don't have our hash=>id database yet
         self.assertFalse(os.path.exists(hash_id_db_filename))
 
@@ -307,6 +304,8 @@
             self.assertEqual(open(hash_id_db_filename).read(), "hash-ids")
         result.addCallback(callback)
 
+        logging_mock.assert_called_once_with(
+            "Downloaded hash=>id database from %s" % hash_id_db_url)
         return result
 
     def test_fetch_hash_id_db_does_not_download_twice(self):
@@ -346,7 +345,8 @@
 
         return result
 
-    def test_fetch_hash_id_db_undetermined_server_uuid(self):
+    @patch("logging.warning", return_value=None)
+    def test_fetch_hash_id_db_undetermined_server_uuid(self, logging_mock):
         """
         If the server-uuid can't be determined for some reason, no download
         should be attempted and the failure should be properly logged.
@@ -354,16 +354,14 @@
         message_store = self.broker_service.message_store
         message_store.set_server_uuid(None)
 
-        logging_mock = self.mocker.replace("logging.warning")
-        logging_mock("Couldn't determine which hash=>id database to use: "
-                     "server UUID not available")
-        self.mocker.result(None)
-        self.mocker.replay()
-
         result = self.reporter.fetch_hash_id_db()
+        logging_mock.assert_called_once_with(
+            "Couldn't determine which hash=>id database to use: "
+            "server UUID not available")
         return result
 
-    def test_fetch_hash_id_db_undetermined_codename(self):
+    @patch("logging.warning", return_value=None)
+    def test_fetch_hash_id_db_undetermined_codename(self, logging_mock):
 
         # Fake uuid
         message_store = self.broker_service.message_store
@@ -372,20 +370,15 @@
         # Undetermined codename
         self.reporter.lsb_release_filename = self.makeFile("Foo=bar")
 
-        # The failure should be properly logged
-        logging_mock = self.mocker.replace("logging.warning")
-        logging_mock("Couldn't determine which hash=>id database to use: "
-                     "missing code-name key in %s" %
-                     self.reporter.lsb_release_filename)
-        self.mocker.result(None)
-
-        # Go!
-        self.mocker.replay()
         result = self.reporter.fetch_hash_id_db()
 
+        logging_mock.assert_called_once_with(
+            "Couldn't determine which hash=>id database to use: "
+            "missing code-name key in %s" % self.reporter.lsb_release_filename)
         return result
 
-    def test_fetch_hash_id_db_undetermined_arch(self):
+    @patch("logging.warning", return_value=None)
+    def test_fetch_hash_id_db_undetermined_arch(self, logging_mock):
 
         # Fake uuid and codename
         message_store = self.broker_service.message_store
@@ -395,16 +388,10 @@
         # Undetermined arch
         self.facade.set_arch("")
 
-        # The failure should be properly logged
-        logging_mock = self.mocker.replace("logging.warning")
-        logging_mock("Couldn't determine which hash=>id database to use: "
-                     "unknown dpkg architecture")
-        self.mocker.result(None)
-
-        # Go!
-        self.mocker.replay()
         result = self.reporter.fetch_hash_id_db()
-
+        logging_mock.assert_called_once_with(
+            "Couldn't determine which hash=>id database to use: "
+            "unknown dpkg architecture")
         return result
 
     def test_fetch_hash_id_db_with_default_url(self):
@@ -444,7 +431,8 @@
         result.addCallback(callback)
         return result
 
-    def test_fetch_hash_id_db_with_download_error(self):
+    @patch("logging.warning", return_value=None)
+    def test_fetch_hash_id_db_with_download_error(self, logging_mock):
 
         # Assume package_hash_id_url is set
         self.config.data_path = self.makeDir()
@@ -465,11 +453,6 @@
         fetch_async_result.errback(FetchError("fetch error"))
         self.mocker.result(fetch_async_result)
 
-        # The failure should be properly logged
-        logging_mock = self.mocker.replace("logging.warning")
-        logging_mock("Couldn't download hash=>id database: fetch error")
-        self.mocker.result(None)
-
         # Now go!
         self.mocker.replay()
         result = self.reporter.fetch_hash_id_db()
@@ -482,9 +465,13 @@
             self.assertEqual(os.path.exists(hash_id_db_filename), False)
         result.addCallback(callback)
 
+        logging_mock.assert_called_once_with(
+            "Couldn't download hash=>id database: fetch error")
+
         return result
 
-    def test_fetch_hash_id_db_with_undetermined_url(self):
+    @patch("logging.warning", return_value=None)
+    def test_fetch_hash_id_db_with_undetermined_url(self, logging_mock):
 
         # We don't know where to fetch the hash=>id database from
         self.config.url = None
@@ -496,14 +483,6 @@
         self.reporter.lsb_release_filename = self.makeFile(SAMPLE_LSB_RELEASE)
         self.facade.set_arch("arch")
 
-        # The failure should be properly logged
-        logging_mock = self.mocker.replace("logging.warning")
-        logging_mock("Can't determine the hash=>id database url")
-        self.mocker.result(None)
-
-        # Let's go
-        self.mocker.replay()
-
         result = self.reporter.fetch_hash_id_db()
 
         # We shouldn't have any hash=>id database
@@ -514,6 +493,8 @@
             self.assertEqual(os.path.exists(hash_id_db_filename), False)
         result.addCallback(callback)
 
+        logging_mock.assert_called_once_with(
+            "Can't determine the hash=>id database url")
         return result
 
     def test_fetch_hash_id_db_with_custom_certificate(self):
@@ -1234,17 +1215,13 @@
         self.reactor.advance(0)
         return result
 
-    def test_run_apt_update_warns_about_lock_failure(self):
+    @patch("logging.warning", return_value=None)
+    def test_run_apt_update_warns_about_lock_failure(self, logging_mock):
         """
         The L{PackageReporter.run_apt_update} method logs a warnings when
         apt-update fails acquiring the lock.
         """
         self._make_fake_apt_update(code=100)
-        logging_mock = self.mocker.replace("logging.warning")
-        logging_mock("Could not acquire the apt lock. Retrying in 20 seconds.")
-        logging_mock("Could not acquire the apt lock. Retrying in 40 seconds.")
-        logging_mock("'%s' exited with status 100 ()" %
-                     self.reporter.apt_update_filename)
 
         spawn_mock = self.mocker.replace(
             "landscape.lib.twisted_util.spawn_process")
@@ -1269,6 +1246,12 @@
 
         result.addCallback(callback)
         self.reactor.advance(60)
+        message = "Could not acquire the apt lock. Retrying in {} seconds."
+        calls = [call(message.format(20)),
+                 call(message.format(40)),
+                 call("'{}' exited with status 1000 ()".format(
+                     self.reporter.apt_update_filename))]
+        logging_mock.has_calls(calls)
         return result
 
     def test_run_apt_update_stops_retrying_after_lock_acquired(self):