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

[Steffen Allner]

Update tests for landscape.package.changer and improve landscape.lib.tests.test_fs tests to cover Unicode strings too.

This MP considers the tests for landscape.package.changer, stressing the fact that hashes are bytes in tests and make the outcome of a function more deterministic with sorting.

Additionally, changed the self.assertFileContent to open the files explicitly in binary mode, which made me realise that tests in l.l.t.test_fs were insufficient. Now they test properly.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

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

review: Approve (test results)

[Steffen Allner (sallner)]

Add some inline comments.

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

Thanks for the fixes, looks good.

Answer to your where-do-you-put-sorted question inline.

review: Approve

[Daniel Havlik (nilo)]

+1

review: Approve

[🤖 Landscape Builder (landscape-builder)]

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

text conflict in py3_ready_tests

[🤖 Landscape Builder (landscape-builder)]

No approved revision specified.

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

Ugh, each of the branches is now going to have conflicts in py3_ready_tests: it's going to be a bit annoying, but we'll have to land them one-by-one and re-merge trunk to keep the diffs nice (i.e. add a test to py3_ready_tests in a commit that fixes it). If that sounds too annoying, you can drop changes to py3_ready_tests and do a small branch after everything else lands.

[Steffen Allner (sallner)]

> Ugh, each of the branches is now going to have conflicts in py3_ready_tests:
> it's going to be a bit annoying, but we'll have to land them one-by-one and
> re-merge trunk to keep the diffs nice (i.e. add a test to py3_ready_tests in a
> commit that fixes it). If that sounds too annoying, you can drop changes to
> py3_ready_tests and do a small branch after everything else lands.

Yes, I tried to separate it a bit beforehand, but it did not work. I will do the merges. Then I can also combine it for the last merge to landscape.package.test.

Will Do a separate branch next time.

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

Not at all: I like it when the test that should now pass is in the branch as well.

You would probably just have to do them all by depending on each other in a series. There's a bzr plugin that helps with that (bzr pipelines), but it's badly documented so you probably don't want to deal with that :-)

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

Please merge trunk into this branch so I can get it landed.

[Steffen Allner (sallner)]

> Please merge trunk into this branch so I can get it landed.

I have merged the trunk into the py3-facade branch, please merge it, then I will continue here.

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

991. By Steffen Allner on 2017-03-22

Backmerge from trunk.

[Steffen Allner (sallner)]

Conflicts resolved.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 991
Branch: lp:~gocept/landscape-client/py3-package-changer
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3702/

review: Needs Fixing (test results)

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

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

review: Approve (test results)

=== modified file 'landscape/lib/tests/test_fs.py'
--- landscape/lib/tests/test_fs.py	2017-03-14 14:45:53 +0000
+++ landscape/lib/tests/test_fs.py	2017-03-22 15:14:59 +0000
@@ -8,7 +8,7 @@
 
 from landscape.tests.helpers import LandscapeTest
 
-from landscape.lib.fs import append_text_file, touch_file
+from landscape.lib.fs import append_text_file, append_binary_file, touch_file
 from landscape.lib.fs import read_text_file, read_binary_file
 
 
@@ -98,7 +98,7 @@
         path = self.makeFile()
         touch_file(path)
         utime_mock.assert_called_once_with(path, None)
-        self.assertFileContent(path, "")
+        self.assertFileContent(path, b"")
 
     def test_touch_file_multiple_times(self):
         """
@@ -107,7 +107,7 @@
         path = self.makeFile()
         touch_file(path)
         touch_file(path)
-        self.assertFileContent(path, "")
+        self.assertFileContent(path, b"")
 
     def test_touch_file_with_offset_seconds(self):
         """
@@ -127,7 +127,7 @@
         utime_mock.assert_called_once_with(
             path, (expected_time, expected_time))
 
-        self.assertFileContent(path, "")
+        self.assertFileContent(path, b"")
 
 
 class AppendFileTest(LandscapeTest):
@@ -137,8 +137,8 @@
         The L{append_text_file} function appends contents to an existing file.
         """
         existing_file = self.makeFile("foo bar")
-        append_text_file(existing_file, " baz")
-        self.assertFileContent(existing_file, "foo bar baz")
+        append_text_file(existing_file, u" baz ☃")
+        self.assertFileContent(existing_file, b"foo bar baz \xe2\x98\x83")
 
     def test_append_text_no_file(self):
         """
@@ -146,16 +146,16 @@
         exist already.
         """
         new_file = os.path.join(self.makeDir(), "new_file")
-        append_text_file(new_file, "contents")
-        self.assertFileContent(new_file, "contents")
+        append_text_file(new_file, u"contents ☃")
+        self.assertFileContent(new_file, b"contents \xe2\x98\x83")
 
     def test_append_existing_binary_file(self):
         """
         The L{append_text_file} function appends contents to an existing file.
         """
         existing_file = self.makeFile("foo bar")
-        append_text_file(existing_file, " baz")
-        self.assertFileContent(existing_file, "foo bar baz")
+        append_binary_file(existing_file, b" baz \xe2\x98\x83")
+        self.assertFileContent(existing_file, b"foo bar baz \xe2\x98\x83")
 
     def test_append_binary_no_file(self):
         """
@@ -163,5 +163,5 @@
         exist already.
         """
         new_file = os.path.join(self.makeDir(), "new_file")
-        append_text_file(new_file, "contents")
-        self.assertFileContent(new_file, "contents")
+        append_binary_file(new_file, b"contents \xe2\x98\x83")
+        self.assertFileContent(new_file, b"contents \xe2\x98\x83")

=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2017-03-21 10:21:50 +0000
+++ landscape/package/facade.py	2017-03-22 15:14:59 +0000
@@ -186,7 +186,8 @@
 
     def get_package_holds(self):
         """Return the name of all the packages that are on hold."""
-        return [version.package.name for version in self.get_locked_packages()]
+        return sorted([version.package.name
+                       for version in self.get_locked_packages()])
 
     def _set_dpkg_selections(self, selection):
         """Set the dpkg selection.

=== modified file 'landscape/package/tests/test_changer.py'
--- landscape/package/tests/test_changer.py	2017-03-13 15:38:09 +0000
+++ landscape/package/tests/test_changer.py	2017-03-22 15:14:59 +0000
@@ -196,27 +196,27 @@
         self.assertTrue(self.store.get_next_task("changer"))
 
     def test_install_unknown_package(self):
-        self.store.set_hash_ids({"hash": 456})
+        self.store.set_hash_ids({b"hash": 456})
         self.store.add_task("changer",
                             {"type": "change-packages", "install": [456],
                              "operation-id": 123})
 
         self.changer.handle_tasks()
 
-        self.assertIn("Package data not yet synchronized with server ('hash')",
-                      self.logfile.getvalue())
+        self.assertIn("Package data not yet synchronized with server (%r)" %
+                      b"hash", self.logfile.getvalue())
         self.assertTrue(self.store.get_next_task("changer"))
 
     def test_remove_unknown_package(self):
-        self.store.set_hash_ids({"hash": 456})
+        self.store.set_hash_ids({b"hash": 456})
         self.store.add_task("changer",
                             {"type": "change-packages", "remove": [456],
                              "operation-id": 123})
 
         self.changer.handle_tasks()
 
-        self.assertIn("Package data not yet synchronized with server ('hash')",
-                      self.logfile.getvalue())
+        self.assertIn("Package data not yet synchronized with server (%r)" %
+                      b"hash", self.logfile.getvalue())
         self.assertTrue(self.store.get_next_task("changer"))
 
     def test_unknown_data_timeout(self):
@@ -972,7 +972,7 @@
 
         def assert_result(result):
             self.facade.reload_channels()
-            self.assertEqual(["foo", "bar"], self.facade.get_package_holds())
+            self.assertEqual(["bar", "foo"], self.facade.get_package_holds())
 
         result = self.changer.handle_tasks()
         return result.addCallback(assert_result)

=== modified file 'landscape/tests/helpers.py'
--- landscape/tests/helpers.py	2017-03-10 15:19:26 +0000
+++ landscape/tests/helpers.py	2017-03-22 15:14:59 +0000
@@ -178,9 +178,8 @@
         return deferred.addCallback(self.assertEqual, result)
 
     def assertFileContent(self, filename, expected_content):
-        fd = open(filename)
-        actual_content = fd.read()
-        fd.close()
+        with open(filename, "rb") as fd:
+            actual_content = fd.read()
         self.assertEqual(expected_content, actual_content)
 
     def assertConfigEqual(self, first, second):

=== modified file 'py3_ready_tests'
--- py3_ready_tests	2017-03-22 13:09:11 +0000
+++ py3_ready_tests	2017-03-22 15:14:59 +0000
@@ -2,6 +2,7 @@
 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