Skip to content
Open
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/19676.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove unnecessary `self.pump` calls in some of our tests.
1 change: 0 additions & 1 deletion tests/crypto/test_keyring.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,6 @@ async def second_lookup() -> None:

d2 = ensureDeferred(second_lookup())

self.pump()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For reference, as far as I understand, these were found by commenting out the pump functionality and removing them from any tests that still passed.

# the second request should be pending, but the fetcher should not yet have been
# called
self.assertEqual(second_lookup_state[0], 1)
Expand Down
10 changes: 0 additions & 10 deletions tests/federation/test_complexity.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,6 @@ def test_join_too_large(self) -> None:
{"membership": "join"},
)

self.pump()

# The request failed with a SynapseError saying the resource limit was
# exceeded.
f = self.get_failure(d, SynapseError)
Expand Down Expand Up @@ -125,8 +123,6 @@ def test_join_too_large_admin(self) -> None:
{"membership": "join"},
)

self.pump()

# The request failed with a SynapseError saying the resource limit was
# exceeded.
f = self.get_failure(d, SynapseError)
Expand Down Expand Up @@ -170,8 +166,6 @@ async def get_current_state_event_counts(room_id: str) -> int:
{"membership": "join"},
)

self.pump()

# The request failed with a SynapseError saying the resource limit was
# exceeded.
f = self.get_failure(d, SynapseError)
Expand Down Expand Up @@ -221,8 +215,6 @@ def test_join_too_large_no_admin(self) -> None:
{"membership": "join"},
)

self.pump()

# The request failed with a SynapseError saying the resource limit was
# exceeded.
f = self.get_failure(d, SynapseError)
Expand Down Expand Up @@ -251,7 +243,5 @@ def test_join_too_large_admin(self) -> None:
{"membership": "join"},
)

self.pump()

# The request success since the user is an admin
self.get_success(d)
1 change: 0 additions & 1 deletion tests/federation/test_federation_catch_up.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ def test_catch_up_last_successful_stream_ordering_tracking(self) -> None:
)

self.helper.send(room, "wombats!", tok=u1_token)
self.pump()

lsso_1 = self.get_success(
self.hs.get_datastores().main.get_destination_last_successful_stream_ordering(
Expand Down
18 changes: 4 additions & 14 deletions tests/http/test_matrixfederationclient.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,6 @@ def do_request() -> Generator["Deferred[Any]", object, object]:
b"%s" % (len(res_json), res_json)
)

self.pump()

res = self.successResultOf(test_d)

# check the response is as expected
Expand Down Expand Up @@ -710,8 +708,6 @@ def test_json_error(self, return_value: bytes) -> None:
b"%s" % (len(return_value), return_value)
)

self.pump()

f = self.failureResultOf(test_d)
self.assertIsInstance(f.value, RequestSendFailed)

Expand Down Expand Up @@ -751,8 +747,6 @@ def test_too_big(self) -> None:
b"HTTP/1.1 200 OK\r\nServer: Fake\r\nContent-Type: application/json\r\n\r\n"
)

self.pump()

# should still be waiting
self.assertNoResult(test_d)

Expand Down Expand Up @@ -853,7 +847,7 @@ def test_proxy_requests_through_federation_sender_worker(self) -> None:
self.hs.get_federation_http_client().get_json("remoteserv:8008", "foo/bar")
)

# Pump the reactor so our deferred goes through the motions
# Needed under Postgres
self.pump()
Comment on lines +850 to 851

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could use with a better explanation if we understand. Or at-least some FIXME: Better explanation why?


# Make sure that the request was proxied through the `federation_sender` worker
Expand Down Expand Up @@ -905,9 +899,7 @@ def test_proxy_request_with_network_error_through_federation_sender_worker(
self.hs.get_federation_http_client().get_json("remoteserv:8008", "foo/bar")
)

# Pump the reactor so our deferred goes through the motions. We pump with 10
# seconds (0.1 * 100) so the `MatrixFederationHttpClient` runs out of retries
# and finally passes along the error response.
# Needed under Postgres
self.pump(0.1)

# Make sure that the request was proxied through the `federation_sender` worker
Expand Down Expand Up @@ -984,7 +976,7 @@ def test_proxy_requests_and_discards_hop_by_hop_headers(self) -> None:
)
)

# Pump the reactor so our deferred goes through the motions
# Needed under Postgres
self.pump()

# Make sure that the request was proxied through the `federation_sender` worker
Expand Down Expand Up @@ -1078,9 +1070,7 @@ def test_not_able_to_proxy_requests_through_federation_sender_worker_when_wrong_
self.hs.get_federation_http_client().get_json("remoteserv:8008", "foo/bar")
)

# Pump the reactor so our deferred goes through the motions. We pump with 10
# seconds (0.1 * 100) so the `MatrixFederationHttpClient` runs out of retries
# and finally passes along the error response.
# Needed under Postgres
self.pump(0.1)
Comment on lines -1081 to 1084

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was the previous comment a untrue? It sounds plausible. Introduced in matrix-org/synapse#15913

"deferred goes through the motions" is wrong but the retries explanation sounds accurate given we sleep in MatrixFederationHttpClient (sleep is based on clock.call_later)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah ok derp, this test is skipped on SQLite, d'oh, that explains the difference. I will check the Postgres ones more carefully


# Make sure that the request was *NOT* proxied through the `federation_sender`
Expand Down
7 changes: 0 additions & 7 deletions tests/http/test_simple_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,16 +46,13 @@ def test_dns_error(self) -> None:
If the DNS lookup returns an error, it will bubble up.
"""
d = defer.ensureDeferred(self.cl.get_json("http://testserv2:8008/foo/bar"))
self.pump()

f = self.failureResultOf(d)
self.assertIsInstance(f.value, DNSLookupError)

def test_client_connection_refused(self) -> None:
d = defer.ensureDeferred(self.cl.get_json("http://testserv:8008/foo/bar"))

self.pump()

# Nothing happened yet
self.assertNoResult(d)

Expand All @@ -79,8 +76,6 @@ def test_client_never_connect(self) -> None:
"""
d = defer.ensureDeferred(self.cl.get_json("http://testserv:8008/foo/bar"))

self.pump()

# Nothing happened yet
self.assertNoResult(d)

Expand All @@ -106,8 +101,6 @@ def test_client_connect_no_response(self) -> None:
"""
d = defer.ensureDeferred(self.cl.get_json("http://testserv:8008/foo/bar"))

self.pump()

# Nothing happened yet
self.assertNoResult(d)

Expand Down
2 changes: 0 additions & 2 deletions tests/media/test_media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -845,8 +845,6 @@ def test_unknown_v3_endpoint(self) -> None:
HttpResponseException(404, "NOT FOUND", unknown_endpoint)
)

self.pump()

# There should now be another request to the r0 URL.
self.assertEqual(len(self.fetches), 2)
self.assertEqual(self.fetches[1][1], "example.com")
Expand Down
37 changes: 0 additions & 37 deletions tests/push/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,9 +159,6 @@ def test_sends_http(self) -> None:
self.assertEqual(len(pushers), 1)
last_stream_ordering = pushers[0].last_stream_ordering

# Advance time a bit, so the pusher will register something has happened
self.pump()

# It hasn't succeeded yet, so the stream ordering shouldn't have moved
pushers = list(
self.get_success(
Expand All @@ -182,7 +179,6 @@ def test_sends_http(self) -> None:

# Make the push succeed
self.push_attempts[0][0].callback({})
self.pump()

# The stream ordering has increased
pushers = list(
Expand All @@ -205,7 +201,6 @@ def test_sends_http(self) -> None:

# Make the second push succeed
self.push_attempts[1][0].callback({})
self.pump()

# The stream ordering has increased, again
pushers = list(
Expand Down Expand Up @@ -284,12 +279,8 @@ def test_sends_high_priority_for_encrypted(self) -> None:
tok=other_access_token,
)

# Advance time a bit, so the pusher will register something has happened
self.pump()

# Make the push succeed
self.push_attempts[0][0].callback({})
self.pump()

# Check our push made it with high priority
self.assertEqual(len(self.push_attempts), 1)
Expand All @@ -308,7 +299,6 @@ def test_sends_high_priority_for_encrypted(self) -> None:

# Check no push notifications are sent regarding the membership changes
# (that would confuse the test)
self.pump()
self.assertEqual(len(self.push_attempts), 1)

# Send another encrypted event
Expand All @@ -330,8 +320,6 @@ def test_sends_high_priority_for_encrypted(self) -> None:
tok=other_access_token,
)

# Advance time a bit, so the pusher will register something has happened
self.pump()
self.assertEqual(len(self.push_attempts), 2)
self.assertEqual(
self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify"
Expand Down Expand Up @@ -385,12 +373,8 @@ def test_sends_high_priority_for_one_to_one_only(self) -> None:
# Send a message
self.helper.send(room, body="Hi!", tok=other_access_token)

# Advance time a bit, so the pusher will register something has happened
self.pump()

# Make the push succeed
self.push_attempts[0][0].callback({})
self.pump()

# Check our push made it with high priority — this is a one-to-one room
self.assertEqual(len(self.push_attempts), 1)
Expand All @@ -406,14 +390,11 @@ def test_sends_high_priority_for_one_to_one_only(self) -> None:

# Check no push notifications are sent regarding the membership changes
# (that would confuse the test)
self.pump()
self.assertEqual(len(self.push_attempts), 1)

# Send another event
self.helper.send(room, body="Welcome!", tok=other_access_token)

# Advance time a bit, so the pusher will register something has happened
self.pump()
self.assertEqual(len(self.push_attempts), 2)
self.assertEqual(
self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify"
Expand Down Expand Up @@ -472,12 +453,8 @@ def test_sends_high_priority_for_mention(self) -> None:
# Send a message
self.helper.send(room, body="Oh, user, hello!", tok=other_access_token)

# Advance time a bit, so the pusher will register something has happened
self.pump()

# Make the push succeed
self.push_attempts[0][0].callback({})
self.pump()

# Check our push made it with high priority
self.assertEqual(len(self.push_attempts), 1)
Expand All @@ -489,8 +466,6 @@ def test_sends_high_priority_for_mention(self) -> None:
# Send another event, this time with no mention
self.helper.send(room, body="Are you there?", tok=other_access_token)

# Advance time a bit, so the pusher will register something has happened
self.pump()
self.assertEqual(len(self.push_attempts), 2)
self.assertEqual(
self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify"
Expand Down Expand Up @@ -554,12 +529,8 @@ def test_sends_high_priority_for_atroom(self) -> None:
tok=other_access_token,
)

# Advance time a bit, so the pusher will register something has happened
self.pump()

# Make the push succeed
self.push_attempts[0][0].callback({})
self.pump()

# Check our push made it with high priority
self.assertEqual(len(self.push_attempts), 1)
Expand All @@ -573,8 +544,6 @@ def test_sends_high_priority_for_atroom(self) -> None:
room, body="@room the spider is gone", tok=yet_another_access_token
)

# Advance time a bit, so the pusher will register something has happened
self.pump()
self.assertEqual(len(self.push_attempts), 2)
self.assertEqual(
self.push_attempts[1][1], "http://example.com/_matrix/push/v1/notify"
Expand Down Expand Up @@ -703,7 +672,6 @@ def _test_push_unread_count(self) -> None:
self.helper.send(room_id, body="HELLO???", tok=other_access_token)

def _advance_time_and_make_push_succeed(self, expected_push_attempts: int) -> None:
self.pump()
self.push_attempts[expected_push_attempts - 1][0].callback({})

def _check_push_attempt(
Expand Down Expand Up @@ -1084,7 +1052,6 @@ def test_jitter(self) -> None:
index += 1

self.reactor.advance(1)
self.pump()

self.assertEqual(len(self.push_attempts), 11)

Expand Down Expand Up @@ -1150,7 +1117,6 @@ def test_msc4076_badge_count(
self.helper.send(room, body="Hi!", tok=other_access_token)

# Advance time a bit, so the pusher will register something has happened
self.pump()

# One push was attempted to be sent
self.assertEqual(len(self.push_attempts), 1)
Expand Down Expand Up @@ -1218,17 +1184,14 @@ def test_push_backoff(self) -> None:
self.push_attempts[0][2]["notification"]["content"]["body"], "Message 1"
)
self.push_attempts[0][0].callback({})
self.pump()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How does the push backoff stuff work? Seems plausible that we're dealing with schedule clock.call_later in this stuff.

We need to make sure these don't have any meaning regardless of whether the test passes without them.

(applies to this whole file)


# Send another message, this time it fails
self.helper.send(room, body="Message 2", tok=other_access_token)
self.assertEqual(len(self.push_attempts), 2)
self.push_attempts[1][0].errback(Exception("couldn't connect"))
self.pump()

# Sending yet another message doesn't trigger a push immediately
self.helper.send(room, body="Message 3", tok=other_access_token)
self.pump()
self.assertEqual(len(self.push_attempts), 2)

# .. but waiting for a bit will cause more pushes
Expand Down
Loading
Loading