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

Fix-up type hints for tests.push #14816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fdb189a
Type check push tests.
clokep Jan 10, 2023
09bf7c8
Require type hints.
clokep Jan 10, 2023
2cd31b3
Fix-up types for presentable names.
clokep Jan 10, 2023
d07ce92
Add missing type hints to test_email.
clokep Jan 11, 2023
4143705
Newsfragment
clokep Jan 11, 2023
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: 0 additions & 1 deletion mypy.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ exclude = (?x)
|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
5 changes: 4 additions & 1 deletion stubs/synapse/synapse_rust/push.pyi
View file
Open in desktop
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 •
edited
Loading

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.

16 changes: 7 additions & 9 deletions tests/push/test_push_rule_evaluator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from typing import Dict, Optional, Union
from typing import Dict, List, Optional, Union, cast

import frozendict

Expand All @@ -30,7 +30,7 @@
from synapse.server import HomeServer
from synapse.storage.databases.main.appservice import _make_exclusive_regex
from synapse.synapse_rust.push import PushRuleEvaluator
from synapse.types import JsonDict, UserID
from synapse.types import JsonDict, JsonMapping, UserID
from synapse.util import Clock

from tests import unittest
Expand All @@ -39,7 +39,7 @@

class PushRuleEvaluatorTestCase(unittest.TestCase):
def _get_evaluator(
self, content: JsonDict, related_events=None
self, content: JsonMapping, related_events=None
) -> PushRuleEvaluator:
event = FrozenEvent(
{
Expand All @@ -59,7 +59,7 @@ def _get_evaluator(
_flatten_dict(event),
room_member_count,
sender_power_level,
power_levels.get("notifications", {}),
cast(Dict[str, int], power_levels.get("notifications", {})),
{} if related_events is None else related_events,
True,
event.room_version.msc3931_push_features,
Expand All @@ -70,9 +70,7 @@ def test_display_name(self) -> None:
"""Check for a matching display name in the body of the event."""
evaluator = self._get_evaluator({"body": "foo bar baz"})

condition = {
"kind": "contains_display_name",
}
condition = {"kind": "contains_display_name"}

# Blank names are skipped.
self.assertFalse(evaluator.matches(condition, "@user:test", ""))
Expand All @@ -93,7 +91,7 @@ def test_display_name(self) -> None:
self.assertTrue(evaluator.matches(condition, "@user:test", "foo bar"))

def _assert_matches(
self, condition: JsonDict, content: JsonDict, msg: Optional[str] = None
self, condition: JsonDict, content: JsonMapping, msg: Optional[str] = None
) -> None:
evaluator = self._get_evaluator(content)
self.assertTrue(evaluator.matches(condition, "@user:test", "display_name"), msg)
Expand Down Expand Up @@ -287,7 +285,7 @@ def test_tweaks_for_actions(self) -> None:
This tests the behaviour of tweaks_for_actions.
"""

actions = [
actions: List[Union[Dict[str, str], str]] = [
{"set_tweak": "sound", "value": "default"},
{"set_tweak": "highlight"},
"notify",
Expand Down