From 18deeff15c8561c114aa1df591da1f42abd40ea0 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 28 Sep 2021 14:05:54 +0100 Subject: [PATCH 1/8] Pull out GetUserDirectoryTables helper --- tests/handlers/test_user_directory.py | 103 +++++++++++++------------- tests/storage/test_user_directory.py | 36 +++++++++ 2 files changed, 89 insertions(+), 50 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 266333c5532c..ee7785a1c168 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -11,7 +11,6 @@ # 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 List, Tuple from unittest.mock import Mock from urllib.parse import quote @@ -25,6 +24,7 @@ from synapse.types import create_requester from tests import unittest +from tests.storage.test_user_directory import GetUserDirectoryTables from tests.unittest import override_config @@ -50,6 +50,7 @@ def prepare(self, reactor, clock, hs): self.handler = hs.get_user_directory_handler() self.event_builder_factory = self.hs.get_event_builder_factory() self.event_creation_handler = self.hs.get_event_creation_handler() + self.user_dir_helper = GetUserDirectoryTables(self.store) def test_handle_local_profile_change_with_support_user(self): support_user_id = "@support:test" @@ -191,11 +192,16 @@ def test_private_room(self): self.helper.join(room, user=u2, tok=u2_token) # Check we have populated the database correctly. - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) self.assertEqual( - self._compress_shared(shares_private), {(u1, u2, room), (u2, u1, room)} + self.user_dir_helper._compress_shared(shares_private), + {(u1, u2, room), (u2, u1, room)}, ) self.assertEqual(public_users, []) @@ -215,10 +221,14 @@ def test_private_room(self): self.helper.leave(room, user=u2, tok=u2_token) # Check we have removed the values. - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) - self.assertEqual(self._compress_shared(shares_private), set()) + self.assertEqual(self.user_dir_helper._compress_shared(shares_private), set()) self.assertEqual(public_users, []) # User1 now gets no search results for any of the other users. @@ -246,11 +256,16 @@ def test_spam_checker(self): self.helper.join(room, user=u2, tok=u2_token) # Check we have populated the database correctly. - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) self.assertEqual( - self._compress_shared(shares_private), {(u1, u2, room), (u2, u1, room)} + self.user_dir_helper._compress_shared(shares_private), + {(u1, u2, room), (u2, u1, room)}, ) self.assertEqual(public_users, []) @@ -300,11 +315,16 @@ def test_legacy_spam_checker(self): self.helper.join(room, user=u2, tok=u2_token) # Check we have populated the database correctly. - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) self.assertEqual( - self._compress_shared(shares_private), {(u1, u2, room), (u2, u1, room)} + self.user_dir_helper._compress_shared(shares_private), + {(u1, u2, room), (u2, u1, room)}, ) self.assertEqual(public_users, []) @@ -317,35 +337,6 @@ def test_legacy_spam_checker(self): s = self.get_success(self.handler.search_users(u1, "user2", 10)) self.assertEqual(len(s["results"]), 1) - def _compress_shared(self, shared): - """ - Compress a list of users who share rooms dicts to a list of tuples. - """ - r = set() - for i in shared: - r.add((i["user_id"], i["other_user_id"], i["room_id"])) - return r - - def get_users_in_public_rooms(self) -> List[Tuple[str, str]]: - r = self.get_success( - self.store.db_pool.simple_select_list( - "users_in_public_rooms", None, ("user_id", "room_id") - ) - ) - retval = [] - for i in r: - retval.append((i["user_id"], i["room_id"])) - return retval - - def get_users_who_share_private_rooms(self) -> List[Tuple[str, str, str]]: - return self.get_success( - self.store.db_pool.simple_select_list( - "users_who_share_private_rooms", - None, - ["user_id", "other_user_id", "room_id"], - ) - ) - def _add_background_updates(self): """ Add the background updates we need to run. @@ -415,8 +406,12 @@ def test_initial(self): self.get_success(self.store.update_user_directory_stream_pos(None)) self.get_success(self.store.delete_all_from_user_dir()) - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) # Nothing updated yet self.assertEqual(shares_private, []) @@ -432,15 +427,19 @@ def test_initial(self): self.store.db_pool.updates.do_next_background_update(100), by=0.1 ) - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) # User 1 and User 2 are in the same public room self.assertEqual(set(public_users), {(u1, room), (u2, room)}) # User 1 and User 3 share private rooms self.assertEqual( - self._compress_shared(shares_private), + self.user_dir_helper._compress_shared(shares_private), {(u1, u3, private_room), (u3, u1, private_room)}, ) @@ -471,12 +470,16 @@ def test_initial_share_all_users(self): self.store.db_pool.updates.do_next_background_update(100), by=0.1 ) - shares_private = self.get_users_who_share_private_rooms() - public_users = self.get_users_in_public_rooms() + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) # No users share rooms self.assertEqual(public_users, []) - self.assertEqual(self._compress_shared(shares_private), set()) + self.assertEqual(self.user_dir_helper._compress_shared(shares_private), set()) # Despite not sharing a room, search_all_users means we get a search # result. diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 222e5d129d73..3f5e536fe585 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -11,6 +11,9 @@ # 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 Dict, List, Tuple + +from synapse.storage import DataStore from tests.unittest import HomeserverTestCase, override_config @@ -21,6 +24,39 @@ BELA = "@somenickname:a" +class GetUserDirectoryTables: + """Helper functions that we want to reuse in tests/handlers/test+user_directory.py""" + + def __init__(self, store: DataStore): + self.store = store + + def _compress_shared(self, shared: List[Dict[str, str]]): + """ + Compress a list of users who share rooms dicts to a list of tuples. + """ + r = set() + for i in shared: + r.add((i["user_id"], i["other_user_id"], i["room_id"])) + return r + + async def get_users_in_public_rooms(self) -> List[Tuple[str, str]]: + r = await self.store.db_pool.simple_select_list( + "users_in_public_rooms", None, ("user_id", "room_id") + ) + + retval = [] + for i in r: + retval.append((i["user_id"], i["room_id"])) + return retval + + async def get_users_who_share_private_rooms(self) -> List[Dict[str, str]]: + return await self.store.db_pool.simple_select_list( + "users_who_share_private_rooms", + None, + ["user_id", "other_user_id", "room_id"], + ) + + class UserDirectoryStoreTestCase(HomeserverTestCase): def prepare(self, reactor, clock, hs): self.store = hs.get_datastore() From 1112098a0afbb6c1612e2c6d995d1a91cafa24f5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 28 Sep 2021 16:57:04 +0100 Subject: [PATCH 2/8] Don't rebuild the dir in tests that don't need it In #10796 I changed registering a user to add directory entries under. This means we don't have to force a directory regbuild in to tests of the user directory search. --- tests/handlers/test_user_directory.py | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index ee7785a1c168..2fc07cf07d2a 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -456,20 +456,6 @@ def test_initial_share_all_users(self): self.register_user("user2", "pass") u3 = self.register_user("user3", "pass") - # Wipe the user dir - self.get_success(self.store.update_user_directory_stream_pos(None)) - self.get_success(self.store.delete_all_from_user_dir()) - - # Do the initial population of the user directory via the background update - self._add_background_updates() - - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) - shares_private = self.get_success( self.user_dir_helper.get_users_who_share_private_rooms() ) @@ -538,15 +524,6 @@ def test_prefer_local_users(self): local_users = [local_user_1, local_user_2, local_user_3] remote_users = [remote_user_1, remote_user_2, remote_user_3] - # Populate the user directory via background update - self._add_background_updates() - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) - # The local searching user searches for the term "user", which other users have # in their user id results = self.get_success( From 707dc17507a17a9e9ddd66ad44c30726b7f1bbff Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 28 Sep 2021 14:21:16 +0100 Subject: [PATCH 3/8] Move test_initial to tests/storage --- tests/handlers/test_user_directory.py | 118 ++------------------- tests/storage/test_user_directory.py | 142 ++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 108 deletions(-) diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 2fc07cf07d2a..deeffdf3dd14 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -29,8 +29,16 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): - """ - Tests the UserDirectoryHandler. + """Tests the UserDirectoryHandler. + + We're broadly testing two kinds of things here. + + 1. Check that we correctly update the user directory in response + to events (e.g. join a room, leave a room, change name, make public) + 2. Check that the search logic behaves as expected. + + The background process that rebuilds the user directory is tested in + tests/storage/test_user_directory.py. """ servlets = [ @@ -337,112 +345,6 @@ def test_legacy_spam_checker(self): s = self.get_success(self.handler.search_users(u1, "user2", 10)) self.assertEqual(len(s["results"]), 1) - def _add_background_updates(self): - """ - Add the background updates we need to run. - """ - # Ugh, have to reset this flag - self.store.db_pool.updates._all_done = False - - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_user_directory_createtables", - "progress_json": "{}", - }, - ) - ) - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_user_directory_process_rooms", - "progress_json": "{}", - "depends_on": "populate_user_directory_createtables", - }, - ) - ) - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_user_directory_process_users", - "progress_json": "{}", - "depends_on": "populate_user_directory_process_rooms", - }, - ) - ) - self.get_success( - self.store.db_pool.simple_insert( - "background_updates", - { - "update_name": "populate_user_directory_cleanup", - "progress_json": "{}", - "depends_on": "populate_user_directory_process_users", - }, - ) - ) - - def test_initial(self): - """ - The user directory's initial handler correctly updates the search tables. - """ - u1 = self.register_user("user1", "pass") - u1_token = self.login(u1, "pass") - u2 = self.register_user("user2", "pass") - u2_token = self.login(u2, "pass") - u3 = self.register_user("user3", "pass") - u3_token = self.login(u3, "pass") - - room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) - self.helper.invite(room, src=u1, targ=u2, tok=u1_token) - self.helper.join(room, user=u2, tok=u2_token) - - private_room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) - self.helper.invite(private_room, src=u1, targ=u3, tok=u1_token) - self.helper.join(private_room, user=u3, tok=u3_token) - - self.get_success(self.store.update_user_directory_stream_pos(None)) - self.get_success(self.store.delete_all_from_user_dir()) - - shares_private = self.get_success( - self.user_dir_helper.get_users_who_share_private_rooms() - ) - public_users = self.get_success( - self.user_dir_helper.get_users_in_public_rooms() - ) - - # Nothing updated yet - self.assertEqual(shares_private, []) - self.assertEqual(public_users, []) - - # Do the initial population of the user directory via the background update - self._add_background_updates() - - while not self.get_success( - self.store.db_pool.updates.has_completed_background_updates() - ): - self.get_success( - self.store.db_pool.updates.do_next_background_update(100), by=0.1 - ) - - shares_private = self.get_success( - self.user_dir_helper.get_users_who_share_private_rooms() - ) - public_users = self.get_success( - self.user_dir_helper.get_users_in_public_rooms() - ) - - # User 1 and User 2 are in the same public room - self.assertEqual(set(public_users), {(u1, room), (u2, room)}) - - # User 1 and User 3 share private rooms - self.assertEqual( - self.user_dir_helper._compress_shared(shares_private), - {(u1, u3, private_room), (u3, u1, private_room)}, - ) - def test_initial_share_all_users(self): """ Search all users = True means that a user does not have to share a diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 3f5e536fe585..02c9a9412134 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -13,6 +13,8 @@ # limitations under the License. from typing import Dict, List, Tuple +from synapse.rest import admin +from synapse.rest.client import login, room from synapse.storage import DataStore from tests.unittest import HomeserverTestCase, override_config @@ -57,6 +59,146 @@ async def get_users_who_share_private_rooms(self) -> List[Dict[str, str]]: ) +class UserDirectoryInitialPopulationTestcase(HomeserverTestCase): + """Ensure that rebuilding the directory writes the correct data to the DB. + + See also tests/handlers/test_user_directory.py for similar checks. They + test the incremental updates, rather than the big rebuild. + """ + + servlets = [ + login.register_servlets, + admin.register_servlets_for_client_rest_resource, + room.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.store = hs.get_datastore() + self.user_dir_helper = GetUserDirectoryTables(self.store) + + def _purge_and_rebuild_user_dir(self): + """Nuke the user directory tables, start the background process to + repopulate them, and wait for the process to complete. This allows us + to inspect the outcome of the background process alone, without any of + the other incremental updates. + """ + self.get_success(self.store.update_user_directory_stream_pos(None)) + self.get_success(self.store.delete_all_from_user_dir()) + + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) + + # Nothing updated yet + self.assertEqual(shares_private, []) + self.assertEqual(public_users, []) + + # Ugh, have to reset this flag + self.store.db_pool.updates._all_done = False + + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_createtables", + "progress_json": "{}", + }, + ) + ) + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_process_rooms", + "progress_json": "{}", + "depends_on": "populate_user_directory_createtables", + }, + ) + ) + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_process_users", + "progress_json": "{}", + "depends_on": "populate_user_directory_process_rooms", + }, + ) + ) + self.get_success( + self.store.db_pool.simple_insert( + "background_updates", + { + "update_name": "populate_user_directory_cleanup", + "progress_json": "{}", + "depends_on": "populate_user_directory_process_users", + }, + ) + ) + + while not self.get_success( + self.store.db_pool.updates.has_completed_background_updates() + ): + self.get_success( + self.store.db_pool.updates.do_next_background_update(100), by=0.1 + ) + + def test_initial(self): + """ + The user directory's initial handler correctly updates the search tables. + """ + u1 = self.register_user("user1", "pass") + u1_token = self.login(u1, "pass") + u2 = self.register_user("user2", "pass") + u2_token = self.login(u2, "pass") + u3 = self.register_user("user3", "pass") + u3_token = self.login(u3, "pass") + + room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) + self.helper.invite(room, src=u1, targ=u2, tok=u1_token) + self.helper.join(room, user=u2, tok=u2_token) + + private_room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) + self.helper.invite(private_room, src=u1, targ=u3, tok=u1_token) + self.helper.join(private_room, user=u3, tok=u3_token) + + self.get_success(self.store.update_user_directory_stream_pos(None)) + self.get_success(self.store.delete_all_from_user_dir()) + + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) + + # Nothing updated yet + self.assertEqual(shares_private, []) + self.assertEqual(public_users, []) + + # Do the initial population of the user directory via the background update + self._purge_and_rebuild_user_dir() + + shares_private = self.get_success( + self.user_dir_helper.get_users_who_share_private_rooms() + ) + public_users = self.get_success( + self.user_dir_helper.get_users_in_public_rooms() + ) + + # User 1 and User 2 are in the same public room + self.assertEqual(set(public_users), {(u1, room), (u2, room)}) + + # User 1 and User 3 share private rooms + self.assertEqual( + self.user_dir_helper._compress_shared(shares_private), + {(u1, u3, private_room), (u3, u1, private_room)}, + ) + + class UserDirectoryStoreTestCase(HomeserverTestCase): def prepare(self, reactor, clock, hs): self.store = hs.get_datastore() From 495c308dec9312f50993a9fa4f2793310190771d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 28 Sep 2021 16:33:59 +0100 Subject: [PATCH 4/8] Add type hints to both test_user_directory files --- mypy.ini | 6 ++ .../storage/databases/main/user_directory.py | 2 +- tests/handlers/test_user_directory.py | 65 +++++++++++-------- tests/storage/test_user_directory.py | 24 ++++--- tests/unittest.py | 4 +- 5 files changed, 64 insertions(+), 37 deletions(-) diff --git a/mypy.ini b/mypy.ini index 437d0a46a593..568166db3300 100644 --- a/mypy.ini +++ b/mypy.ini @@ -162,6 +162,12 @@ disallow_untyped_defs = True [mypy-synapse.util.wheel_timer] disallow_untyped_defs = True +[mypy-tests.handlers.test_user_directory] +disallow_untyped_defs = True + +[mypy-tests.storage.test_user_directory] +disallow_untyped_defs = True + [mypy-pymacaroons.*] ignore_missing_imports = True diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 90d65edc4267..c26e3e066f9d 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -527,7 +527,7 @@ async def get_user_in_directory(self, user_id: str) -> Optional[Dict[str, str]]: desc="get_user_in_directory", ) - async def update_user_directory_stream_pos(self, stream_id: int) -> None: + async def update_user_directory_stream_pos(self, stream_id: Optional[int]) -> None: await self.db_pool.simple_update_one( table="user_directory_stream_pos", keyvalues={}, diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index deeffdf3dd14..9dd2564d3f87 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -11,17 +11,20 @@ # 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 unittest.mock import Mock +from unittest.mock import Mock, patch from urllib.parse import quote from twisted.internet import defer +from twisted.internet.testing import MemoryReactor import synapse.rest.admin from synapse.api.constants import UserTypes from synapse.api.room_versions import RoomVersion, RoomVersions from synapse.rest.client import login, room, user_directory +from synapse.server import HomeServer from synapse.storage.roommember import ProfileInfo from synapse.types import create_requester +from synapse.util import Clock from tests import unittest from tests.storage.test_user_directory import GetUserDirectoryTables @@ -47,20 +50,19 @@ class UserDirectoryTestCase(unittest.HomeserverTestCase): room.register_servlets, ] - def make_homeserver(self, reactor, clock): - + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: config = self.default_config() config["update_user_directory"] = True return self.setup_test_homeserver(config=config) - def prepare(self, reactor, clock, hs): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastore() self.handler = hs.get_user_directory_handler() self.event_builder_factory = self.hs.get_event_builder_factory() self.event_creation_handler = self.hs.get_event_creation_handler() self.user_dir_helper = GetUserDirectoryTables(self.store) - def test_handle_local_profile_change_with_support_user(self): + def test_handle_local_profile_change_with_support_user(self) -> None: support_user_id = "@support:test" self.get_success( self.store.register_user( @@ -73,7 +75,9 @@ def test_handle_local_profile_change_with_support_user(self): ) self.get_success( - self.handler.handle_local_profile_change(support_user_id, None) + self.handler.handle_local_profile_change( + support_user_id, ProfileInfo("I love support me", None) + ) ) profile = self.get_success(self.store.get_user_in_directory(support_user_id)) self.assertTrue(profile is None) @@ -86,7 +90,7 @@ def test_handle_local_profile_change_with_support_user(self): profile = self.get_success(self.store.get_user_in_directory(regular_user_id)) self.assertTrue(profile["display_name"] == display_name) - def test_handle_local_profile_change_with_deactivated_user(self): + def test_handle_local_profile_change_with_deactivated_user(self) -> None: # create user r_user_id = "@regular:test" self.get_success( @@ -121,7 +125,7 @@ def test_handle_local_profile_change_with_deactivated_user(self): profile = self.get_success(self.store.get_user_in_directory(r_user_id)) self.assertTrue(profile is None) - def test_handle_user_deactivated_support_user(self): + def test_handle_user_deactivated_support_user(self) -> None: s_user_id = "@support:test" self.get_success( self.store.register_user( @@ -129,20 +133,29 @@ def test_handle_user_deactivated_support_user(self): ) ) - self.store.remove_from_user_dir = Mock(return_value=defer.succeed(None)) - self.get_success(self.handler.handle_local_user_deactivated(s_user_id)) - self.store.remove_from_user_dir.not_called() + mock_remove_from_user_dir = Mock(return_value=defer.succeed(None)) + with patch.object( + self.store, "remove_from_user_dir", mock_remove_from_user_dir + ): + self.get_success(self.handler.handle_local_user_deactivated(s_user_id)) + # BUG: the correct spelling is assert_not_called, but that makes the test fail + # and it's not clear that this is actually the behaviour we want. + mock_remove_from_user_dir.not_called() - def test_handle_user_deactivated_regular_user(self): + def test_handle_user_deactivated_regular_user(self) -> None: r_user_id = "@regular:test" self.get_success( self.store.register_user(user_id=r_user_id, password_hash=None) ) - self.store.remove_from_user_dir = Mock(return_value=defer.succeed(None)) - self.get_success(self.handler.handle_local_user_deactivated(r_user_id)) - self.store.remove_from_user_dir.called_once_with(r_user_id) - def test_reactivation_makes_regular_user_searchable(self): + mock_remove_from_user_dir = Mock(return_value=defer.succeed(None)) + with patch.object( + self.store, "remove_from_user_dir", mock_remove_from_user_dir + ): + self.get_success(self.handler.handle_local_user_deactivated(r_user_id)) + mock_remove_from_user_dir.assert_called_once_with(r_user_id) + + def test_reactivation_makes_regular_user_searchable(self) -> None: user = self.register_user("regular", "pass") user_token = self.login(user, "pass") admin_user = self.register_user("admin", "pass", admin=True) @@ -180,7 +193,7 @@ def test_reactivation_makes_regular_user_searchable(self): self.assertEqual(len(s["results"]), 1) self.assertEqual(s["results"][0]["user_id"], user) - def test_private_room(self): + def test_private_room(self) -> None: """ A user can be searched for only by people that are either in a public room, or that share a private chat. @@ -246,7 +259,7 @@ def test_private_room(self): s = self.get_success(self.handler.search_users(u1, "user3", 10)) self.assertEqual(len(s["results"]), 0) - def test_spam_checker(self): + def test_spam_checker(self) -> None: """ A user which fails the spam checks will not appear in search results. """ @@ -281,7 +294,7 @@ def test_spam_checker(self): s = self.get_success(self.handler.search_users(u1, "user2", 10)) self.assertEqual(len(s["results"]), 1) - async def allow_all(user_profile): + async def allow_all(user_profile: ProfileInfo) -> bool: # Allow all users. return False @@ -295,7 +308,7 @@ async def allow_all(user_profile): self.assertEqual(len(s["results"]), 1) # Configure a spam checker that filters all users. - async def block_all(user_profile): + async def block_all(user_profile: ProfileInfo) -> bool: # All users are spammy. return True @@ -305,7 +318,7 @@ async def block_all(user_profile): s = self.get_success(self.handler.search_users(u1, "user2", 10)) self.assertEqual(len(s["results"]), 0) - def test_legacy_spam_checker(self): + def test_legacy_spam_checker(self) -> None: """ A spam checker without the expected method should be ignored. """ @@ -345,7 +358,7 @@ def test_legacy_spam_checker(self): s = self.get_success(self.handler.search_users(u1, "user2", 10)) self.assertEqual(len(s["results"]), 1) - def test_initial_share_all_users(self): + def test_initial_share_all_users(self) -> None: """ Search all users = True means that a user does not have to share a private room with the searching user or be in a public room to be search @@ -392,7 +405,7 @@ def test_initial_share_all_users(self): } } ) - def test_prefer_local_users(self): + def test_prefer_local_users(self) -> None: """Tests that local users are shown higher in search results when user_directory.prefer_local_users is True. """ @@ -447,7 +460,7 @@ def _add_user_to_room( room_id: str, room_version: RoomVersion, user_id: str, - ): + ) -> None: # Add a user to the room. builder = self.event_builder_factory.for_room_version( room_version, @@ -479,7 +492,7 @@ class TestUserDirSearchDisabled(unittest.HomeserverTestCase): synapse.rest.admin.register_servlets_for_client_rest_resource, ] - def make_homeserver(self, reactor, clock): + def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: config = self.default_config() config["update_user_directory"] = True hs = self.setup_test_homeserver(config=config) @@ -488,7 +501,7 @@ def make_homeserver(self, reactor, clock): return hs - def test_disabling_room_list(self): + def test_disabling_room_list(self) -> None: self.config.userdirectory.user_directory_search_enabled = True # First we create a room with another user so that user dir is non-empty diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 02c9a9412134..d0b7e45c7acb 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -11,11 +11,15 @@ # 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 Dict, List, Tuple +from typing import Dict, List, Set, Tuple + +from twisted.internet.testing import MemoryReactor from synapse.rest import admin from synapse.rest.client import login, room +from synapse.server import HomeServer from synapse.storage import DataStore +from synapse.util import Clock from tests.unittest import HomeserverTestCase, override_config @@ -32,7 +36,9 @@ class GetUserDirectoryTables: def __init__(self, store: DataStore): self.store = store - def _compress_shared(self, shared: List[Dict[str, str]]): + def _compress_shared( + self, shared: List[Dict[str, str]] + ) -> Set[Tuple[str, str, str]]: """ Compress a list of users who share rooms dicts to a list of tuples. """ @@ -72,11 +78,11 @@ class UserDirectoryInitialPopulationTestcase(HomeserverTestCase): room.register_servlets, ] - def prepare(self, reactor, clock, hs): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastore() self.user_dir_helper = GetUserDirectoryTables(self.store) - def _purge_and_rebuild_user_dir(self): + def _purge_and_rebuild_user_dir(self) -> None: """Nuke the user directory tables, start the background process to repopulate them, and wait for the process to complete. This allows us to inspect the outcome of the background process alone, without any of @@ -146,7 +152,7 @@ def _purge_and_rebuild_user_dir(self): self.store.db_pool.updates.do_next_background_update(100), by=0.1 ) - def test_initial(self): + def test_initial(self) -> None: """ The user directory's initial handler correctly updates the search tables. """ @@ -200,7 +206,7 @@ def test_initial(self): class UserDirectoryStoreTestCase(HomeserverTestCase): - def prepare(self, reactor, clock, hs): + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastore() # alice and bob are both in !room_id. bobby is not but shares @@ -211,7 +217,7 @@ def prepare(self, reactor, clock, hs): self.get_success(self.store.update_profile_in_user_dir(BELA, "Bela", None)) self.get_success(self.store.add_users_in_public_rooms("!room:id", (ALICE, BOB))) - def test_search_user_dir(self): + def test_search_user_dir(self) -> None: # normally when alice searches the directory she should just find # bob because bobby doesn't share a room with her. r = self.get_success(self.store.search_user_dir(ALICE, "bob", 10)) @@ -222,7 +228,7 @@ def test_search_user_dir(self): ) @override_config({"user_directory": {"search_all_users": True}}) - def test_search_user_dir_all_users(self): + def test_search_user_dir_all_users(self) -> None: r = self.get_success(self.store.search_user_dir(ALICE, "bob", 10)) self.assertFalse(r["limited"]) self.assertEqual(2, len(r["results"])) @@ -236,7 +242,7 @@ def test_search_user_dir_all_users(self): ) @override_config({"user_directory": {"search_all_users": True}}) - def test_search_user_dir_stop_words(self): + def test_search_user_dir_stop_words(self) -> None: """Tests that a user can look up another user by searching for the start if its display name even if that name happens to be a common English word that would usually be ignored in full text searches. diff --git a/tests/unittest.py b/tests/unittest.py index 7a6f5954d06c..442841479d9e 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -26,6 +26,7 @@ from canonicaljson import json from twisted.internet.defer import Deferred, ensureDeferred, succeed +from twisted.internet.testing import MemoryReactor from twisted.python.failure import Failure from twisted.python.threadpool import ThreadPool from twisted.trial import unittest @@ -46,6 +47,7 @@ ) from synapse.server import HomeServer from synapse.types import UserID, create_requester +from synapse.util import Clock from synapse.util.httpresourcetree import create_resource_tree from synapse.util.ratelimitutils import FederationRateLimiter @@ -371,7 +373,7 @@ def default_config(self): return config - def prepare(self, reactor, clock, homeserver): + def prepare(self, reactor: MemoryReactor, clock: Clock, homeserver: HomeServer): """ Prepare for the test. This involves things like mocking out parts of the homeserver, or building test data common across the whole test From da6260c50c1139c22784eec8ef42a70f6f714779 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 28 Sep 2021 17:17:54 +0100 Subject: [PATCH 5/8] Changelog --- changelog.d/10935.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/10935.misc diff --git a/changelog.d/10935.misc b/changelog.d/10935.misc new file mode 100644 index 000000000000..80529c04cae2 --- /dev/null +++ b/changelog.d/10935.misc @@ -0,0 +1 @@ +Refactor user directory tests in preparation for upcoming changes. From b7e3b9f76d31408be798d241d2df45bc34992982 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 28 Sep 2021 18:39:17 +0100 Subject: [PATCH 6/8] Workaround twisted type movement Olddeps looks to be using twisted 18.9. twisted/twisted:4cade8bb1e5ce45a86962fe2548470a413c8ce7a moved the location of `MemoryReactor` in 19.7, so we'll have to use the old import on olddeps. --- tests/handlers/test_user_directory.py | 2 +- tests/storage/test_user_directory.py | 3 +-- tests/test_terms_auth.py | 3 +-- tests/types.py | 9 +++++++++ tests/unittest.py | 2 +- 5 files changed, 13 insertions(+), 6 deletions(-) create mode 100644 tests/types.py diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index 9dd2564d3f87..da94bbce6420 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -15,7 +15,6 @@ from urllib.parse import quote from twisted.internet import defer -from twisted.internet.testing import MemoryReactor import synapse.rest.admin from synapse.api.constants import UserTypes @@ -28,6 +27,7 @@ from tests import unittest from tests.storage.test_user_directory import GetUserDirectoryTables +from tests.types import MemoryReactor from tests.unittest import override_config diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index d0b7e45c7acb..2f709edb0313 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -13,14 +13,13 @@ # limitations under the License. from typing import Dict, List, Set, Tuple -from twisted.internet.testing import MemoryReactor - from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer from synapse.storage import DataStore from synapse.util import Clock +from tests.types import MemoryReactor from tests.unittest import HomeserverTestCase, override_config ALICE = "@alice:a" diff --git a/tests/test_terms_auth.py b/tests/test_terms_auth.py index 67dcf567cdb8..b5201e810ff2 100644 --- a/tests/test_terms_auth.py +++ b/tests/test_terms_auth.py @@ -15,12 +15,11 @@ import json from unittest.mock import Mock -from twisted.test.proto_helpers import MemoryReactorClock - from synapse.rest.client.register import register_servlets from synapse.util import Clock from tests import unittest +from tests.types import MemoryReactorClock class TermsTestCase(unittest.HomeserverTestCase): diff --git a/tests/types.py b/tests/types.py new file mode 100644 index 000000000000..a85565cef9fb --- /dev/null +++ b/tests/types.py @@ -0,0 +1,9 @@ +try: + from twisted.internet.testing import MemoryReactor, MemoryReactorClock +except ImportError: + # Twisted 19.7 moved stuff from twisted.test.proto_helpers to + # twisted.internet.testing. Let's do the import once rather than doing it + # in lots of different files. + from twisted.test.proto_helpers import MemoryReactor, MemoryReactorClock + +__all__ = ["MemoryReactor", "MemoryReactorClock"] diff --git a/tests/unittest.py b/tests/unittest.py index 442841479d9e..5b960f0917fc 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -26,7 +26,6 @@ from canonicaljson import json from twisted.internet.defer import Deferred, ensureDeferred, succeed -from twisted.internet.testing import MemoryReactor from twisted.python.failure import Failure from twisted.python.threadpool import ThreadPool from twisted.trial import unittest @@ -54,6 +53,7 @@ from tests.server import FakeChannel, get_clock, make_request, setup_test_homeserver from tests.test_utils import event_injection, setup_awaitable_errors from tests.test_utils.logging_setup import setup_logging +from tests.types import MemoryReactor from tests.utils import default_config, setupdb setupdb() From 8dc90a7ef390a89a75a3275cdd3dabb34005a15d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 29 Sep 2021 17:22:17 +0100 Subject: [PATCH 7/8] Typo fix, thanks Rich Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- tests/storage/test_user_directory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 2f709edb0313..819970796870 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -30,7 +30,7 @@ class GetUserDirectoryTables: - """Helper functions that we want to reuse in tests/handlers/test+user_directory.py""" + """Helper functions that we want to reuse in tests/handlers/test_user_directory.py""" def __init__(self, store: DataStore): self.store = store From 9a3d35e3bbed71da4312c79c73957b7675bb5f5f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Wed, 29 Sep 2021 17:27:29 +0100 Subject: [PATCH 8/8] Actually, just use twisted.test.proto_helpers --- tests/handlers/test_user_directory.py | 2 +- tests/storage/test_user_directory.py | 3 ++- tests/test_terms_auth.py | 3 ++- tests/types.py | 9 --------- tests/unittest.py | 2 +- 5 files changed, 6 insertions(+), 13 deletions(-) delete mode 100644 tests/types.py diff --git a/tests/handlers/test_user_directory.py b/tests/handlers/test_user_directory.py index da94bbce6420..2988befb21b4 100644 --- a/tests/handlers/test_user_directory.py +++ b/tests/handlers/test_user_directory.py @@ -15,6 +15,7 @@ from urllib.parse import quote from twisted.internet import defer +from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin from synapse.api.constants import UserTypes @@ -27,7 +28,6 @@ from tests import unittest from tests.storage.test_user_directory import GetUserDirectoryTables -from tests.types import MemoryReactor from tests.unittest import override_config diff --git a/tests/storage/test_user_directory.py b/tests/storage/test_user_directory.py index 819970796870..74c8a8599e7d 100644 --- a/tests/storage/test_user_directory.py +++ b/tests/storage/test_user_directory.py @@ -13,13 +13,14 @@ # limitations under the License. from typing import Dict, List, Set, Tuple +from twisted.test.proto_helpers import MemoryReactor + from synapse.rest import admin from synapse.rest.client import login, room from synapse.server import HomeServer from synapse.storage import DataStore from synapse.util import Clock -from tests.types import MemoryReactor from tests.unittest import HomeserverTestCase, override_config ALICE = "@alice:a" diff --git a/tests/test_terms_auth.py b/tests/test_terms_auth.py index b5201e810ff2..67dcf567cdb8 100644 --- a/tests/test_terms_auth.py +++ b/tests/test_terms_auth.py @@ -15,11 +15,12 @@ import json from unittest.mock import Mock +from twisted.test.proto_helpers import MemoryReactorClock + from synapse.rest.client.register import register_servlets from synapse.util import Clock from tests import unittest -from tests.types import MemoryReactorClock class TermsTestCase(unittest.HomeserverTestCase): diff --git a/tests/types.py b/tests/types.py deleted file mode 100644 index a85565cef9fb..000000000000 --- a/tests/types.py +++ /dev/null @@ -1,9 +0,0 @@ -try: - from twisted.internet.testing import MemoryReactor, MemoryReactorClock -except ImportError: - # Twisted 19.7 moved stuff from twisted.test.proto_helpers to - # twisted.internet.testing. Let's do the import once rather than doing it - # in lots of different files. - from twisted.test.proto_helpers import MemoryReactor, MemoryReactorClock - -__all__ = ["MemoryReactor", "MemoryReactorClock"] diff --git a/tests/unittest.py b/tests/unittest.py index 5b960f0917fc..3696897fa91b 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -28,6 +28,7 @@ from twisted.internet.defer import Deferred, ensureDeferred, succeed from twisted.python.failure import Failure from twisted.python.threadpool import ThreadPool +from twisted.test.proto_helpers import MemoryReactor from twisted.trial import unittest from twisted.web.resource import Resource @@ -53,7 +54,6 @@ from tests.server import FakeChannel, get_clock, make_request, setup_test_homeserver from tests.test_utils import event_injection, setup_awaitable_errors from tests.test_utils.logging_setup import setup_logging -from tests.types import MemoryReactor from tests.utils import default_config, setupdb setupdb()