From f540411bc07ce8cb5bd093bdfcb6f642d02e6502 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 14 Sep 2022 14:20:44 +0200 Subject: [PATCH 1/9] Improve the `synapse.api.auth.Auth` mock used in unit tests. Signed-off-by: Quentin Gliech --- changelog.d/13809.misc | 1 + tests/unittest.py | 27 ++++++++++----------------- 2 files changed, 11 insertions(+), 17 deletions(-) create mode 100644 changelog.d/13809.misc diff --git a/changelog.d/13809.misc b/changelog.d/13809.misc new file mode 100644 index 000000000000..c2dacca2f25e --- /dev/null +++ b/changelog.d/13809.misc @@ -0,0 +1 @@ +Improve the `synapse.api.auth.Auth` mock used in unit tests. diff --git a/tests/unittest.py b/tests/unittest.py index 975b0a23a7b7..455192fdd193 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -300,46 +300,39 @@ def setUp(self) -> None: if hasattr(self, "user_id"): if self.hijack_auth: assert self.helper.auth_user_id is not None + token = "some_fake_token" # We need a valid token ID to satisfy foreign key constraints. token_id = self.get_success( self.hs.get_datastores().main.add_access_token_to_user( self.helper.auth_user_id, - "some_fake_token", + token, None, None, ) ) async def get_user_by_access_token( - token: Optional[str] = None, allow_guest: bool = False - ) -> JsonDict: + token: str, allow_guest: bool = False + ) -> Requester: assert self.helper.auth_user_id is not None - return { - "user": UserID.from_string(self.helper.auth_user_id), - "token_id": token_id, - "is_guest": False, - } + return create_requester( + user_id=UserID.from_string(self.helper.auth_user_id), + access_token_id=token_id, + ) async def get_user_by_req( request: SynapseRequest, allow_guest: bool = False, allow_expired: bool = False, ) -> Requester: - assert self.helper.auth_user_id is not None - return create_requester( - UserID.from_string(self.helper.auth_user_id), - token_id, - False, - False, - None, - ) + return await get_user_by_access_token(token) # Type ignore: mypy doesn't like us assigning to methods. self.hs.get_auth().get_user_by_req = get_user_by_req # type: ignore[assignment] self.hs.get_auth().get_user_by_access_token = get_user_by_access_token # type: ignore[assignment] self.hs.get_auth().get_access_token_from_request = Mock( # type: ignore[assignment] - return_value="1234" + return_value=token ) if self.needs_threadpool: From 53a1b6cd0565ceb59e9037410f46115b61285f7e Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 16 Sep 2022 12:06:26 +0200 Subject: [PATCH 2/9] Patch `Auth` using `unittest.mock.patch` instead Signed-off-by: Quentin Gliech --- tests/unittest.py | 47 +++++++++++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 455192fdd193..f5db4eccd83f 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -35,7 +35,7 @@ TypeVar, Union, ) -from unittest.mock import Mock, patch +from unittest.mock import patch import canonicaljson import signedjson.key @@ -68,7 +68,7 @@ from synapse.rest import RegisterServletsFunc from synapse.server import HomeServer from synapse.storage.keys import FetchKeyResult -from synapse.types import JsonDict, Requester, UserID, create_requester +from synapse.types import JsonDict, UserID, create_requester from synapse.util import Clock from synapse.util.httpresourcetree import create_resource_tree @@ -312,27 +312,30 @@ def setUp(self) -> None: ) ) - async def get_user_by_access_token( - token: str, allow_guest: bool = False - ) -> Requester: - assert self.helper.auth_user_id is not None - return create_requester( - user_id=UserID.from_string(self.helper.auth_user_id), - access_token_id=token_id, - ) + requester = create_requester( + user_id=UserID.from_string(self.helper.auth_user_id), + access_token_id=token_id, + ) + + patch.object( + self.hs.get_auth(), + "get_user_by_req", + return_value=requester, + autospec=True, + ) + + patch.object( + self.hs.get_auth(), + "get_user_by_access_token", + return_value=requester, + autospec=True, + ) - async def get_user_by_req( - request: SynapseRequest, - allow_guest: bool = False, - allow_expired: bool = False, - ) -> Requester: - return await get_user_by_access_token(token) - - # Type ignore: mypy doesn't like us assigning to methods. - self.hs.get_auth().get_user_by_req = get_user_by_req # type: ignore[assignment] - self.hs.get_auth().get_user_by_access_token = get_user_by_access_token # type: ignore[assignment] - self.hs.get_auth().get_access_token_from_request = Mock( # type: ignore[assignment] - return_value=token + patch.object( + self.hs.get_auth(), + "get_access_token_from_request", + return_value=token, + autospec=True, ) if self.needs_threadpool: From 0c398320dc0aee90e1107ce95eb7147908b8b8c5 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 16 Sep 2022 14:03:20 +0200 Subject: [PATCH 3/9] Attempt to fix the mock on older Python versions Signed-off-by: Quentin Gliech --- tests/unittest.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unittest.py b/tests/unittest.py index f5db4eccd83f..2ea8355a08ca 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -35,7 +35,7 @@ TypeVar, Union, ) -from unittest.mock import patch +from unittest.mock import AsyncMock, patch import canonicaljson import signedjson.key @@ -322,6 +322,7 @@ def setUp(self) -> None: "get_user_by_req", return_value=requester, autospec=True, + new_callable=AsyncMock, ) patch.object( @@ -329,6 +330,7 @@ def setUp(self) -> None: "get_user_by_access_token", return_value=requester, autospec=True, + new_callable=AsyncMock, ) patch.object( From e4c1522f041db7ba8e949d834f71b0ccfab5b851 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 16 Sep 2022 14:08:50 +0200 Subject: [PATCH 4/9] Second attempt at fixing the mock on older Python versions Signed-off-by: Quentin Gliech --- tests/unittest.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 2ea8355a08ca..0199ba93aeb2 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -13,6 +13,7 @@ # 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 asyncio import gc import hashlib import hmac @@ -35,7 +36,7 @@ TypeVar, Union, ) -from unittest.mock import AsyncMock, patch +from unittest.mock import MagicMock, patch import canonicaljson import signedjson.key @@ -317,20 +318,23 @@ def setUp(self) -> None: access_token_id=token_id, ) + requester_future = asyncio.Future() + requester_future.set_result(requester) + patch.object( self.hs.get_auth(), "get_user_by_req", - return_value=requester, + return_value=requester_future, autospec=True, - new_callable=AsyncMock, + new_callable=MagicMock, ) patch.object( self.hs.get_auth(), "get_user_by_access_token", - return_value=requester, + return_value=requester_future, autospec=True, - new_callable=AsyncMock, + new_callable=MagicMock, ) patch.object( From a626bdbe75125fc7bf7fc8ae82816c862674dd7e Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 16 Sep 2022 14:15:16 +0200 Subject: [PATCH 5/9] Use `make_awaitable` instead of creating the Future manually Signed-off-by: Quentin Gliech --- tests/unittest.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 0199ba93aeb2..a5dff09a81f4 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -13,7 +13,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. -import asyncio import gc import hashlib import hmac @@ -80,7 +79,7 @@ make_request, setup_test_homeserver, ) -from tests.test_utils import event_injection, setup_awaitable_errors +from tests.test_utils import event_injection, make_awaitable, setup_awaitable_errors from tests.test_utils.logging_setup import setup_logging from tests.utils import default_config, setupdb @@ -318,13 +317,10 @@ def setUp(self) -> None: access_token_id=token_id, ) - requester_future = asyncio.Future() - requester_future.set_result(requester) - patch.object( self.hs.get_auth(), "get_user_by_req", - return_value=requester_future, + return_value=make_awaitable(requester), autospec=True, new_callable=MagicMock, ) @@ -332,7 +328,7 @@ def setUp(self) -> None: patch.object( self.hs.get_auth(), "get_user_by_access_token", - return_value=requester_future, + return_value=make_awaitable(requester), autospec=True, new_callable=MagicMock, ) From d69e4e563a8f1d462ea146dda21b54f2a085660b Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Mon, 19 Sep 2022 15:20:52 +0200 Subject: [PATCH 6/9] Fix tests where the authed user ID gets overridden Signed-off-by: Quentin Gliech --- tests/unittest.py | 36 ++++++++++++++---------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index a5dff09a81f4..081189c7cf98 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -35,7 +35,7 @@ TypeVar, Union, ) -from unittest.mock import MagicMock, patch +from unittest.mock import patch import canonicaljson import signedjson.key @@ -68,7 +68,7 @@ from synapse.rest import RegisterServletsFunc from synapse.server import HomeServer from synapse.storage.keys import FetchKeyResult -from synapse.types import JsonDict, UserID, create_requester +from synapse.types import JsonDict, Requester, UserID, create_requester from synapse.util import Clock from synapse.util.httpresourcetree import create_resource_tree @@ -79,7 +79,7 @@ make_request, setup_test_homeserver, ) -from tests.test_utils import event_injection, make_awaitable, setup_awaitable_errors +from tests.test_utils import event_injection, setup_awaitable_errors from tests.test_utils.logging_setup import setup_logging from tests.utils import default_config, setupdb @@ -312,26 +312,18 @@ def setUp(self) -> None: ) ) - requester = create_requester( - user_id=UserID.from_string(self.helper.auth_user_id), - access_token_id=token_id, - ) - - patch.object( - self.hs.get_auth(), - "get_user_by_req", - return_value=make_awaitable(requester), - autospec=True, - new_callable=MagicMock, - ) + # This has to be a function and not just a Mock, because + # `self.helper.auth_user_id` is temporarily reassigned in some tests + async def get_requester(*args, **kwargs) -> Requester: + assert self.helper.auth_user_id is not None + return create_requester( + user_id=UserID.from_string(self.helper.auth_user_id), + access_token_id=token_id, + ) - patch.object( - self.hs.get_auth(), - "get_user_by_access_token", - return_value=make_awaitable(requester), - autospec=True, - new_callable=MagicMock, - ) + # Type ignore: mypy doesn't like us assigning to methods. + self.hs.get_auth().get_user_by_req = get_requester # type: ignore[assignment] + self.hs.get_auth().get_user_by_access_token = get_requester # type: ignore[assignment] patch.object( self.hs.get_auth(), From 3f1cd16e5676db0785d8a04790b21144f7f0a886 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 21 Sep 2022 11:27:42 +0200 Subject: [PATCH 7/9] Try to fix tests on old python Signed-off-by: Quentin Gliech --- tests/unittest.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 081189c7cf98..00cb023198b5 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -35,7 +35,7 @@ TypeVar, Union, ) -from unittest.mock import patch +from unittest.mock import Mock, patch import canonicaljson import signedjson.key @@ -324,13 +324,7 @@ async def get_requester(*args, **kwargs) -> Requester: # Type ignore: mypy doesn't like us assigning to methods. self.hs.get_auth().get_user_by_req = get_requester # type: ignore[assignment] self.hs.get_auth().get_user_by_access_token = get_requester # type: ignore[assignment] - - patch.object( - self.hs.get_auth(), - "get_access_token_from_request", - return_value=token, - autospec=True, - ) + self.hs.get_auth().get_access_token_from_request = Mock(return_value=token) # type: ignore[assignment] if self.needs_threadpool: self.reactor.threadpool = ThreadPool() # type: ignore[assignment] From 4219250a2dd9d9832eca825db0fd3482d57402a1 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 21 Sep 2022 12:17:28 +0200 Subject: [PATCH 8/9] Use patch.object instead of assignments Signed-off-by: Quentin Gliech --- tests/unittest.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index 00cb023198b5..b06d007be24b 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -321,10 +321,12 @@ async def get_requester(*args, **kwargs) -> Requester: access_token_id=token_id, ) - # Type ignore: mypy doesn't like us assigning to methods. - self.hs.get_auth().get_user_by_req = get_requester # type: ignore[assignment] - self.hs.get_auth().get_user_by_access_token = get_requester # type: ignore[assignment] - self.hs.get_auth().get_access_token_from_request = Mock(return_value=token) # type: ignore[assignment] + get_access_token = Mock(return_value=token) + + auth = self.hs.get_auth() + patch.object(auth, "get_user_by_req", get_requester) + patch.object(auth, "get_user_by_access_token", get_requester) + patch.object(auth, "get_access_token_from_request", get_access_token) if self.needs_threadpool: self.reactor.threadpool = ThreadPool() # type: ignore[assignment] From caab31fb921cc593238cd9f893e50d37d07e5efd Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Wed, 21 Sep 2022 14:03:28 +0200 Subject: [PATCH 9/9] Rollback to using assignments instead of patch.object --- tests/unittest.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/tests/unittest.py b/tests/unittest.py index b06d007be24b..00cb023198b5 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -321,12 +321,10 @@ async def get_requester(*args, **kwargs) -> Requester: access_token_id=token_id, ) - get_access_token = Mock(return_value=token) - - auth = self.hs.get_auth() - patch.object(auth, "get_user_by_req", get_requester) - patch.object(auth, "get_user_by_access_token", get_requester) - patch.object(auth, "get_access_token_from_request", get_access_token) + # Type ignore: mypy doesn't like us assigning to methods. + self.hs.get_auth().get_user_by_req = get_requester # type: ignore[assignment] + self.hs.get_auth().get_user_by_access_token = get_requester # type: ignore[assignment] + self.hs.get_auth().get_access_token_from_request = Mock(return_value=token) # type: ignore[assignment] if self.needs_threadpool: self.reactor.threadpool = ThreadPool() # type: ignore[assignment]