Merge lp:~gocept/landscape-client/py3-broker-exchange into lp:~landscape/landscape-client/trunk

[Steffen Allner]

Port landscape/broker/exchange.py to Python 3.

This mostly involved bytes/unicode fixes.

This patch also switches to using Unittest.assertCouldEqual (from assertItemsEqual).

As the diff for the landscape.broker module would be to large, I MP the submodules separately.

Here we have landscape.broker.exchange, where the tests were failing because bytes were not uses as input when required by a schema ("api") and the fact that digests were attempted on unicodes.

Additionally, I needed assertCountEqual to adjust the a test for non-deterministic sets in Python 3 so I added an alias to LandscapeTest.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 973
Branch: lp:~gocept/landscape-client/py3-broker-exchange
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3713/

review: Approve (test results)

[Adam Collard (adam-collard)]

Looks good, one suggestion in-line for a small clean-up. +1

review: Approve

[Daniel Havlik (nilo)]

+1

review: Approve

[Steffen Allner (sallner)]

It seems not to be so easy, see inline comment.

[Adam Collard (adam-collard)]

Reply to inline comment with fixed code.

[lp:~gocept/landscape-client/py3-broker-exchange]

974. By Steffen Allner on 2017-03-27

Directly use an alias instead of a wrapping method.

[Steffen Allner (sallner)]

Pushed a new revision to reduce code.

[lp:~gocept/landscape-client/py3-broker-exchange]

975. By Steffen Allner on 2017-03-27

Backmerge from trunk

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 975
Branch: lp:~gocept/landscape-client/py3-broker-exchange
Jenkins: https://ci.lscape.net/job/latch-test-xenial/3783/

review: Approve (test results)

[Eric Snow (ericsnowcurrently)]

LGTM

review: Approve

[Steffen Allner (sallner)]

> LGTM

Thanks for reviewing. Danilo used to adapt the commit message to a way that suites your needs in the landscape project, and then starts the merging process. I would be happy, if we could keep it like that, as I am still quite unfamiliar with launchpad an the workflow.

=== modified file 'landscape/broker/exchange.py'
--- landscape/broker/exchange.py	2017-03-21 13:00:17 +0000
+++ landscape/broker/exchange.py	2017-03-27 06:59:04 +0000
@@ -715,7 +715,7 @@
         return payload
 
     def _hash_types(self, types):
-        accepted_types_str = ";".join(types)
+        accepted_types_str = ";".join(types).encode("ascii")
         return md5(accepted_types_str).digest()
 
     def _handle_result(self, payload, result):

=== modified file 'landscape/broker/tests/test_exchange.py'
--- landscape/broker/tests/test_exchange.py	2016-06-15 17:33:13 +0000
+++ landscape/broker/tests/test_exchange.py	2017-03-27 06:59:04 +0000
@@ -104,7 +104,7 @@
         messages = self.transport.payloads[0]["messages"]
         self.assertEqual(messages, [{"type": "empty",
                                      "timestamp": 0,
-                                     "api": "3.2"}])
+                                     "api": b"3.2"}])
 
     def test_send_urgent(self):
         """
@@ -149,7 +149,7 @@
         """
         payload = self.exchanger._make_payload()
         self.assertIn("accepted-types", payload)
-        self.assertEqual(payload["accepted-types"], md5("").digest())
+        self.assertEqual(payload["accepted-types"], md5(b"").digest())
 
     def test_handle_message_sets_accepted_types(self):
         """
@@ -170,8 +170,8 @@
             {"type": "accepted-types", "types": ["a", "b"]})
         self.exchanger.handle_message(
             {"type": "accepted-types", "types": ["b", "c"]})
-        self.assertEqual(stash, [("a", True), ("b", True),
-                                 ("a", False), ("c", True)])
+        self.assertCountEqual(stash, [("a", True), ("b", True),
+                                      ("a", False), ("c", True)])
 
     def test_wb_accepted_types_roundtrip(self):
         """
@@ -183,7 +183,7 @@
         payload = self.exchanger._make_payload()
         self.assertIn("accepted-types", payload)
         self.assertEqual(payload["accepted-types"],
-                         md5("ack;bar").digest())
+                         md5(b"ack;bar").digest())
 
     def test_accepted_types_causes_urgent_if_held_messages_exist(self):
         """
@@ -523,33 +523,33 @@
         payload = self.transport.payloads[-1]
         self.assertMessages(payload["messages"], [])
         self.assertEqual(payload.get("client-api"), CLIENT_API)
-        self.assertEqual(payload.get("server-api"), "3.2")
-        self.assertEqual(self.transport.message_api, "3.2")
-
-        self.mstore.add({"type": "a", "api": "1.0"})
-        self.mstore.add({"type": "b", "api": "1.0"})
-        self.mstore.add({"type": "c", "api": "1.1"})
-        self.mstore.add({"type": "d", "api": "1.1"})
-
-        self.exchanger.exchange()
-
-        payload = self.transport.payloads[-1]
-        self.assertMessages(payload["messages"],
-                            [{"type": "a", "api": "1.0"},
-                             {"type": "b", "api": "1.0"}])
-        self.assertEqual(payload.get("client-api"), CLIENT_API)
-        self.assertEqual(payload.get("server-api"), "1.0")
-        self.assertEqual(self.transport.message_api, "1.0")
-
-        self.exchanger.exchange()
-
-        payload = self.transport.payloads[-1]
-        self.assertMessages(payload["messages"],
-                            [{"type": "c", "api": "1.1"},
-                             {"type": "d", "api": "1.1"}])
-        self.assertEqual(payload.get("client-api"), CLIENT_API)
-        self.assertEqual(payload.get("server-api"), "1.1")
-        self.assertEqual(self.transport.message_api, "1.1")
+        self.assertEqual(payload.get("server-api"), b"3.2")
+        self.assertEqual(self.transport.message_api, b"3.2")
+
+        self.mstore.add({"type": "a", "api": b"1.0"})
+        self.mstore.add({"type": "b", "api": b"1.0"})
+        self.mstore.add({"type": "c", "api": b"1.1"})
+        self.mstore.add({"type": "d", "api": b"1.1"})
+
+        self.exchanger.exchange()
+
+        payload = self.transport.payloads[-1]
+        self.assertMessages(payload["messages"],
+                            [{"type": "a", "api": b"1.0"},
+                             {"type": "b", "api": b"1.0"}])
+        self.assertEqual(payload.get("client-api"), CLIENT_API)
+        self.assertEqual(payload.get("server-api"), b"1.0")
+        self.assertEqual(self.transport.message_api, b"1.0")
+
+        self.exchanger.exchange()
+
+        payload = self.transport.payloads[-1]
+        self.assertMessages(payload["messages"],
+                            [{"type": "c", "api": b"1.1"},
+                             {"type": "d", "api": b"1.1"}])
+        self.assertEqual(payload.get("client-api"), CLIENT_API)
+        self.assertEqual(payload.get("server-api"), b"1.1")
+        self.assertEqual(self.transport.message_api, b"1.1")
 
     def test_exchange_token(self):
         """
@@ -827,27 +827,27 @@
         """
         self.transport.extra.pop("server-api", None)
         self.exchanger.exchange()
-        self.assertEqual("3.2", self.mstore.get_server_api())
+        self.assertEqual(b"3.2", self.mstore.get_server_api())
 
     def test_wb_client_with_older_api_and_server_with_newer(self):
         """
         If a server notifies us that it case use a very new API, but we
         don't know how to speak it, we keep using ours.
         """
-        self.exchanger._api = "3.3"
-        self.transport.extra["server-api"] = "3.4"
+        self.exchanger._api = b"3.3"
+        self.transport.extra["server-api"] = b"3.4"
         self.exchanger.exchange()
-        self.assertEqual("3.3", self.mstore.get_server_api())
+        self.assertEqual(b"3.3", self.mstore.get_server_api())
 
     def test_wb_client_with_newer_api_and_server_with_older(self):
         """
         If a server notifies us that it can use an API which is older
         than the one we support, we'll just use the server API.
         """
-        self.exchanger._api = "3.4"
-        self.transport.extra["server-api"] = "3.3"
+        self.exchanger._api = b"3.4"
+        self.transport.extra["server-api"] = b"3.3"
         self.exchanger.exchange()
-        self.assertEqual("3.3", self.mstore.get_server_api())
+        self.assertEqual(b"3.3", self.mstore.get_server_api())
 
     def test_server_uuid_is_stored_on_message_store(self):
         self.transport.extra["server-uuid"] = "first-uuid"
@@ -1078,10 +1078,10 @@
         """
         If we get a 404, we try to donwgrade our server API version.
         """
-        self.mstore.set_server_api("3.3")
+        self.mstore.set_server_api(b"3.3")
         self.transport.responses.append(HTTPCodeError(404, ""))
         self.exchanger.exchange()
-        self.assertEqual("3.2", self.mstore.get_server_api())
+        self.assertEqual(b"3.2", self.mstore.get_server_api())
 
 
 class AcceptedTypesMessageExchangeTest(LandscapeTest):
@@ -1120,7 +1120,7 @@
         self.exchanger.register_client_accepted_message_type("type-A")
         self.exchanger.register_client_accepted_message_type("type-B")
         types = sorted(["type-A", "type-B"] + DEFAULT_ACCEPTED_TYPES)
-        accepted_types_digest = md5(";".join(types)).digest()
+        accepted_types_digest = md5(";".join(types).encode("ascii")).digest()
         self.transport.extra["client-accepted-types-hash"] = \
             accepted_types_digest
         self.exchanger.exchange()
@@ -1146,7 +1146,7 @@
         client will send a new list to the server.
         """
         self.exchanger.register_client_accepted_message_type("type-A")
-        types_hash = md5("type-A").digest()
+        types_hash = md5(b"type-A").digest()
         self.transport.extra["client-accepted-types-hash"] = types_hash
         self.exchanger.exchange()
         self.exchanger.register_client_accepted_message_type("type-B")
@@ -1162,7 +1162,7 @@
         send a new list of types.
         """
         self.exchanger.register_client_accepted_message_type("type-A")
-        types_hash = md5("type-A").digest()
+        types_hash = md5(b"type-A").digest()
         self.transport.extra["client-accepted-types-hash"] = types_hash
         self.exchanger.exchange()
         self.transport.extra["client-accepted-types-hash"] = "lol"

=== modified file 'landscape/tests/helpers.py'
--- landscape/tests/helpers.py	2017-03-21 17:57:12 +0000
+++ landscape/tests/helpers.py	2017-03-27 06:59:04 +0000
@@ -148,6 +148,9 @@
         else:
             return result[0]
 
+    if not _PY3:
+        assertCountEqual = TestCase.assertItemsEqual
+
     def assertNoResult(self, deferred):
         """See C{twisted.trial._synctest._Assertions.assertNoResult}.
 

=== modified file 'py3_ready_tests'
--- py3_ready_tests	2017-03-23 14:26:11 +0000
+++ py3_ready_tests	2017-03-27 06:59:04 +0000
@@ -9,6 +9,5 @@
 
 
 
-
-
+landscape.broker.tests.test_exchange
 landscape.broker.tests.test_store