Merge lp:~gocept/landscape-client/fetch-returns-bytes-in-tests into lp:~landscape/landscape-client/trunk

[Steffen Allner]

Return bytes from mocks of landscape.lib.fetch.fetch().

In this MP an inconsistency between tests and actual implementation was fixed. The landscape.lib.fetch.fetch() returns bytes now explicitly, but this was not reflected in the mocks used throughout the landscape-client project.

I tried to convert all return values of the mocks to bytes and adjusted the tests accordingly.

In this MP an inconsistency between tests and actual implementation was fixed. The landscape.lib.fetch.fetch() returns bytes now explicitly, but this was not reflected in the mocks used throughout the landscape-client project.

I tried to convert all return values of the mocks to bytes and adjusted the tests accordingly.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[Steffen Allner (sallner)]

Inline clarification.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-check
Result: Success
Revno: 1000
Branch: lp:~gocept/landscape-client/fetch-returns-bytes-in-tests
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3838/

review: Approve (test results)

[Eric Snow (ericsnowcurrently)]

LGTM

review: Approve

[Simon Poirier (simpoir)]

+1

review: Approve

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2017-03-28 06:09:31 +0000
+++ landscape/configuration.py	2017-04-05 11:38:17 +0000
@@ -9,11 +9,12 @@
 from functools import partial
 import base64
 import getpass
+import io
 import os
 import pwd
 import sys
 
-from landscape.compat import StringIO, input
+from landscape.compat import input
 
 from landscape.lib.tag import is_valid_tag
 
@@ -117,7 +118,8 @@
                         os.environ["https_proxy"] = self.https_proxy
                     content = self.fetch_import_url(self.import_from)
                     parser = self._get_config_object(
-                        alternative_config=StringIO(content))
+                        alternative_config=io.StringIO(
+                            content.decode("utf-8")))
                 elif not os.path.isfile(self.import_from):
                     raise ImportOptionError("File %s doesn't exist." %
                                             self.import_from)

=== modified file 'landscape/lib/tests/test_cloud.py'
--- landscape/lib/tests/test_cloud.py	2017-03-09 14:14:57 +0000
+++ landscape/lib/tests/test_cloud.py	2017-04-05 11:38:17 +0000
@@ -21,14 +21,15 @@
                 return succeed(value)
 
         self.fetch_func = fetch_stub
-        self.add_query_result("instance-id", "i00001")
-        self.add_query_result("ami-id", "ami-00002")
-        self.add_query_result("instance-type", "hs1.8xlarge")
+        self.add_query_result("instance-id", b"i00001")
+        self.add_query_result("ami-id", b"ami-00002")
+        self.add_query_result("instance-type", b"hs1.8xlarge")
 
     def add_query_result(self, name, value):
         """
         Add a url to self.query_results that is then available through
-        self.fetch_func.
+        self.fetch_func. C{value} must be bytes or an Error as the original
+        fetch returns bytes.
         """
         url = "%s/meta-data/%s" % (EC2_API, name)
         self.query_results[url] = value
@@ -84,9 +85,9 @@
 
     def test_fetch_ec2_meta_data_truncates(self):
         """L{_fetch_ec2_meta_data} truncates values that are too long."""
-        self.add_query_result("ami-id", "a" * MAX_LENGTH * 5)
-        self.add_query_result("instance-id", "b" * MAX_LENGTH * 5)
-        self.add_query_result("instance-type", "c" * MAX_LENGTH * 5)
+        self.add_query_result("ami-id", b"a" * MAX_LENGTH * 5)
+        self.add_query_result("instance-id", b"b" * MAX_LENGTH * 5)
+        self.add_query_result("instance-type", b"c" * MAX_LENGTH * 5)
         deferred = fetch_ec2_meta_data(fetch=self.fetch_func)
         result = self.successResultOf(deferred)
         self.assertEqual(
@@ -107,7 +108,7 @@
         self.successResultOf(
             _fetch_ec2_item(
                 "instance-type", accumulate, fetch=self.fetch_func))
-        self.assertEqual(["i00001", "hs1.8xlarge"], accumulate)
+        self.assertEqual([b"i00001", b"hs1.8xlarge"], accumulate)
 
     def test_wb_fetch_ec2_item_error_returns_failure(self):
         """

=== modified file 'landscape/manager/tests/test_scriptexecution.py'
--- landscape/manager/tests/test_scriptexecution.py	2017-03-27 11:52:09 +0000
+++ landscape/manager/tests/test_scriptexecution.py	2017-04-05 11:38:17 +0000
@@ -209,9 +209,10 @@
             mock_umask.assert_has_calls([mock.call(0o022)])
             mock_mkdtemp.assert_called_with()
 
-        def cleanup(_):
+        def cleanup(result):
             patch_umask.stop()
             patch_mkdtemp.stop()
+            return result
 
         return result.addErrback(check).addBoth(cleanup)
 
@@ -243,7 +244,7 @@
         patch_fetch = mock.patch(
             "landscape.manager.scriptexecution.fetch_async")
         mock_fetch = patch_fetch.start()
-        mock_fetch.return_value = succeed("some other data")
+        mock_fetch.return_value = succeed(b"some other data")
 
         headers = {"User-Agent": "landscape-client/%s" % VERSION,
                    "Content-Type": "application/octet-stream",
@@ -260,8 +261,10 @@
                 "https://localhost/attachment/14", headers=headers,
                 cainfo=None)
 
-        def cleanup(_):
+        def cleanup(result):
             patch_fetch.stop()
+            # We have to return the Failure or result to get a working test.
+            return result
 
         return result.addCallback(check).addBoth(cleanup)
 
@@ -281,7 +284,7 @@
         patch_fetch = mock.patch(
             "landscape.manager.scriptexecution.fetch_async")
         mock_fetch = patch_fetch.start()
-        mock_fetch.return_value = succeed("some other data")
+        mock_fetch.return_value = succeed(b"some other data")
 
         headers = {"User-Agent": "landscape-client/%s" % VERSION,
                    "Content-Type": "application/octet-stream",
@@ -298,8 +301,9 @@
                 "https://localhost/attachment/14", headers=headers,
                 cainfo="/some/key")
 
-        def cleanup(_):
+        def cleanup(result):
             patch_fetch.stop()
+            return result
 
         return result.addCallback(check).addBoth(cleanup)
 
@@ -357,8 +361,9 @@
             mock_chown.assert_called_with()
             self.assertEqual(result, "foobar")
 
-        def cleanup(_):
+        def cleanup(result):
             patch_chown.stop()
+            return result
 
         return result.addErrback(check).addBoth(cleanup)
 
@@ -397,8 +402,9 @@
         def check(result):
             mock_getpwnam.assert_called_with("user")
 
-        def cleanup(_):
+        def cleanup(result):
             patch_getpwnam.stop()
+            return result
 
         return result.addCallback(check).addBoth(cleanup)
 
@@ -437,8 +443,9 @@
             mock_chown.assert_has_calls(
                 [mock.call(mock.ANY, uid, gid) for x in range(3)])
 
-        def cleanup(_):
+        def cleanup(result):
             patch_chown.stop()
+            return result
 
         return result.addCallback(check).addBoth(cleanup)
 

=== modified file 'landscape/monitor/tests/test_computerinfo.py'
--- landscape/monitor/tests/test_computerinfo.py	2017-03-29 14:14:35 +0000
+++ landscape/monitor/tests/test_computerinfo.py	2017-04-05 11:38:17 +0000
@@ -62,14 +62,15 @@
                 return succeed(value)
 
         self.fetch_func = fetch_stub
-        self.add_query_result("instance-id", "i00001")
-        self.add_query_result("ami-id", "ami-00002")
-        self.add_query_result("instance-type", "hs1.8xlarge")
+        self.add_query_result("instance-id", b"i00001")
+        self.add_query_result("ami-id", b"ami-00002")
+        self.add_query_result("instance-type", b"hs1.8xlarge")
 
     def add_query_result(self, name, value):
         """
         Add a url to self.query_results that is then available through
-        self.fetch_func.
+        self.fetch_func. C{value} must be bytes or an Error as the original
+        fetch returns bytes.
         """
         url = "http://169.254.169.254/latest/meta-data/" + name
         self.query_results[url] = value
@@ -494,7 +495,7 @@
         self.assertEqual(1, plugin._cloud_retries)
         self.assertEqual(None, result)
         # Fix the error condition for the retry.
-        self.add_query_result("ami-id", "ami-00002")
+        self.add_query_result("ami-id", b"ami-00002")
         result = yield plugin._fetch_ec2_meta_data()
         self.assertEqual({"instance-id": u"i00001", "ami-id": u"ami-00002",
                           "instance-type": u"hs1.8xlarge"},

=== modified file 'landscape/package/reporter.py'
--- landscape/package/reporter.py	2017-03-21 17:00:34 +0000
+++ landscape/package/reporter.py	2017-04-05 11:38:17 +0000
@@ -17,7 +17,7 @@
 from landscape.lib.sequenceranges import sequence_to_ranges
 from landscape.lib.twisted_util import gather_results, spawn_process
 from landscape.lib.fetch import fetch_async
-from landscape.lib.fs import touch_file
+from landscape.lib.fs import touch_file, create_binary_file
 from landscape.lib.lsb_release import parse_lsb_release, LSB_RELEASE_FILENAME
 
 from landscape.package.taskhandler import (
@@ -131,9 +131,7 @@
             url = str(base_url + os.path.basename(hash_id_db_filename))
 
             def fetch_ok(data):
-                hash_id_db_fd = open(hash_id_db_filename, "w")
-                hash_id_db_fd.write(data)
-                hash_id_db_fd.close()
+                create_binary_file(hash_id_db_filename, data)
                 logging.info("Downloaded hash=>id database from %s" % url)
 
             def fetch_error(failure):

=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2017-03-21 17:00:34 +0000
+++ landscape/package/tests/test_reporter.py	2017-04-05 11:38:17 +0000
@@ -270,7 +270,7 @@
         return deferred.addCallback(got_result)
 
     @mock.patch("landscape.package.reporter.fetch_async",
-                return_value=succeed("hash-ids"))
+                return_value=succeed(b"hash-ids"))
     @mock.patch("logging.info", return_value=None)
     def test_fetch_hash_id_db(self, logging_mock, mock_fetch_async):
 
@@ -308,7 +308,7 @@
         return result
 
     @mock.patch("landscape.package.reporter.fetch_async",
-                return_value=succeed("hash-ids"))
+                return_value=succeed(b"hash-ids"))
     @mock.patch("logging.info", return_value=None)
     def test_fetch_hash_id_db_with_proxy(self, logging_mock, mock_fetch_async):
         """fetching hash-id-db uses proxy settings"""
@@ -414,7 +414,7 @@
         return result
 
     @mock.patch("landscape.package.reporter.fetch_async",
-                return_value=succeed("hash-ids"))
+                return_value=succeed(b"hash-ids"))
     def test_fetch_hash_id_db_with_default_url(self, mock_fetch_async):
         # Let's say package_hash_id_url is not set but url is
         self.config.data_path = self.makeDir()
@@ -507,7 +507,7 @@
         return result
 
     @mock.patch("landscape.package.reporter.fetch_async",
-                return_value=succeed("hash-ids"))
+                return_value=succeed(b"hash-ids"))
     def test_fetch_hash_id_db_with_custom_certificate(self, mock_fetch_async):
         """
         The L{PackageReporter.fetch_hash_id_db} method takes into account the

=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2017-03-28 06:09:31 +0000
+++ landscape/tests/test_configuration.py	2017-04-05 11:38:17 +0000
@@ -1595,13 +1595,13 @@
             self, mock_sysvconfig, mock_fetch, mock_print_text):
         mock_sysvconfig().restart_landscape.return_value = True
         configuration = (
-            "[client]\n"
-            "computer_title = New Title\n"
-            "account_name = New Name\n"
-            "registration_key = New Password\n"
-            "http_proxy = http://new.proxy\n"
-            "https_proxy = https://new.proxy\n"
-            "url = http://new.url\n")
+            b"[client]\n"
+            b"computer_title = New Title\n"
+            b"account_name = New Name\n"
+            b"registration_key = New Password\n"
+            b"http_proxy = http://new.proxy\n"
+            b"https_proxy = https://new.proxy\n"
+            b"url = http://new.url\n")
 
         mock_fetch.return_value = configuration
 
@@ -1670,7 +1670,7 @@
             "Fetching configuration from https://config.url...")
 
     @mock.patch("landscape.configuration.print_text")
-    @mock.patch("landscape.configuration.fetch", return_value="")
+    @mock.patch("landscape.configuration.fetch", return_value=b"")
     def test_import_from_url_with_empty_content(
             self, mock_fetch, mock_print_text):
         # Use a command line option as well to test the precedence.
@@ -1687,7 +1687,7 @@
 
     @mock.patch("landscape.configuration.print_text")
     @mock.patch("landscape.configuration.fetch",
-                return_value="<strong>BOGUS!</strong>")
+                return_value=b"<strong>BOGUS!</strong>")
     def test_import_from_url_with_bogus_content(
             self, mock_fetch, mock_print_text):
         # Use a command line option as well to test the precedence.