-
Notifications
You must be signed in to change notification settings - Fork 405
Remove MockClock()
#18992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove MockClock()
#18992
Conversation
`Clock` is the wrapper utilities around the `reactor`
|
|
||
| class E2eRoomKeysHandlerTestCase(unittest.HomeserverTestCase): | ||
| def make_homeserver(self, reactor: MemoryReactor, clock: Clock) -> HomeServer: | ||
| return self.setup_test_homeserver(replication_layer=mock.Mock()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I was dealing with setup_test_homeserver so much, I noticed this unused mock
| extra_homeserver_attributes["clock"] = Clock(reactor, server_name=server_name) | ||
|
|
||
| if "clock" not in kwargs: | ||
| kwargs["clock"] = MockClock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactoring away MockClock
Making all of these arguments way more clear to follow.
| synapse.server.HomeServer | ||
| """ | ||
| kwargs = dict(kwargs) | ||
| kwargs.update(self._hs_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this more straight-forward by removing the self._hs_args and generic kwargs concept in favor of direct keyword-args.
cdc20d1 to
e400b69
Compare
| logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left-over from debugging?
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really care about getting rid of these? I always leave them around because it's annoying to re-add them later and our linting doesn't complain about them being unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging but happy to remove in a follow-up when you reply
|
Thanks for the review @anoadragon453 🐣 |
Remove
MockClock()Spawning from adding some logcontext debug logs in #18966 and since we're not logging at the
set_current_context(...)level (see reasoning there), this removes some usage ofset_current_context(...).Specifically,
MockClock.call_later(...)doesn't handle logcontexts correctly. It uses the calling logcontext as the callback context (wrong, as the logcontext could finish before the callback finishes) and it didn't reset back to the sentinel context before handing back to the reactor. It was like this since it was introduced 10+ years ago. Instead of fixing the implementation which would just be a copy of our normalClock, we can just removeMockClocksynapse/tests/utils.py
Lines 284 to 300 in 84e1d15
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.