From b0cf25b3397ba735557c1ba0162dcec46a043d44 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 3 Dec 2021 11:35:24 +0000 Subject: [PATCH 1/7] Convert one of the `setup_test_homeserver`s to `make_test_homeserver_synchronous` and pass in the homeserver rather than calling a same-named function to ask for one. Later commits will jiggle things around to make this sensible. --- tests/server.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/tests/server.py b/tests/server.py index 40cf5b12c3a0..41eb3995bdd9 100644 --- a/tests/server.py +++ b/tests/server.py @@ -57,7 +57,6 @@ from synapse.types import JsonDict from synapse.util import Clock -from tests.utils import setup_test_homeserver as _sth logger = logging.getLogger(__name__) @@ -450,14 +449,11 @@ def _(res): return d -def setup_test_homeserver(cleanup_func, *args, **kwargs): +def make_test_homeserver_synchronous(server: HomeServer) -> None: """ - Set up a synchronous test server, driven by the reactor used by - the homeserver. + Make the given test homeserver's database interactions synchronous. """ - server = _sth(cleanup_func, *args, **kwargs) - # Make the thread pool synchronous. clock = server.get_clock() for database in server.get_datastores().databases: @@ -485,6 +481,7 @@ def runInteraction(interaction, *args, **kwargs): pool.runWithConnection = runWithConnection pool.runInteraction = runInteraction + # Replace the thread pool with a threadless 'thread' pool pool.threadpool = ThreadPool(clock._reactor) pool.running = True @@ -492,8 +489,6 @@ def runInteraction(interaction, *args, **kwargs): # thread, so we need to disable the dedicated thread behaviour. server.get_datastores().main.USE_DEDICATED_DB_THREADS_FOR_EVENT_FETCHING = False - return server - def get_clock() -> Tuple[ThreadedMemoryReactorClock, Clock]: clock = ThreadedMemoryReactorClock() From 22f8b6c3e609fa8691be61204c617cc76dd70283 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 3 Dec 2021 11:37:21 +0000 Subject: [PATCH 2/7] Move `tests.utils.setup_test_homeserver` to `tests.server` It had no users. We have just taken the identity of a previous function but don't provide the same behaviour, so we need to fix this in the next commit... --- tests/server.py | 185 ++++++++++++++++++++++++++++++- tests/storage/test_base.py | 3 +- tests/storage/test_roommember.py | 2 +- tests/utils.py | 175 +---------------------------- 4 files changed, 188 insertions(+), 177 deletions(-) diff --git a/tests/server.py b/tests/server.py index 41eb3995bdd9..017e5cf6356c 100644 --- a/tests/server.py +++ b/tests/server.py @@ -11,9 +11,12 @@ # 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. - +import hashlib import json import logging +import time +import uuid +import warnings from collections import deque from io import SEEK_END, BytesIO from typing import ( @@ -27,6 +30,7 @@ Type, Union, ) +from unittest.mock import Mock import attr from typing_extensions import Deque @@ -53,10 +57,24 @@ from twisted.web.resource import IResource from twisted.web.server import Request, Site +from synapse.config.database import DatabaseConnectionConfig from synapse.http.site import SynapseRequest +from synapse.server import HomeServer +from synapse.storage import DataStore +from synapse.storage.engines import PostgresEngine, create_engine from synapse.types import JsonDict from synapse.util import Clock +from tests.utils import ( + LEAVE_DB, + POSTGRES_BASE_DB, + POSTGRES_HOST, + POSTGRES_PASSWORD, + POSTGRES_USER, + USE_POSTGRES_FOR_TESTS, + MockClock, + default_config, +) logger = logging.getLogger(__name__) @@ -668,3 +686,168 @@ def connect_client( client.makeConnection(FakeTransport(server, reactor)) return client, server + + +class TestHomeServer(HomeServer): + DATASTORE_CLASS = DataStore + + +def setup_test_homeserver( + cleanup_func, + name="test", + config=None, + reactor=None, + homeserver_to_use: Type[HomeServer] = TestHomeServer, + **kwargs, +): + """ + Setup a homeserver suitable for running tests against. Keyword arguments + are passed to the Homeserver constructor. + + If no datastore is supplied, one is created and given to the homeserver. + + Args: + cleanup_func : The function used to register a cleanup routine for + after the test. + + Calling this method directly is deprecated: you should instead derive from + HomeserverTestCase. + """ + if reactor is None: + from twisted.internet import reactor + + if config is None: + config = default_config(name, parse=True) + + config.ldap_enabled = False + + if "clock" not in kwargs: + kwargs["clock"] = MockClock() + + if USE_POSTGRES_FOR_TESTS: + test_db = "synapse_test_%s" % uuid.uuid4().hex + + database_config = { + "name": "psycopg2", + "args": { + "database": test_db, + "host": POSTGRES_HOST, + "password": POSTGRES_PASSWORD, + "user": POSTGRES_USER, + "cp_min": 1, + "cp_max": 5, + }, + } + else: + database_config = { + "name": "sqlite3", + "args": {"database": ":memory:", "cp_min": 1, "cp_max": 1}, + } + + if "db_txn_limit" in kwargs: + database_config["txn_limit"] = kwargs["db_txn_limit"] + + database = DatabaseConnectionConfig("master", database_config) + config.database.databases = [database] + + db_engine = create_engine(database.config) + + # Create the database before we actually try and connect to it, based off + # the template database we generate in setupdb() + if isinstance(db_engine, PostgresEngine): + db_conn = db_engine.module.connect( + database=POSTGRES_BASE_DB, + user=POSTGRES_USER, + host=POSTGRES_HOST, + password=POSTGRES_PASSWORD, + ) + db_conn.autocommit = True + cur = db_conn.cursor() + cur.execute("DROP DATABASE IF EXISTS %s;" % (test_db,)) + cur.execute( + "CREATE DATABASE %s WITH TEMPLATE %s;" % (test_db, POSTGRES_BASE_DB) + ) + cur.close() + db_conn.close() + + hs = homeserver_to_use( + name, + config=config, + version_string="Synapse/tests", + reactor=reactor, + ) + + # Install @cache_in_self attributes + for key, val in kwargs.items(): + setattr(hs, "_" + key, val) + + # Mock TLS + hs.tls_server_context_factory = Mock() + hs.tls_client_options_factory = Mock() + + hs.setup() + if homeserver_to_use == TestHomeServer: + hs.setup_background_tasks() + + if isinstance(db_engine, PostgresEngine): + database = hs.get_datastores().databases[0] + + # We need to do cleanup on PostgreSQL + def cleanup(): + import psycopg2 + + # Close all the db pools + database._db_pool.close() + + dropped = False + + # Drop the test database + db_conn = db_engine.module.connect( + database=POSTGRES_BASE_DB, + user=POSTGRES_USER, + host=POSTGRES_HOST, + password=POSTGRES_PASSWORD, + ) + db_conn.autocommit = True + cur = db_conn.cursor() + + # Try a few times to drop the DB. Some things may hold on to the + # database for a few more seconds due to flakiness, preventing + # us from dropping it when the test is over. If we can't drop + # it, warn and move on. + for _ in range(5): + try: + cur.execute("DROP DATABASE IF EXISTS %s;" % (test_db,)) + db_conn.commit() + dropped = True + except psycopg2.OperationalError as e: + warnings.warn( + "Couldn't drop old db: " + str(e), category=UserWarning + ) + time.sleep(0.5) + + cur.close() + db_conn.close() + + if not dropped: + warnings.warn("Failed to drop old DB.", category=UserWarning) + + if not LEAVE_DB: + # Register the cleanup hook + cleanup_func(cleanup) + + # bcrypt is far too slow to be doing in unit tests + # Need to let the HS build an auth handler and then mess with it + # because AuthHandler's constructor requires the HS, so we can't make one + # beforehand and pass it in to the HS's constructor (chicken / egg) + async def hash(p): + return hashlib.md5(p.encode("utf8")).hexdigest() + + hs.get_auth_handler().hash = hash + + async def validate_hash(p, h): + return hashlib.md5(p.encode("utf8")).hexdigest() == h + + hs.get_auth_handler().validate_hash = validate_hash + + return hs diff --git a/tests/storage/test_base.py b/tests/storage/test_base.py index ddad44bd6cbb..3e4f0579c9cb 100644 --- a/tests/storage/test_base.py +++ b/tests/storage/test_base.py @@ -23,7 +23,8 @@ from synapse.storage.engines import create_engine from tests import unittest -from tests.utils import TestHomeServer, default_config +from tests.server import TestHomeServer +from tests.utils import default_config class SQLBaseStoreTestCase(unittest.TestCase): diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index fccab733c029..5cfdfe9b852e 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -19,8 +19,8 @@ from synapse.types import UserID, create_requester from tests import unittest +from tests.server import TestHomeServer from tests.test_utils import event_injection -from tests.utils import TestHomeServer class RoomMemberStoreTestCase(unittest.HomeserverTestCase): diff --git a/tests/utils.py b/tests/utils.py index 983859120f55..6d013e851845 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -14,12 +14,7 @@ # limitations under the License. import atexit -import hashlib import os -import time -import uuid -import warnings -from typing import Type from unittest.mock import Mock, patch from urllib import parse as urlparse @@ -28,14 +23,11 @@ from synapse.api.constants import EventTypes from synapse.api.errors import CodeMessageException, cs_error from synapse.api.room_versions import RoomVersions -from synapse.config.database import DatabaseConnectionConfig from synapse.config.homeserver import HomeServerConfig from synapse.config.server import DEFAULT_ROOM_VERSION from synapse.logging.context import current_context, set_current_context -from synapse.server import HomeServer -from synapse.storage import DataStore from synapse.storage.database import LoggingDatabaseConnection -from synapse.storage.engines import PostgresEngine, create_engine +from synapse.storage.engines import create_engine from synapse.storage.prepare_database import prepare_database # set this to True to run the tests against postgres instead of sqlite. @@ -182,171 +174,6 @@ def default_config(name, parse=False): return config_dict -class TestHomeServer(HomeServer): - DATASTORE_CLASS = DataStore - - -def setup_test_homeserver( - cleanup_func, - name="test", - config=None, - reactor=None, - homeserver_to_use: Type[HomeServer] = TestHomeServer, - **kwargs, -): - """ - Setup a homeserver suitable for running tests against. Keyword arguments - are passed to the Homeserver constructor. - - If no datastore is supplied, one is created and given to the homeserver. - - Args: - cleanup_func : The function used to register a cleanup routine for - after the test. - - Calling this method directly is deprecated: you should instead derive from - HomeserverTestCase. - """ - if reactor is None: - from twisted.internet import reactor - - if config is None: - config = default_config(name, parse=True) - - config.ldap_enabled = False - - if "clock" not in kwargs: - kwargs["clock"] = MockClock() - - if USE_POSTGRES_FOR_TESTS: - test_db = "synapse_test_%s" % uuid.uuid4().hex - - database_config = { - "name": "psycopg2", - "args": { - "database": test_db, - "host": POSTGRES_HOST, - "password": POSTGRES_PASSWORD, - "user": POSTGRES_USER, - "cp_min": 1, - "cp_max": 5, - }, - } - else: - database_config = { - "name": "sqlite3", - "args": {"database": ":memory:", "cp_min": 1, "cp_max": 1}, - } - - if "db_txn_limit" in kwargs: - database_config["txn_limit"] = kwargs["db_txn_limit"] - - database = DatabaseConnectionConfig("master", database_config) - config.database.databases = [database] - - db_engine = create_engine(database.config) - - # Create the database before we actually try and connect to it, based off - # the template database we generate in setupdb() - if isinstance(db_engine, PostgresEngine): - db_conn = db_engine.module.connect( - database=POSTGRES_BASE_DB, - user=POSTGRES_USER, - host=POSTGRES_HOST, - password=POSTGRES_PASSWORD, - ) - db_conn.autocommit = True - cur = db_conn.cursor() - cur.execute("DROP DATABASE IF EXISTS %s;" % (test_db,)) - cur.execute( - "CREATE DATABASE %s WITH TEMPLATE %s;" % (test_db, POSTGRES_BASE_DB) - ) - cur.close() - db_conn.close() - - hs = homeserver_to_use( - name, - config=config, - version_string="Synapse/tests", - reactor=reactor, - ) - - # Install @cache_in_self attributes - for key, val in kwargs.items(): - setattr(hs, "_" + key, val) - - # Mock TLS - hs.tls_server_context_factory = Mock() - hs.tls_client_options_factory = Mock() - - hs.setup() - if homeserver_to_use == TestHomeServer: - hs.setup_background_tasks() - - if isinstance(db_engine, PostgresEngine): - database = hs.get_datastores().databases[0] - - # We need to do cleanup on PostgreSQL - def cleanup(): - import psycopg2 - - # Close all the db pools - database._db_pool.close() - - dropped = False - - # Drop the test database - db_conn = db_engine.module.connect( - database=POSTGRES_BASE_DB, - user=POSTGRES_USER, - host=POSTGRES_HOST, - password=POSTGRES_PASSWORD, - ) - db_conn.autocommit = True - cur = db_conn.cursor() - - # Try a few times to drop the DB. Some things may hold on to the - # database for a few more seconds due to flakiness, preventing - # us from dropping it when the test is over. If we can't drop - # it, warn and move on. - for _ in range(5): - try: - cur.execute("DROP DATABASE IF EXISTS %s;" % (test_db,)) - db_conn.commit() - dropped = True - except psycopg2.OperationalError as e: - warnings.warn( - "Couldn't drop old db: " + str(e), category=UserWarning - ) - time.sleep(0.5) - - cur.close() - db_conn.close() - - if not dropped: - warnings.warn("Failed to drop old DB.", category=UserWarning) - - if not LEAVE_DB: - # Register the cleanup hook - cleanup_func(cleanup) - - # bcrypt is far too slow to be doing in unit tests - # Need to let the HS build an auth handler and then mess with it - # because AuthHandler's constructor requires the HS, so we can't make one - # beforehand and pass it in to the HS's constructor (chicken / egg) - async def hash(p): - return hashlib.md5(p.encode("utf8")).hexdigest() - - hs.get_auth_handler().hash = hash - - async def validate_hash(p, h): - return hashlib.md5(p.encode("utf8")).hexdigest() == h - - hs.get_auth_handler().validate_hash = validate_hash - - return hs - - def mock_getRawHeaders(headers=None): headers = headers if headers is not None else {} From 64576775e6e4c14c89d0f5172734e279f08643c9 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 3 Dec 2021 11:40:05 +0000 Subject: [PATCH 3/7] Give `tests.server.setup_test_homeserver` (nominally!) the same behaviour by calling into `make_test_homeserver_synchronous`. The function *could* have been inlined at this point but the function is big enough and it felt fine to leave it as is. At least there isn't a confusing name clash anymore! --- tests/server.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/server.py b/tests/server.py index 017e5cf6356c..b29df37595ba 100644 --- a/tests/server.py +++ b/tests/server.py @@ -850,4 +850,7 @@ async def validate_hash(p, h): hs.get_auth_handler().validate_hash = validate_hash + # Make the threadpool and database transactions synchronous for testing. + make_test_homeserver_synchronous(hs) + return hs From 537053ea8587cba7ee86946dd4b8a8e19cf8b492 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 3 Dec 2021 12:25:37 +0000 Subject: [PATCH 4/7] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/11503.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11503.misc diff --git a/changelog.d/11503.misc b/changelog.d/11503.misc new file mode 100644 index 000000000000..03a24a922495 --- /dev/null +++ b/changelog.d/11503.misc @@ -0,0 +1 @@ +Refactor `tests.util.setup_test_homeserver` and `tests.server.setup_test_homeserver`. \ No newline at end of file From 9aea6efc2bea3fa9d4915ebb4039f8d151be256b Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 21 Dec 2021 11:51:58 +0000 Subject: [PATCH 5/7] Prefix `make_test_homeserver_synchronous` with an underscore --- tests/server.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/server.py b/tests/server.py index b29df37595ba..ca2b7a5b9771 100644 --- a/tests/server.py +++ b/tests/server.py @@ -467,7 +467,7 @@ def _(res): return d -def make_test_homeserver_synchronous(server: HomeServer) -> None: +def _make_test_homeserver_synchronous(server: HomeServer) -> None: """ Make the given test homeserver's database interactions synchronous. """ @@ -851,6 +851,6 @@ async def validate_hash(p, h): hs.get_auth_handler().validate_hash = validate_hash # Make the threadpool and database transactions synchronous for testing. - make_test_homeserver_synchronous(hs) + _make_test_homeserver_synchronous(hs) return hs From d5b64809f73ec9bb23dd8decd4627d4ad59cdd0e Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 21 Dec 2021 11:54:39 +0000 Subject: [PATCH 6/7] Move LEAVE_DB across to tests.server --- tests/server.py | 5 ++++- tests/utils.py | 1 - 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/server.py b/tests/server.py index ca2b7a5b9771..64c66ef9c71c 100644 --- a/tests/server.py +++ b/tests/server.py @@ -14,6 +14,7 @@ import hashlib import json import logging +import os import time import uuid import warnings @@ -66,7 +67,6 @@ from synapse.util import Clock from tests.utils import ( - LEAVE_DB, POSTGRES_BASE_DB, POSTGRES_HOST, POSTGRES_PASSWORD, @@ -79,6 +79,9 @@ logger = logging.getLogger(__name__) +LEAVE_DB = os.environ.get("SYNAPSE_LEAVE_DB", False) + + class TimedOutException(Exception): """ A web query timed out. diff --git a/tests/utils.py b/tests/utils.py index 6d013e851845..552b98c13da9 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -36,7 +36,6 @@ # POSTGRES_BASE_DB and update it to the current schema. Then, for each test case, we # create another unique database, using the base database as a template. USE_POSTGRES_FOR_TESTS = os.environ.get("SYNAPSE_POSTGRES", False) -LEAVE_DB = os.environ.get("SYNAPSE_LEAVE_DB", False) POSTGRES_USER = os.environ.get("SYNAPSE_POSTGRES_USER", None) POSTGRES_HOST = os.environ.get("SYNAPSE_POSTGRES_HOST", None) POSTGRES_PASSWORD = os.environ.get("SYNAPSE_POSTGRES_PASSWORD", None) From 36b83195ba65ca4808830eac651b41e0845203a7 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 21 Dec 2021 15:41:22 +0000 Subject: [PATCH 7/7] Revert "Move LEAVE_DB across to tests.server" This reverts commit d5b64809f73ec9bb23dd8decd4627d4ad59cdd0e. --- tests/server.py | 5 +---- tests/utils.py | 1 + 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/server.py b/tests/server.py index 64c66ef9c71c..ca2b7a5b9771 100644 --- a/tests/server.py +++ b/tests/server.py @@ -14,7 +14,6 @@ import hashlib import json import logging -import os import time import uuid import warnings @@ -67,6 +66,7 @@ from synapse.util import Clock from tests.utils import ( + LEAVE_DB, POSTGRES_BASE_DB, POSTGRES_HOST, POSTGRES_PASSWORD, @@ -79,9 +79,6 @@ logger = logging.getLogger(__name__) -LEAVE_DB = os.environ.get("SYNAPSE_LEAVE_DB", False) - - class TimedOutException(Exception): """ A web query timed out. diff --git a/tests/utils.py b/tests/utils.py index 552b98c13da9..6d013e851845 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -36,6 +36,7 @@ # POSTGRES_BASE_DB and update it to the current schema. Then, for each test case, we # create another unique database, using the base database as a template. USE_POSTGRES_FOR_TESTS = os.environ.get("SYNAPSE_POSTGRES", False) +LEAVE_DB = os.environ.get("SYNAPSE_LEAVE_DB", False) POSTGRES_USER = os.environ.get("SYNAPSE_POSTGRES_USER", None) POSTGRES_HOST = os.environ.get("SYNAPSE_POSTGRES_HOST", None) POSTGRES_PASSWORD = os.environ.get("SYNAPSE_POSTGRES_PASSWORD", None)