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

Add missing type hints in tests #14879

Merged
merged 5 commits into from
Jan 26, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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: 0 additions & 1 deletion mypy.ini
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ exclude = (?x)
|tests/http/federation/test_srv_resolver.py
|tests/http/test_proxyagent.py
|tests/module_api/test_api.py
|tests/rest/client/test_transactions.py
|tests/rest/media/v1/test_media_storage.py
|tests/server.py
|tests/server_notices/test_resource_limits_server_notices.py
Expand Down
3 changes: 2 additions & 1 deletion synapse/rest/client/transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

from typing_extensions import ParamSpec

from twisted.internet.defer import Deferred
from twisted.python.failure import Failure
from twisted.web.server import Request

Expand Down Expand Up @@ -90,7 +91,7 @@ def fetch_or_execute(
fn: Callable[P, Awaitable[Tuple[int, JsonDict]]],
*args: P.args,
**kwargs: P.kwargs,
) -> Awaitable[Tuple[int, JsonDict]]:
) -> "Deferred[Tuple[int, JsonDict]]":
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
"""Fetches the response for this transaction, or executes the given function
to produce a response for this transaction.

Expand Down
42 changes: 28 additions & 14 deletions tests/rest/client/test_transactions.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,22 @@
# limitations under the License.

from http import HTTPStatus
from typing import Any, Generator, Tuple, cast
from unittest.mock import Mock, call

from twisted.internet import defer, reactor
from twisted.internet import defer, reactor as _reactor

from synapse.logging.context import SENTINEL_CONTEXT, LoggingContext, current_context
from synapse.rest.client.transactions import CLEANUP_PERIOD_MS, HttpTransactionCache
from synapse.types import ISynapseReactor, JsonDict
from synapse.util import Clock

from tests import unittest
from tests.test_utils import make_awaitable
from tests.utils import MockClock

reactor = cast(ISynapseReactor, _reactor)


class HttpTransactionCacheTestCase(unittest.TestCase):
def setUp(self) -> None:
Expand All @@ -34,11 +38,13 @@ def setUp(self) -> None:
self.hs.get_auth = Mock()
self.cache = HttpTransactionCache(self.hs)

self.mock_http_response = (HTTPStatus.OK, "GOOD JOB!")
self.mock_http_response = (HTTPStatus.OK, {"result": "GOOD JOB!"})
Comment on lines -37 to +41
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is just making the mock response match what we expect a RestServlet method to return?

Copy link
Member Author

Choose a reason for hiding this comment

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

fetch_or_execute requires an (int, JsonDict) I believe. So yeah it seemed easiest to just give a more accurate value.

self.mock_key = "foo"

@defer.inlineCallbacks
def test_executes_given_function(self):
def test_executes_given_function(
self,
) -> Generator["defer.Deferred[Any]", object, None]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this signature means that

  • res: Any?
  • no constraints on the type of the thing being yielded

That's ever so slightly sad, but I don't think there's a good way to get mypy to check this short of using proper async defs. (And I think that is a little awkward to do in tests?)

Copy link
Member Author

Choose a reason for hiding this comment

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

It unfortunately needs to match what deferred.inlineCallbacks wants, I think. And the type on that isn't super accurate IIRC?

cb = Mock(return_value=make_awaitable(self.mock_http_response))
res = yield self.cache.fetch_or_execute(
self.mock_key, cb, "some_arg", keyword="arg"
Expand All @@ -47,7 +53,9 @@ def test_executes_given_function(self):
self.assertEqual(res, self.mock_http_response)

@defer.inlineCallbacks
def test_deduplicates_based_on_key(self):
def test_deduplicates_based_on_key(
self,
) -> Generator["defer.Deferred[Any]", object, None]:
cb = Mock(return_value=make_awaitable(self.mock_http_response))
for i in range(3): # invoke multiple times
res = yield self.cache.fetch_or_execute(
Expand All @@ -58,18 +66,20 @@ def test_deduplicates_based_on_key(self):
cb.assert_called_once_with("some_arg", keyword="arg", changing_args=0)

@defer.inlineCallbacks
def test_logcontexts_with_async_result(self):
def test_logcontexts_with_async_result(
self,
) -> Generator["defer.Deferred[Any]", object, None]:
@defer.inlineCallbacks
def cb():
def cb() -> Generator["defer.Deferred[object]", object, Tuple[int, JsonDict]]:
yield Clock(reactor).sleep(0)
return "yay"
return 1, {}

@defer.inlineCallbacks
def test():
def test() -> Generator["defer.Deferred[Any]", object, None]:
with LoggingContext("c") as c1:
res = yield self.cache.fetch_or_execute(self.mock_key, cb)
self.assertIs(current_context(), c1)
self.assertEqual(res, "yay")
self.assertEqual(res, (1, {}))

# run the test twice in parallel
d = defer.gatherResults([test(), test()])
Expand All @@ -78,13 +88,15 @@ def test():
self.assertIs(current_context(), SENTINEL_CONTEXT)

@defer.inlineCallbacks
def test_does_not_cache_exceptions(self):
def test_does_not_cache_exceptions(
self,
) -> Generator["defer.Deferred[Any]", object, None]:
"""Checks that, if the callback throws an exception, it is called again
for the next request.
"""
called = [False]

def cb():
def cb() -> "defer.Deferred[Tuple[int, JsonDict]]":
if called[0]:
# return a valid result the second time
return defer.succeed(self.mock_http_response)
Expand All @@ -104,13 +116,15 @@ def cb():
self.assertIs(current_context(), test_context)

@defer.inlineCallbacks
def test_does_not_cache_failures(self):
def test_does_not_cache_failures(
self,
) -> Generator["defer.Deferred[Any]", object, None]:
"""Checks that, if the callback returns a failure, it is called again
for the next request.
"""
called = [False]

def cb():
def cb() -> "defer.Deferred[Tuple[int, JsonDict]]":
if called[0]:
# return a valid result the second time
return defer.succeed(self.mock_http_response)
Expand All @@ -130,7 +144,7 @@ def cb():
self.assertIs(current_context(), test_context)

@defer.inlineCallbacks
def test_cleans_up(self):
def test_cleans_up(self) -> Generator["defer.Deferred[Any]", object, None]:
cb = Mock(return_value=make_awaitable(self.mock_http_response))
yield self.cache.fetch_or_execute(self.mock_key, cb, "an arg")
# should NOT have cleaned up yet
Expand Down