Remove unnecessary self.pump calls in some of our tests.#19676
Remove unnecessary self.pump calls in some of our tests.#19676reivilibre wants to merge 3 commits intodevelopfrom
self.pump calls in some of our tests.#19676Conversation
|
|
||
| d2 = ensureDeferred(second_lookup()) | ||
|
|
||
| self.pump() |
There was a problem hiding this comment.
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.
| # Needed under Postgres | ||
| self.pump() |
There was a problem hiding this comment.
Could use with a better explanation if we understand. Or at-least some FIXME: Better explanation why?
| # 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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
ah ok derp, this test is skipped on SQLite, d'oh, that explains the difference. I will check the Postgres ones more carefully
| self.inject_room_member(self.room, self.u_bob, Membership.JOIN) | ||
| self.inject_room_member(self.room, self.u_charlie.to_string(), Membership.JOIN) | ||
|
|
||
| self.pump() |
There was a problem hiding this comment.
This one could be plausible given we _count_known_servers updates in a 60 second loop, https://github.com/matrix-org/synapse/blob/be65a8ec0195955c15fdb179c9158b187638e39a/synapse/storage/databases/main/roommember.py#L98-L101
We are testing that it's still empty after a count has been run. There should be a comment though.
| self.push_attempts[0][2]["notification"]["content"]["body"], "Message 1" | ||
| ) | ||
| self.push_attempts[0][0].callback({}) | ||
| self.pump() |
There was a problem hiding this comment.
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)
| shorthand=False, | ||
| await_result=False, | ||
| ) | ||
| self.pump() |
There was a problem hiding this comment.
I see we're removing the pump below. Do we know why this one is necessary?
|
|
||
| test_d = defer.ensureDeferred(self.cl.get_json("testserv:8008", "foo/bar")) | ||
|
|
||
| self.pump() |
There was a problem hiding this comment.
Seems like we should be able to remove this one 🤔
Are we expecting to hit the timeout or exercise the retry features?
2 participants
Spawning from some discussion on #19602
There are a good handful of
self.pumpcalls that are not necessary for the test to complete.This PR removes the ones that a basic dirty search & replace script found.
After this, I have a
self.advancehelper I intend to PR up that is a wrapper forself.reactor.advancewith assertions that there is actually progress to be made.