Merge lp:~gocept/landscape-client/py3-package-releaseupgrader into lp:~landscape/landscape-client/trunk

[Steffen Allner]

Use bytes/unicode where appropriate in landscape.package.releaseupgrader module and ensure tests for this module work in both py2 and py3.

This MP considers th landscape.package.releaseupgrader module and its tests. Important fixes were in handling fetch_async() results in the mock, use bytes when checking communication with processes and use unicode for log output.

Additionally, I removed a dependency to landscape.compat by using io.StringIO() directly.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 992
Branch: lp:~gocept/landscape-client/py3-package-releaseupgrader
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3695/

review: Approve (test results)

[Daniel Havlik (nilo)]

+1

review: Approve

[Данило Шеган (danilo)]

Looks good, +1.

review: Approve

[🤖 Landscape Builder (landscape-builder)]

Attempt to merge into lp:landscape-client failed due to conflicts:

text conflict in py3_ready_tests

[Данило Шеган (danilo)]

Please re-merge trunk so we can land this.

[lp:~gocept/landscape-client/py3-package-releaseupgrader]

993. By Steffen Allner on 2017-03-23

Backmerge from trunk.

994. By Steffen Allner on 2017-03-23

As we now open the fila in binary mode in the helper, we need to adapt the tests here.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 994
Branch: lp:~gocept/landscape-client/py3-package-releaseupgrader
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3708/

review: Approve (test results)

[Steffen Allner (sallner)]

I merged the trunk and repaired failing tests on the way.

=== modified file 'landscape/package/releaseupgrader.py'
--- landscape/package/releaseupgrader.py	2017-03-13 15:38:09 +0000
+++ landscape/package/releaseupgrader.py	2017-03-23 07:44:11 +0000
@@ -1,14 +1,14 @@
+import grp
+import io
+import logging
 import os
-import sys
-import grp
 import pwd
 import shutil
-import logging
+import sys
 import tarfile
 
 from twisted.internet.defer import succeed
 
-from landscape.compat import StringIO
 from landscape.lib.fetch import url_to_filename, fetch_to_files
 from landscape.lib.lsb_release import parse_lsb_release, LSB_RELEASE_FILENAME
 from landscape.lib.gpg import gpg_verify
@@ -186,18 +186,18 @@
         @param err: The standard error of the upgrade-tool process.
         @return: A text aggregating the process output, error and log files.
         """
-        buf = StringIO()
+        buf = io.StringIO()
 
         for label, content in [("output", out), ("error", err)]:
             if content:
-                buf.write("=== Standard %s ===\n\n%s\n\n" % (label, content))
+                buf.write(u"=== Standard %s ===\n\n%s\n\n" % (label, content))
 
         for basename in sorted(os.listdir(self.logs_directory)):
             if not basename.endswith(".log"):
                 continue
             filename = os.path.join(self.logs_directory, basename)
             content = read_text_file(filename, - self.logs_limit)
-            buf.write("=== %s ===\n\n%s\n\n" % (basename, content))
+            buf.write(u"=== %s ===\n\n%s\n\n" % (basename, content))
 
         return buf.getvalue()
 
@@ -224,6 +224,9 @@
 
         def send_operation_result(args):
             out, err, code = args
+            out = out.decode("utf-8")
+            err = err.decode("utf-8")
+
             if code == 0:
                 status = SUCCEEDED
             else:

=== modified file 'landscape/package/tests/test_releaseupgrader.py'
--- landscape/package/tests/test_releaseupgrader.py	2017-01-11 10:39:32 +0000
+++ landscape/package/tests/test_releaseupgrader.py	2017-03-23 07:44:11 +0000
@@ -3,11 +3,9 @@
 import signal
 import tarfile
 import unittest
-from unittest import skipIf
 
 from twisted.internet import reactor
 from twisted.internet.defer import succeed, fail, Deferred
-from twisted.python.compat import _PY3
 
 from landscape.lib.gpg import InvalidGPGSignature
 from landscape.lib.fetch import HTTPCodeError
@@ -62,8 +60,8 @@
         signature_url = "http://some/where/karmic.tar.gz.gpg"
 
         method_returns = {
-            tarball_url: succeed("tarball"),
-            signature_url: succeed("signature")}
+            tarball_url: succeed(b"tarball"),
+            signature_url: succeed(b"signature")}
 
         def side_effect(param):
             return method_returns[param]
@@ -77,9 +75,9 @@
         def check_result(ignored):
             directory = self.config.upgrade_tool_directory
             self.assertFileContent(
-                os.path.join(directory, "karmic.tar.gz"), "tarball")
+                os.path.join(directory, "karmic.tar.gz"), b"tarball")
             self.assertFileContent(
-                os.path.join(directory, "karmic.tar.gz.gpg"), "signature")
+                os.path.join(directory, "karmic.tar.gz.gpg"), b"signature")
             self.assertIn("INFO: Successfully fetched upgrade-tool files",
                           self.logfile.getvalue())
             calls = [mock.call(tarball_url), mock.call(signature_url)]
@@ -98,8 +96,8 @@
         signature_url = "http://some/where/karmic.tar.gz.gpg"
 
         method_returns = {
-            tarball_url: succeed("tarball"),
-            signature_url: fail(HTTPCodeError(404, "not found"))}
+            tarball_url: succeed(b"tarball"),
+            signature_url: fail(HTTPCodeError(404, b"not found"))}
 
         def side_effect(param):
             return method_returns[param]
@@ -181,7 +179,7 @@
         def check_result(ignored):
             filename = os.path.join(self.config.upgrade_tool_directory, "file")
             self.assertTrue(os.path.exists(filename))
-            self.assertFileContent(filename, "data\n")
+            self.assertFileContent(filename, b"data\n")
 
         result.addCallback(check_result)
         return result
@@ -198,9 +196,9 @@
 
         def check_result(ignored):
             self.assertFileContent(mirrors_filename,
-                                   "ftp://ftp.lug.ro/ubuntu/\n"
-                                   "http://ppa.launchpad.net/landscape/"
-                                   "trunk/ubuntu/\n")
+                                   b"ftp://ftp.lug.ro/ubuntu/\n"
+                                   b"http://ppa.launchpad.net/landscape/"
+                                   b"trunk/ubuntu/\n")
 
         result = self.upgrader.tweak("hardy")
         result.addCallback(check_result)
@@ -286,8 +284,7 @@
                          "stderr\n\n"
                          "=== main.log ===\n\n"
                          "long log\n\n")
-    
-    @skipIf(_PY3, 'Takes long with Python3, probably unclean Reactor')
+
     def test_upgrade(self):
         """
         The L{ReleaseUpgrader.upgrade} method spawns the appropropriate
@@ -339,7 +336,6 @@
 
         return deferred.addBoth(cleanup)
 
-    @skipIf(_PY3, 'Takes long with Python3, probably unclean Reactor')
     def test_upgrade_with_env_variables(self):
         """
         The L{ReleaseUpgrader.upgrade} method optionally sets environment
@@ -499,8 +495,6 @@
 
         return deferred.addBoth(cleanup)
 
-
-    @skipIf(_PY3, 'Takes long with Python3, probably unclean Reactor')
     def test_finish(self):
         """
         The L{ReleaseUpgrader.finish} method wipes the upgrade-tool directory
@@ -527,9 +521,10 @@
             def check_result(args):
                 out, err, code = args
                 self.assertFalse(os.path.exists(upgrade_tool_directory))
-                self.assertEqual(out, "--force-apt-update\n%s\n"
-                                  % os.getcwd())
-                self.assertEqual(err, "")
+                self.assertEqual(
+                    out,
+                    b"--force-apt-update\n%s\n" % os.getcwd().encode("utf-8"))
+                self.assertEqual(err, b"")
                 self.assertEqual(code, 0)
                 find_reporter_mock.assert_called_once_with()
 
@@ -602,9 +597,9 @@
 
             def check_result(args):
                 out, err, code = args
-                self.assertEqual(out, "--force-apt-update "
-                                       "--config=/some/config\n")
-                self.assertEqual(err, "")
+                self.assertEqual(out, b"--force-apt-update "
+                                      b"--config=/some/config\n")
+                self.assertEqual(err, b"")
                 self.assertEqual(code, 0)
                 find_reporter_mock.assert_called_once_with()
 

=== modified file 'py3_ready_tests'
--- py3_ready_tests	2017-03-22 15:14:41 +0000
+++ py3_ready_tests	2017-03-23 07:44:11 +0000
@@ -1,10 +1,12 @@
 landscape.lib.tests
 landscape.sysinfo.tests
-landscape.package.tests.test_store
-landscape.package.tests.test_reporter
-landscape.package.tests.test_changer
-
-landscape.package.tests.test_facade
-landscape.package.tests.test_skeleton
-landscape.package.tests.test_taskhandler
+landscape.package.tests
+
+
+
+
+
+
+
+