Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix-up type hints for tests.push #14816

Merged
merged 5 commits into from
Jan 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14816.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add missing type hints.
5 changes: 1 addition & 4 deletions mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@ exclude = (?x)
|tests/logging/__init__.py
|tests/logging/test_terse_json.py
|tests/module_api/test_api.py
|tests/push/test_email.py
|tests/push/test_presentable_names.py
|tests/push/test_push_rule_evaluator.py
|tests/rest/client/test_transactions.py
|tests/rest/media/v1/test_media_storage.py
|tests/server.py
Expand Down Expand Up @@ -101,7 +98,7 @@ disallow_untyped_defs = True
[mypy-tests.metrics.*]
disallow_untyped_defs = True

[mypy-tests.push.test_bulk_push_rule_evaluator]
[mypy-tests.push.*]
disallow_untyped_defs = True

[mypy-tests.rest.*]
Expand Down
5 changes: 4 additions & 1 deletion stubs/synapse/synapse_rust/push.pyi
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Any, Collection, Dict, Mapping, Optional, Sequence, Tuple, Union
from typing import Any, Collection, Dict, Mapping, Optional, Sequence, Set, Tuple, Union

from synapse.types import JsonDict

Expand Down Expand Up @@ -54,3 +54,6 @@ class PushRuleEvaluator:
user_id: Optional[str],
display_name: Optional[str],
) -> Collection[Union[Mapping, str]]: ...
def matches(
self, condition: JsonDict, user_id: Optional[str], display_name: Optional[str]
) -> bool: ...
Comment on lines +57 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunate that this got out of sync. How did you notice this was missing?

(Typeshed has some scripting in their CI which checks that every (public) stub corresponds to a runtime-importable name. In principle we could probably do something similar---though it'd be overkill; mypy's disallow-untyped-calls would probably suffice.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was attempting to write some tests and was trying to double check the signature, Cmd+clicked in PyCharm, which brought me to the autogenerated Python code. Then I tried to find the stub and couldn't.

I think unignoring mypy from the test files might have picked it up? Not 100% sure on that though since I assume we use this in production code?

I do know adding an incorrect type here is used, so the stubs are being used somehow.

Copy link
Member Author

@clokep clokep Jan 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think unignoring mypy from the test files might have picked it up? Not 100% sure on that though since I assume we use this in production code?

This generated the following:

tests/push/test_push_rule_evaluator.py:78: error: "PushRuleEvaluator" has no attribute "matches"  [attr-defined]

It seems this is a test-only method, BulkPushRuleEvaluator calls run instead.

46 changes: 24 additions & 22 deletions tests/push/test_email.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,28 @@
# limitations under the License.
import email.message
import os
from typing import Dict, List, Sequence, Tuple
from typing import Any, Dict, List, Sequence, Tuple

import attr
import pkg_resources

from twisted.internet.defer import Deferred
from twisted.test.proto_helpers import MemoryReactor

import synapse.rest.admin
from synapse.api.errors import Codes, SynapseError
from synapse.rest.client import login, room
from synapse.server import HomeServer
from synapse.util import Clock

from tests.unittest import HomeserverTestCase


@attr.s
@attr.s(auto_attribs=True)
class _User:
"Helper wrapper for user ID and access token"
id = attr.ib()
token = attr.ib()
id: str
token: str


class EmailPusherTests(HomeserverTestCase):
Expand All @@ -41,10 +44,9 @@ class EmailPusherTests(HomeserverTestCase):
room.register_servlets,
login.register_servlets,
]
user_id = True
hijack_auth = False

def make_homeserver(self, reactor, clock):
def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:

config = self.default_config()
config["email"] = {
Expand Down Expand Up @@ -72,17 +74,17 @@ def make_homeserver(self, reactor, clock):
# List[Tuple[Deferred, args, kwargs]]
self.email_attempts: List[Tuple[Deferred, Sequence, Dict]] = []

def sendmail(*args, **kwargs):
def sendmail(*args: Any, **kwargs: Any) -> Deferred:
# This mocks out synapse.reactor.send_email._sendmail.
d = Deferred()
d: Deferred = Deferred()
self.email_attempts.append((d, args, kwargs))
return d

hs.get_send_email_handler()._sendmail = sendmail
hs.get_send_email_handler()._sendmail = sendmail # type: ignore[assignment]

return hs

def prepare(self, reactor, clock, hs):
def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
# Register the user who gets notified
self.user_id = self.register_user("user", "pass")
self.access_token = self.login("user", "pass")
Expand Down Expand Up @@ -129,7 +131,7 @@ def prepare(self, reactor, clock, hs):
self.auth_handler = hs.get_auth_handler()
self.store = hs.get_datastores().main

def test_need_validated_email(self):
def test_need_validated_email(self) -> None:
"""Test that we can only add an email pusher if the user has validated
their email.
"""
Expand All @@ -151,7 +153,7 @@ def test_need_validated_email(self):
self.assertEqual(400, cm.exception.code)
self.assertEqual(Codes.THREEPID_NOT_FOUND, cm.exception.errcode)

def test_simple_sends_email(self):
def test_simple_sends_email(self) -> None:
# Create a simple room with two users
room = self.helper.create_room_as(self.user_id, tok=self.access_token)
self.helper.invite(
Expand All @@ -171,7 +173,7 @@ def test_simple_sends_email(self):

self._check_for_mail()

def test_invite_sends_email(self):
def test_invite_sends_email(self) -> None:
# Create a room and invite the user to it
room = self.helper.create_room_as(self.others[0].id, tok=self.others[0].token)
self.helper.invite(
Expand All @@ -184,7 +186,7 @@ def test_invite_sends_email(self):
# We should get emailed about the invite
self._check_for_mail()

def test_invite_to_empty_room_sends_email(self):
def test_invite_to_empty_room_sends_email(self) -> None:
# Create a room and invite the user to it
room = self.helper.create_room_as(self.others[0].id, tok=self.others[0].token)
self.helper.invite(
Expand All @@ -200,7 +202,7 @@ def test_invite_to_empty_room_sends_email(self):
# We should get emailed about the invite
self._check_for_mail()

def test_multiple_members_email(self):
def test_multiple_members_email(self) -> None:
# We want to test multiple notifications, so we pause processing of push
# while we send messages.
self.pusher._pause_processing()
Expand All @@ -227,7 +229,7 @@ def test_multiple_members_email(self):
# We should get emailed about those messages
self._check_for_mail()

def test_multiple_rooms(self):
def test_multiple_rooms(self) -> None:
# We want to test multiple notifications from multiple rooms, so we pause
# processing of push while we send messages.
self.pusher._pause_processing()
Expand Down Expand Up @@ -257,7 +259,7 @@ def test_multiple_rooms(self):
# We should get emailed about those messages
self._check_for_mail()

def test_room_notifications_include_avatar(self):
def test_room_notifications_include_avatar(self) -> None:
# Create a room and set its avatar.
room = self.helper.create_room_as(self.user_id, tok=self.access_token)
self.helper.send_state(
Expand Down Expand Up @@ -290,7 +292,7 @@ def test_room_notifications_include_avatar(self):
)
self.assertIn("_matrix/media/v1/thumbnail/DUMMY_MEDIA_ID", html)

def test_empty_room(self):
def test_empty_room(self) -> None:
"""All users leaving a room shouldn't cause the pusher to break."""
# Create a simple room with two users
room = self.helper.create_room_as(self.user_id, tok=self.access_token)
Expand All @@ -309,7 +311,7 @@ def test_empty_room(self):
# We should get emailed about that message
self._check_for_mail()

def test_empty_room_multiple_messages(self):
def test_empty_room_multiple_messages(self) -> None:
"""All users leaving a room shouldn't cause the pusher to break."""
# Create a simple room with two users
room = self.helper.create_room_as(self.user_id, tok=self.access_token)
Expand All @@ -329,7 +331,7 @@ def test_empty_room_multiple_messages(self):
# We should get emailed about that message
self._check_for_mail()

def test_encrypted_message(self):
def test_encrypted_message(self) -> None:
room = self.helper.create_room_as(self.user_id, tok=self.access_token)
self.helper.invite(
room=room, src=self.user_id, tok=self.access_token, targ=self.others[0].id
Expand All @@ -342,7 +344,7 @@ def test_encrypted_message(self):
# We should get emailed about that message
self._check_for_mail()

def test_no_email_sent_after_removed(self):
def test_no_email_sent_after_removed(self) -> None:
# Create a simple room with two users
room = self.helper.create_room_as(self.user_id, tok=self.access_token)
self.helper.invite(
Expand Down Expand Up @@ -379,7 +381,7 @@ def test_no_email_sent_after_removed(self):
pushers = list(pushers)
self.assertEqual(len(pushers), 0)

def test_remove_unlinked_pushers_background_job(self):
def test_remove_unlinked_pushers_background_job(self) -> None:
"""Checks that all existing pushers associated with unlinked email addresses are removed
upon running the remove_deleted_email_pushers background update.
"""
Expand Down
2 changes: 1 addition & 1 deletion tests/push/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer:

m = Mock()

def post_json_get_json(url, body):
def post_json_get_json(url: str, body: JsonDict) -> Deferred:
d: Deferred = Deferred()
self.push_attempts.append((d, url, body))
return make_deferred_yieldable(d)
Expand Down
44 changes: 23 additions & 21 deletions tests/push/test_presentable_names.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from typing import Iterable, Optional, Tuple
from typing import Iterable, List, Optional, Tuple, cast

from synapse.api.constants import EventTypes, Membership
from synapse.api.room_versions import RoomVersions
from synapse.events import FrozenEvent
from synapse.events import EventBase, FrozenEvent
from synapse.push.presentable_names import calculate_room_name
from synapse.types import StateKey, StateMap

Expand Down Expand Up @@ -51,13 +51,15 @@ def __init__(self, events: Iterable[Tuple[StateKey, dict]]):
)

async def get_event(
self, event_id: StateKey, allow_none: bool = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: I'd've guessed that StateKey was an alias for str, not Tuple[str, str].

self, event_id: str, allow_none: bool = False
) -> Optional[FrozenEvent]:
assert allow_none, "Mock not configured for allow_none = False"

return self._events.get(event_id)
# Decode the state key from the event ID.
state_key = cast(Tuple[str, str], tuple(event_id.split("|", 1)))
return self._events.get(state_key)

async def get_events(self, event_ids: Iterable[StateKey]):
async def get_events(self, event_ids: Iterable[StateKey]) -> StateMap[EventBase]:
# This is cheating since it just returns all events.
return self._events

Expand All @@ -68,27 +70,27 @@ class PresentableNamesTestCase(unittest.HomeserverTestCase):

def _calculate_room_name(
self,
events: StateMap[dict],
events: Iterable[Tuple[Tuple[str, str], dict]],
user_id: str = "",
fallback_to_members: bool = True,
fallback_to_single_member: bool = True,
):
# This isn't 100% accurate, but works with MockDataStore.
room_state_ids = {k[0]: k[0] for k in events}
) -> Optional[str]:
# Encode the state key into the event ID.
room_state_ids = {k[0]: "|".join(k[0]) for k in events}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To check: this is now a map from (type, state_key) to f"{type}|{state_key}"? E.g. ("m.room.member", "a") -> "m.room.member|a".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. We could do more tracking and generate integer event IDs are something but that seemed like a faff.


return self.get_success(
calculate_room_name(
MockDataStore(events),
MockDataStore(events), # type: ignore[arg-type]
room_state_ids,
user_id or self.USER_ID,
fallback_to_members,
fallback_to_single_member,
)
)

def test_name(self):
def test_name(self) -> None:
"""A room name event should be used."""
events = [
events: List[Tuple[Tuple[str, str], dict]] = [
((EventTypes.Name, ""), {"name": "test-name"}),
]
self.assertEqual("test-name", self._calculate_room_name(events))
Expand All @@ -100,9 +102,9 @@ def test_name(self):
events = [((EventTypes.Name, ""), {"name": 1})]
self.assertEqual(1, self._calculate_room_name(events))

def test_canonical_alias(self):
def test_canonical_alias(self) -> None:
"""An canonical alias should be used."""
events = [
events: List[Tuple[Tuple[str, str], dict]] = [
((EventTypes.CanonicalAlias, ""), {"alias": "#test-name:test"}),
]
self.assertEqual("#test-name:test", self._calculate_room_name(events))
Expand All @@ -114,9 +116,9 @@ def test_canonical_alias(self):
events = [((EventTypes.CanonicalAlias, ""), {"alias": "test-name"})]
self.assertEqual("Empty Room", self._calculate_room_name(events))

def test_invite(self):
def test_invite(self) -> None:
"""An invite has special behaviour."""
events = [
events: List[Tuple[Tuple[str, str], dict]] = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.INVITE}),
((EventTypes.Member, self.OTHER_USER_ID), {"displayname": "Other User"}),
]
Expand All @@ -140,9 +142,9 @@ def test_invite(self):
]
self.assertEqual("Room Invite", self._calculate_room_name(events))

def test_no_members(self):
def test_no_members(self) -> None:
"""Behaviour of an empty room."""
events = []
events: List[Tuple[Tuple[str, str], dict]] = []
self.assertEqual("Empty Room", self._calculate_room_name(events))

# Note that events with invalid (or missing) membership are ignored.
Expand All @@ -152,7 +154,7 @@ def test_no_members(self):
]
self.assertEqual("Empty Room", self._calculate_room_name(events))

def test_no_other_members(self):
def test_no_other_members(self) -> None:
"""Behaviour of a room with no other members in it."""
events = [
(
Expand Down Expand Up @@ -185,7 +187,7 @@ def test_no_other_members(self):
self._calculate_room_name(events, user_id=self.OTHER_USER_ID),
)

def test_one_other_member(self):
def test_one_other_member(self) -> None:
"""Behaviour of a room with a single other member."""
events = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.JOIN}),
Expand All @@ -209,7 +211,7 @@ def test_one_other_member(self):
]
self.assertEqual("@user:test", self._calculate_room_name(events))

def test_other_members(self):
def test_other_members(self) -> None:
"""Behaviour of a room with multiple other members."""
# Two other members.
events = [
Expand Down
Loading