From 1a9635b4bea8d36402e3c561fc9129f004ab02b9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Sun, 5 Feb 2023 22:29:01 +0000 Subject: [PATCH 1/6] Make tests.federation pass mypy --- mypy.ini | 2 -- tests/federation/test_federation_catch_up.py | 5 ++--- tests/federation/test_federation_sender.py | 10 ++++++---- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/mypy.ini b/mypy.ini index a6e37bc3775a..751a3e209ff5 100644 --- a/mypy.ini +++ b/mypy.ini @@ -33,8 +33,6 @@ exclude = (?x) |synapse/storage/schema/ |tests/appservice/test_scheduler.py - |tests/federation/test_federation_catch_up.py - |tests/federation/test_federation_sender.py |tests/http/federation/test_matrix_federation_agent.py |tests/http/federation/test_srv_resolver.py |tests/http/test_proxyagent.py diff --git a/tests/federation/test_federation_catch_up.py b/tests/federation/test_federation_catch_up.py index b8fee7289838..37dda89380c2 100644 --- a/tests/federation/test_federation_catch_up.py +++ b/tests/federation/test_federation_catch_up.py @@ -261,12 +261,11 @@ async def fake_send( destination_tm: str, pending_pdus: List[EventBase], _pending_edus: List[Edu], - ) -> bool: + ) -> None: assert destination == destination_tm results_list.extend(pending_pdus) - return True # success! - transaction_manager.send_new_transaction = fake_send + transaction_manager.send_new_transaction = fake_send # type: ignore[assignment] return per_dest_queue, results_list diff --git a/tests/federation/test_federation_sender.py b/tests/federation/test_federation_sender.py index 8692d8190f66..12dd550e9fa4 100644 --- a/tests/federation/test_federation_sender.py +++ b/tests/federation/test_federation_sender.py @@ -41,11 +41,11 @@ def make_homeserver(self, reactor, clock): federation_transport_client=Mock(spec=["send_transaction"]), ) - hs.get_storage_controllers().state.get_current_hosts_in_room = Mock( + hs.get_storage_controllers().state.get_current_hosts_in_room = Mock( # type: ignore[assignment] return_value=make_awaitable({"test", "host2"}) ) - hs.get_storage_controllers().state.get_current_hosts_in_room_or_partial_state_approximation = ( + hs.get_storage_controllers().state.get_current_hosts_in_room_or_partial_state_approximation = ( # type: ignore[assignment] hs.get_storage_controllers().state.get_current_hosts_in_room ) @@ -473,7 +473,9 @@ def test_upload_signatures(self): self.assertEqual(edu["edu_type"], EduTypes.DEVICE_LIST_UPDATE) c = edu["content"] if stream_id is not None: - self.assertEqual(c["prev_id"], [stream_id]) + # type-ignore: mypy marks the next line as unreachable; it does not seem + # to recognise the assignment to stream_id within the body of this loop + self.assertEqual(c["prev_id"], [stream_id]) # type: ignore[unreachable] self.assertGreaterEqual(c["stream_id"], stream_id) stream_id = c["stream_id"] devices = {edu["content"]["device_id"] for edu in self.edus} @@ -555,7 +557,7 @@ def test_unreachable_server(self): # for each device, there should be a single update self.assertEqual(len(self.edus), 3) - stream_id = None + stream_id: Optional[int] = None for edu in self.edus: self.assertEqual(edu["edu_type"], EduTypes.DEVICE_LIST_UPDATE) c = edu["content"] From 374a517874a439998c1ad049b179f2da491c17a8 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Sun, 5 Feb 2023 22:35:18 +0000 Subject: [PATCH 2/6] Untyped defs in tests.federation.transport --- mypy.ini | 2 +- .../federation/transport/server/test__base.py | 4 ++- tests/federation/transport/test_knocking.py | 32 +++++++++++++------ tests/federation/transport/test_server.py | 6 ++-- 4 files changed, 29 insertions(+), 15 deletions(-) diff --git a/mypy.ini b/mypy.ini index 751a3e209ff5..da468a431b9a 100644 --- a/mypy.ini +++ b/mypy.ini @@ -85,7 +85,7 @@ disallow_untyped_defs = True [mypy-tests.events.*] disallow_untyped_defs = True -[mypy-tests.federation.transport.test_client] +[mypy-tests.federation.transport.*] disallow_untyped_defs = True [mypy-tests.handlers.*] diff --git a/tests/federation/transport/server/test__base.py b/tests/federation/transport/server/test__base.py index e88e5d8bb355..55655de8628e 100644 --- a/tests/federation/transport/server/test__base.py +++ b/tests/federation/transport/server/test__base.py @@ -15,6 +15,8 @@ from http import HTTPStatus from typing import Dict, List, Tuple +from twisted.web.resource import Resource + from synapse.api.errors import Codes from synapse.federation.transport.server import BaseFederationServlet from synapse.federation.transport.server._base import Authenticator, _parse_auth_header @@ -62,7 +64,7 @@ class BaseFederationServletCancellationTests(unittest.FederatingHomeserverTestCa path = f"{CancellableFederationServlet.PREFIX}{CancellableFederationServlet.PATH}" - def create_test_resource(self): + def create_test_resource(self) -> Resource: """Overrides `HomeserverTestCase.create_test_resource`.""" resource = JsonResource(self.hs) diff --git a/tests/federation/transport/test_knocking.py b/tests/federation/transport/test_knocking.py index ff589c0b6ca0..70209ab09011 100644 --- a/tests/federation/transport/test_knocking.py +++ b/tests/federation/transport/test_knocking.py @@ -12,15 +12,19 @@ # See the License for the specific language governing permissions and # limitations under the License. from collections import OrderedDict -from typing import Dict, List +from typing import Any, Dict, List, Optional + +from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EventTypes, JoinRules, Membership -from synapse.api.room_versions import RoomVersions -from synapse.events import builder +from synapse.api.room_versions import RoomVersion, RoomVersions +from synapse.events import EventBase, builder +from synapse.events.snapshot import EventContext from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer from synapse.types import RoomAlias +from synapse.util import Clock from tests.test_utils import event_injection from tests.unittest import FederatingHomeserverTestCase, HomeserverTestCase @@ -197,7 +201,9 @@ class FederationKnockingTestCase( login.register_servlets, ] - def prepare(self, reactor, clock, homeserver): + def prepare( + self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer + ) -> None: self.store = homeserver.get_datastores().main # We're not going to be properly signing events as our remote homeserver is fake, @@ -205,23 +211,29 @@ def prepare(self, reactor, clock, homeserver): # Note that these checks are not relevant to this test case. # Have this homeserver auto-approve all event signature checking. - async def approve_all_signature_checking(_, pdu): + async def approve_all_signature_checking( + room_version: RoomVersion, + pdu: EventBase, + record_failure_callback: Any = None, + ) -> EventBase: return pdu - homeserver.get_federation_server()._check_sigs_and_hash = ( + homeserver.get_federation_server()._check_sigs_and_hash = ( # type: ignore[assignment] approve_all_signature_checking ) # Have this homeserver skip event auth checks. This is necessary due to # event auth checks ensuring that events were signed by the sender's homeserver. - async def _check_event_auth(origin, event, context, *args, **kwargs): - return context + async def _check_event_auth( + origin: Optional[str], event: EventBase, context: EventContext + ) -> None: + pass - homeserver.get_federation_event_handler()._check_event_auth = _check_event_auth + homeserver.get_federation_event_handler()._check_event_auth = _check_event_auth # type: ignore[assignment] return super().prepare(reactor, clock, homeserver) - def test_room_state_returned_when_knocking(self): + def test_room_state_returned_when_knocking(self) -> None: """ Tests that specific, stripped state events from a room are returned after a remote homeserver successfully knocks on a local room. diff --git a/tests/federation/transport/test_server.py b/tests/federation/transport/test_server.py index cfd550a04bcc..c4231f4aa972 100644 --- a/tests/federation/transport/test_server.py +++ b/tests/federation/transport/test_server.py @@ -20,7 +20,7 @@ class RoomDirectoryFederationTests(unittest.FederatingHomeserverTestCase): @override_config({"allow_public_rooms_over_federation": False}) - def test_blocked_public_room_list_over_federation(self): + def test_blocked_public_room_list_over_federation(self) -> None: """Test that unauthenticated requests to the public rooms directory 403 when allow_public_rooms_over_federation is False. """ @@ -31,7 +31,7 @@ def test_blocked_public_room_list_over_federation(self): self.assertEqual(403, channel.code) @override_config({"allow_public_rooms_over_federation": True}) - def test_open_public_room_list_over_federation(self): + def test_open_public_room_list_over_federation(self) -> None: """Test that unauthenticated requests to the public rooms directory 200 when allow_public_rooms_over_federation is True. """ @@ -42,7 +42,7 @@ def test_open_public_room_list_over_federation(self): self.assertEqual(200, channel.code) @DEBUG - def test_edu_debugging_doesnt_explode(self): + def test_edu_debugging_doesnt_explode(self) -> None: """Sanity check incoming federation succeeds with `synapse.debug_8631` enabled. Remove this when we strip out issue_8631_logger. From fd05df33ac0bb5844ce4034063039e259430ba95 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Sun, 5 Feb 2023 22:41:46 +0000 Subject: [PATCH 3/6] test methods return None --- tests/federation/test_complexity.py | 12 ++++++------ tests/federation/test_federation_catch_up.py | 12 ++++++------ tests/federation/test_federation_client.py | 8 ++++---- tests/federation/test_federation_sender.py | 20 ++++++++++---------- tests/federation/test_federation_server.py | 12 ++++++------ 5 files changed, 32 insertions(+), 32 deletions(-) diff --git a/tests/federation/test_complexity.py b/tests/federation/test_complexity.py index 9f1115dd23b8..ecb9c04f86db 100644 --- a/tests/federation/test_complexity.py +++ b/tests/federation/test_complexity.py @@ -36,7 +36,7 @@ def default_config(self): config["limit_remote_rooms"] = {"enabled": True, "complexity": 0.05} return config - def test_complexity_simple(self): + def test_complexity_simple(self) -> None: u1 = self.register_user("u1", "pass") u1_token = self.login("u1", "pass") @@ -66,7 +66,7 @@ def test_complexity_simple(self): complexity = channel.json_body["v1"] self.assertEqual(complexity, 1.23) - def test_join_too_large(self): + def test_join_too_large(self) -> None: u1 = self.register_user("u1", "pass") @@ -95,7 +95,7 @@ def test_join_too_large(self): self.assertEqual(f.value.code, 400, f.value) self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) - def test_join_too_large_admin(self): + def test_join_too_large_admin(self) -> None: # Check whether an admin can join if option "admins_can_join" is undefined, # this option defaults to false, so the join should fail. @@ -126,7 +126,7 @@ def test_join_too_large_admin(self): self.assertEqual(f.value.code, 400, f.value) self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) - def test_join_too_large_once_joined(self): + def test_join_too_large_once_joined(self) -> None: u1 = self.register_user("u1", "pass") u1_token = self.login("u1", "pass") @@ -189,7 +189,7 @@ def default_config(self): } return config - def test_join_too_large_no_admin(self): + def test_join_too_large_no_admin(self) -> None: # A user which is not an admin should not be able to join a remote room # which is too complex. @@ -220,7 +220,7 @@ def test_join_too_large_no_admin(self): self.assertEqual(f.value.code, 400, f.value) self.assertEqual(f.value.errcode, Codes.RESOURCE_LIMIT_EXCEEDED) - def test_join_too_large_admin(self): + def test_join_too_large_admin(self) -> None: # An admin should be able to join rooms where a complexity check fails. u1 = self.register_user("u1", "pass", admin=True) diff --git a/tests/federation/test_federation_catch_up.py b/tests/federation/test_federation_catch_up.py index 37dda89380c2..3dd936e0ac9f 100644 --- a/tests/federation/test_federation_catch_up.py +++ b/tests/federation/test_federation_catch_up.py @@ -92,7 +92,7 @@ def get_destination_room(self, room: str, destination: str = "host2") -> dict: )[0] return {"event_id": event_id, "stream_ordering": stream_ordering} - def test_catch_up_destination_rooms_tracking(self): + def test_catch_up_destination_rooms_tracking(self) -> None: """ Tests that we populate the `destination_rooms` table as needed. """ @@ -117,7 +117,7 @@ def test_catch_up_destination_rooms_tracking(self): self.assertEqual(row_2["event_id"], event_id_2) self.assertEqual(row_1["stream_ordering"], row_2["stream_ordering"] - 1) - def test_catch_up_last_successful_stream_ordering_tracking(self): + def test_catch_up_last_successful_stream_ordering_tracking(self) -> None: """ Tests that we populate the `destination_rooms` table as needed. """ @@ -174,7 +174,7 @@ def test_catch_up_last_successful_stream_ordering_tracking(self): "Send succeeded but not marked as last_successful_stream_ordering", ) - def test_catch_up_from_blank_state(self): + def test_catch_up_from_blank_state(self) -> None: """ Runs an overall test of federation catch-up from scratch. Further tests will focus on more narrow aspects and edge-cases, but I @@ -269,7 +269,7 @@ async def fake_send( return per_dest_queue, results_list - def test_catch_up_loop(self): + def test_catch_up_loop(self) -> None: """ Tests the behaviour of _catch_up_transmission_loop. """ @@ -333,7 +333,7 @@ def test_catch_up_loop(self): event_5.internal_metadata.stream_ordering, ) - def test_catch_up_on_synapse_startup(self): + def test_catch_up_on_synapse_startup(self) -> None: """ Tests the behaviour of get_catch_up_outstanding_destinations and _wake_destinations_needing_catchup. @@ -431,7 +431,7 @@ def wake_destination_track(destination): # - all destinations are woken exactly once; they appear once in woken. self.assertCountEqual(woken, server_names[:-1]) - def test_not_latest_event(self): + def test_not_latest_event(self) -> None: """Test that we send the latest event in the room even if its not ours.""" per_dest_queue, sent_pdus = self.make_fake_destination_queue() diff --git a/tests/federation/test_federation_client.py b/tests/federation/test_federation_client.py index e67f4058260f..0e9900ea23d5 100644 --- a/tests/federation/test_federation_client.py +++ b/tests/federation/test_federation_client.py @@ -51,7 +51,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer): self.creator = f"@creator:{self.OTHER_SERVER_NAME}" self.test_room_id = "!room_id" - def test_get_room_state(self): + def test_get_room_state(self) -> None: # mock up some events to use in the response. # In real life, these would have things in `prev_events` and `auth_events`, but that's # a bit annoying to mock up, and the code under test doesn't care, so we don't bother. @@ -140,7 +140,7 @@ def test_get_room_state(self): ["m.room.create", "m.room.member", "m.room.power_levels"], ) - def test_get_pdu_returns_nothing_when_event_does_not_exist(self): + def test_get_pdu_returns_nothing_when_event_does_not_exist(self) -> None: """No event should be returned when the event does not exist""" pulled_pdu_info = self.get_success( self.hs.get_federation_client().get_pdu( @@ -151,11 +151,11 @@ def test_get_pdu_returns_nothing_when_event_does_not_exist(self): ) self.assertEqual(pulled_pdu_info, None) - def test_get_pdu(self): + def test_get_pdu(self) -> None: """Test to make sure an event is returned by `get_pdu()`""" self._get_pdu_once() - def test_get_pdu_event_from_cache_is_pristine(self): + def test_get_pdu_event_from_cache_is_pristine(self) -> None: """Test that modifications made to events returned by `get_pdu()` do not propagate back to to the internal cache (events returned should be a copy). diff --git a/tests/federation/test_federation_sender.py b/tests/federation/test_federation_sender.py index 12dd550e9fa4..939359bd029f 100644 --- a/tests/federation/test_federation_sender.py +++ b/tests/federation/test_federation_sender.py @@ -56,7 +56,7 @@ def default_config(self) -> JsonDict: config["federation_sender_instances"] = None return config - def test_send_receipts(self): + def test_send_receipts(self) -> None: mock_send_transaction = ( self.hs.get_federation_transport_client().send_transaction ) @@ -98,7 +98,7 @@ def test_send_receipts(self): ], ) - def test_send_receipts_thread(self): + def test_send_receipts_thread(self) -> None: mock_send_transaction = ( self.hs.get_federation_transport_client().send_transaction ) @@ -174,7 +174,7 @@ def test_send_receipts_thread(self): ], ) - def test_send_receipts_with_backoff(self): + def test_send_receipts_with_backoff(self) -> None: """Send two receipts in quick succession; the second should be flushed, but only after 20ms""" mock_send_transaction = ( @@ -316,7 +316,7 @@ def record_transaction(self, txn, json_cb): self.edus.extend(data["edus"]) return defer.succeed({}) - def test_send_device_updates(self): + def test_send_device_updates(self) -> None: """Basic case: each device update should result in an EDU""" # create a device u1 = self.register_user("user", "pass") @@ -340,7 +340,7 @@ def test_send_device_updates(self): self.assertEqual(len(self.edus), 1) self.check_device_update_edu(self.edus.pop(0), u1, "D2", stream_id) - def test_dont_send_device_updates_for_remote_users(self): + def test_dont_send_device_updates_for_remote_users(self) -> None: """Check that we don't send device updates for remote users""" # Send the server a device list EDU for the other user, this will cause @@ -379,7 +379,7 @@ def test_dont_send_device_updates_for_remote_users(self): ) self.assertIn("D1", devices) - def test_upload_signatures(self): + def test_upload_signatures(self) -> None: """Uploading signatures on some devices should produce updates for that user""" e2e_handler = self.hs.get_e2e_keys_handler() @@ -481,7 +481,7 @@ def test_upload_signatures(self): devices = {edu["content"]["device_id"] for edu in self.edus} self.assertEqual({"D1", "D2"}, devices) - def test_delete_devices(self): + def test_delete_devices(self) -> None: """If devices are deleted, that should result in EDUs too""" # create devices @@ -523,7 +523,7 @@ def test_delete_devices(self): devices = {edu["content"]["device_id"] for edu in self.edus} self.assertEqual({"D1", "D2", "D3"}, devices) - def test_unreachable_server(self): + def test_unreachable_server(self) -> None: """If the destination server is unreachable, all the updates should get sent on recovery """ @@ -568,7 +568,7 @@ def test_unreachable_server(self): devices = {edu["content"]["device_id"] for edu in self.edus} self.assertEqual({"D1", "D2", "D3"}, devices) - def test_prune_outbound_device_pokes1(self): + def test_prune_outbound_device_pokes1(self) -> None: """If a destination is unreachable, and the updates are pruned, we should get a single update. @@ -617,7 +617,7 @@ def test_prune_outbound_device_pokes1(self): # synapse uses an empty prev_id list to indicate "needs a full resync". self.assertEqual(c["prev_id"], []) - def test_prune_outbound_device_pokes2(self): + def test_prune_outbound_device_pokes2(self) -> None: """If a destination is unreachable, and the updates are pruned, we should get a single update. diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index be719e49c0ca..5455468069a4 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -42,7 +42,7 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase): ] @parameterized.expand([(b"",), (b"foo",), (b'{"limit": Infinity}',)]) - def test_bad_request(self, query_content): + def test_bad_request(self, query_content) -> None: """ Querying with bad data returns a reasonable error code. """ @@ -64,7 +64,7 @@ def test_bad_request(self, query_content): class ServerACLsTestCase(unittest.TestCase): - def test_blacklisted_server(self): + def test_blacklisted_server(self) -> None: e = _create_acl_event({"allow": ["*"], "deny": ["evil.com"]}) logging.info("ACL event: %s", e.content) @@ -74,7 +74,7 @@ def test_blacklisted_server(self): self.assertTrue(server_matches_acl_event("evil.com.au", e)) self.assertTrue(server_matches_acl_event("honestly.not.evil.com", e)) - def test_block_ip_literals(self): + def test_block_ip_literals(self) -> None: e = _create_acl_event({"allow_ip_literals": False, "allow": ["*"]}) logging.info("ACL event: %s", e.content) @@ -83,7 +83,7 @@ def test_block_ip_literals(self): self.assertFalse(server_matches_acl_event("[1:2::]", e)) self.assertTrue(server_matches_acl_event("1:2:3:4", e)) - def test_wildcard_matching(self): + def test_wildcard_matching(self) -> None: e = _create_acl_event({"allow": ["good*.com"]}) self.assertTrue( server_matches_acl_event("good.com", e), @@ -110,7 +110,7 @@ class StateQueryTests(unittest.FederatingHomeserverTestCase): login.register_servlets, ] - def test_needs_to_be_in_room(self): + def test_needs_to_be_in_room(self) -> None: """/v1/state/ requires the server to be in the room""" u1 = self.register_user("u1", "pass") u1_token = self.login("u1", "pass") @@ -157,7 +157,7 @@ def _make_join(self, user_id: str) -> JsonDict: self.assertEqual(channel.code, HTTPStatus.OK, channel.json_body) return channel.json_body - def test_send_join(self): + def test_send_join(self) -> None: """happy-path test of send_join""" joining_user = "@misspiggy:" + self.OTHER_SERVER_NAME join_result = self._make_join(joining_user) From a89bcc191f131ec6451bec6959bf6b5a193a7a9d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Sun, 5 Feb 2023 23:02:21 +0000 Subject: [PATCH 4/6] Remaining type hints in tests.federation --- mypy.ini | 2 +- tests/federation/test_complexity.py | 6 +-- tests/federation/test_federation_catch_up.py | 35 ++++++++++------ tests/federation/test_federation_client.py | 4 +- tests/federation/test_federation_sender.py | 44 ++++++++++++-------- tests/federation/test_federation_server.py | 8 ++-- 6 files changed, 60 insertions(+), 39 deletions(-) diff --git a/mypy.ini b/mypy.ini index da468a431b9a..544bf6cc5626 100644 --- a/mypy.ini +++ b/mypy.ini @@ -85,7 +85,7 @@ disallow_untyped_defs = True [mypy-tests.events.*] disallow_untyped_defs = True -[mypy-tests.federation.transport.*] +[mypy-tests.federation.*] disallow_untyped_defs = True [mypy-tests.handlers.*] diff --git a/tests/federation/test_complexity.py b/tests/federation/test_complexity.py index ecb9c04f86db..d667dd27bf40 100644 --- a/tests/federation/test_complexity.py +++ b/tests/federation/test_complexity.py @@ -17,7 +17,7 @@ from synapse.api.errors import Codes, SynapseError from synapse.rest import admin from synapse.rest.client import login, room -from synapse.types import UserID +from synapse.types import JsonDict, UserID from tests import unittest from tests.test_utils import make_awaitable @@ -31,7 +31,7 @@ class RoomComplexityTests(unittest.FederatingHomeserverTestCase): login.register_servlets, ] - def default_config(self): + def default_config(self) -> JsonDict: config = super().default_config() config["limit_remote_rooms"] = {"enabled": True, "complexity": 0.05} return config @@ -180,7 +180,7 @@ class RoomComplexityAdminTests(unittest.FederatingHomeserverTestCase): login.register_servlets, ] - def default_config(self): + def default_config(self) -> JsonDict: config = super().default_config() config["limit_remote_rooms"] = { "enabled": True, diff --git a/tests/federation/test_federation_catch_up.py b/tests/federation/test_federation_catch_up.py index 3dd936e0ac9f..a986b15f0a87 100644 --- a/tests/federation/test_federation_catch_up.py +++ b/tests/federation/test_federation_catch_up.py @@ -1,13 +1,17 @@ -from typing import List, Tuple +from typing import Callable, List, Optional, Tuple from unittest.mock import Mock +from twisted.test.proto_helpers import MemoryReactor + from synapse.api.constants import EventTypes from synapse.events import EventBase from synapse.federation.sender import PerDestinationQueue, TransactionManager -from synapse.federation.units import Edu +from synapse.federation.units import Edu, Transaction from synapse.rest import admin from synapse.rest.client import login, room +from synapse.server import HomeServer from synapse.types import JsonDict +from synapse.util import Clock from synapse.util.retryutils import NotRetryingDestination from tests.test_utils import event_injection, make_awaitable @@ -28,23 +32,25 @@ class FederationCatchUpTestCases(FederatingHomeserverTestCase): login.register_servlets, ] - def make_homeserver(self, reactor, clock): + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: return self.setup_test_homeserver( federation_transport_client=Mock(spec=["send_transaction"]), ) - def prepare(self, reactor, clock, hs): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: # stub out get_current_hosts_in_room - state_handler = hs.get_state_handler() + state_storage_controller = hs.get_storage_controllers().state # This mock is crucial for destination_rooms to be populated. - state_handler.get_current_hosts_in_room = Mock( - return_value=make_awaitable(["test", "host2"]) + # TODO: this seems to no longer be the case---tests pass with this mock + # commented out. + state_storage_controller.get_current_hosts_in_room = Mock( # type: ignore[assignment] + return_value=make_awaitable({"test", "host2"}) ) # whenever send_transaction is called, record the pdu data - self.pdus = [] - self.failed_pdus = [] + self.pdus: List[JsonDict] = [] + self.failed_pdus: List[JsonDict] = [] self.is_online = True self.hs.get_federation_transport_client().send_transaction.side_effect = ( self.record_transaction @@ -55,8 +61,13 @@ def default_config(self) -> JsonDict: config["federation_sender_instances"] = None return config - async def record_transaction(self, txn, json_cb): - if self.is_online: + async def record_transaction( + self, txn: Transaction, json_cb: Optional[Callable[[], JsonDict]] + ) -> JsonDict: + if json_cb is None: + # The tests seem to expect that this method raises in this situation. + raise Exception("Blank json_cb") + elif self.is_online: data = json_cb() self.pdus.extend(data["pdus"]) return {} @@ -411,7 +422,7 @@ def test_catch_up_on_synapse_startup(self) -> None: # patch wake_destination to just count the destinations instead woken = [] - def wake_destination_track(destination): + def wake_destination_track(destination: str) -> None: woken.append(destination) self.hs.get_federation_sender().wake_destination = wake_destination_track diff --git a/tests/federation/test_federation_client.py b/tests/federation/test_federation_client.py index 0e9900ea23d5..86e1236501fd 100644 --- a/tests/federation/test_federation_client.py +++ b/tests/federation/test_federation_client.py @@ -36,7 +36,9 @@ class FederationClientTest(FederatingHomeserverTestCase): login.register_servlets, ] - def prepare(self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer): + def prepare( + self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer + ) -> None: super().prepare(reactor, clock, homeserver) # mock out the Agent used by the federation client, which is easier than diff --git a/tests/federation/test_federation_sender.py b/tests/federation/test_federation_sender.py index 939359bd029f..32687cf72858 100644 --- a/tests/federation/test_federation_sender.py +++ b/tests/federation/test_federation_sender.py @@ -11,18 +11,22 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -from typing import Optional +from typing import Callable, FrozenSet, List, Optional, Set from unittest.mock import Mock from signedjson import key, sign from signedjson.types import BaseKey, SigningKey from twisted.internet import defer +from twisted.test.proto_helpers import MemoryReactor from synapse.api.constants import EduTypes, RoomEncryptionAlgorithms +from synapse.federation.units import Transaction from synapse.rest import admin from synapse.rest.client import login +from synapse.server import HomeServer from synapse.types import JsonDict, ReadReceipt +from synapse.util import Clock from tests.test_utils import make_awaitable from tests.unittest import HomeserverTestCase @@ -36,7 +40,7 @@ class FederationSenderReceiptsTestCases(HomeserverTestCase): re-enabled for the main process. """ - def make_homeserver(self, reactor, clock): + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: hs = self.setup_test_homeserver( federation_transport_client=Mock(spec=["send_transaction"]), ) @@ -272,46 +276,50 @@ class FederationSenderDevicesTestCases(HomeserverTestCase): login.register_servlets, ] - def make_homeserver(self, reactor, clock): + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: return self.setup_test_homeserver( federation_transport_client=Mock( spec=["send_transaction", "query_user_devices"] ), ) - def default_config(self): + def default_config(self) -> JsonDict: c = super().default_config() # Enable federation sending on the main process. c["federation_sender_instances"] = None return c - def prepare(self, reactor, clock, hs): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: test_room_id = "!room:host1" # stub out `get_rooms_for_user` and `get_current_hosts_in_room` so that the # server thinks the user shares a room with `@user2:host2` - def get_rooms_for_user(user_id): - return defer.succeed({test_room_id}) + def get_rooms_for_user(user_id: str) -> "defer.Deferred[FrozenSet[str]]": + return defer.succeed(frozenset({test_room_id})) - hs.get_datastores().main.get_rooms_for_user = get_rooms_for_user + hs.get_datastores().main.get_rooms_for_user = get_rooms_for_user # type: ignore[assignment] - async def get_current_hosts_in_room(room_id): + async def get_current_hosts_in_room(room_id: str) -> Set[str]: if room_id == test_room_id: - return ["host2"] - - # TODO: We should fail the test when we encounter an unxpected room ID. - # We can't just use `self.fail(...)` here because the app code is greedy - # with `Exception` and will catch it before the test can see it. + return {"host2"} + else: + # TODO: We should fail the test when we encounter an unxpected room ID. + # We can't just use `self.fail(...)` here because the app code is greedy + # with `Exception` and will catch it before the test can see it. + return set() - hs.get_datastores().main.get_current_hosts_in_room = get_current_hosts_in_room + hs.get_datastores().main.get_current_hosts_in_room = get_current_hosts_in_room # type: ignore[assignment] # whenever send_transaction is called, record the edu data - self.edus = [] + self.edus: List[JsonDict] = [] self.hs.get_federation_transport_client().send_transaction.side_effect = ( self.record_transaction ) - def record_transaction(self, txn, json_cb): + def record_transaction( + self, txn: Transaction, json_cb: Optional[Callable[[], JsonDict]] = None + ) -> "defer.Deferred[JsonDict]": + assert json_cb is not None data = json_cb() self.edus.extend(data["edus"]) return defer.succeed({}) @@ -743,7 +751,7 @@ def encode_pubkey(sk: SigningKey) -> str: return key.encode_verify_key_base64(key.get_verify_key(sk)) -def build_device_dict(user_id: str, device_id: str, sk: SigningKey): +def build_device_dict(user_id: str, device_id: str, sk: SigningKey) -> JsonDict: """Build a dict representing the given device""" return { "user_id": user_id, diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index 5455468069a4..bba6469b559b 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -21,7 +21,7 @@ from synapse.api.room_versions import KNOWN_ROOM_VERSIONS from synapse.config.server import DEFAULT_ROOM_VERSION -from synapse.events import make_event_from_dict +from synapse.events import EventBase, make_event_from_dict from synapse.federation.federation_server import server_matches_acl_event from synapse.rest import admin from synapse.rest.client import login, room @@ -42,7 +42,7 @@ class FederationServerTests(unittest.FederatingHomeserverTestCase): ] @parameterized.expand([(b"",), (b"foo",), (b'{"limit": Infinity}',)]) - def test_bad_request(self, query_content) -> None: + def test_bad_request(self, query_content: bytes) -> None: """ Querying with bad data returns a reasonable error code. """ @@ -131,7 +131,7 @@ class SendJoinFederationTests(unittest.FederatingHomeserverTestCase): login.register_servlets, ] - def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: super().prepare(reactor, clock, hs) self._storage_controllers = hs.get_storage_controllers() @@ -324,7 +324,7 @@ def test_send_join_contributes_to_room_join_rate_limit_and_is_limited(self) -> N # is probably sufficient to reassure that the bucket is updated. -def _create_acl_event(content): +def _create_acl_event(content: JsonDict) -> EventBase: return make_event_from_dict( { "room_id": "!a:b", From e9ac302f4f5299cfdcee15cc93311708445e1c75 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Feb 2023 00:46:03 +0000 Subject: [PATCH 5/6] Changelog --- changelog.d/14991.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14991.misc diff --git a/changelog.d/14991.misc b/changelog.d/14991.misc new file mode 100644 index 000000000000..93ceaeafc9b9 --- /dev/null +++ b/changelog.d/14991.misc @@ -0,0 +1 @@ +Improve type hints. From 9d770bf37e56ecd364166e619eb1d183732d1f17 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 6 Feb 2023 14:49:22 +0000 Subject: [PATCH 6/6] Avoid an uncessary type-ignore --- tests/federation/test_federation_sender.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/federation/test_federation_sender.py b/tests/federation/test_federation_sender.py index 32687cf72858..ddeffe1ad53b 100644 --- a/tests/federation/test_federation_sender.py +++ b/tests/federation/test_federation_sender.py @@ -399,7 +399,7 @@ def test_upload_signatures(self) -> None: # expect two edus self.assertEqual(len(self.edus), 2) - stream_id = None + stream_id: Optional[int] = None stream_id = self.check_device_update_edu(self.edus.pop(0), u1, "D1", stream_id) stream_id = self.check_device_update_edu(self.edus.pop(0), u1, "D2", stream_id) @@ -481,8 +481,6 @@ def test_upload_signatures(self) -> None: self.assertEqual(edu["edu_type"], EduTypes.DEVICE_LIST_UPDATE) c = edu["content"] if stream_id is not None: - # type-ignore: mypy marks the next line as unreachable; it does not seem - # to recognise the assignment to stream_id within the body of this loop self.assertEqual(c["prev_id"], [stream_id]) # type: ignore[unreachable] self.assertGreaterEqual(c["stream_id"], stream_id) stream_id = c["stream_id"]