-
Notifications
You must be signed in to change notification settings - Fork 246
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
task(tests): introduce prebuilt mocks and mocking helpers #4197
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4197 +/- ##
==========================================
+ Coverage 84.88% 84.94% +0.05%
==========================================
Files 272 273 +1
Lines 29131 29265 +134
==========================================
+ Hits 24729 24858 +129
- Misses 4402 4407 +5 ☔ View full report in Codecov by Sentry. |
2c8e654
to
e9016b2
Compare
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.
This is great. Passing on to @richvdh because a) I think he will have helpful feedback and b) I'm not available next week.
} | ||
|
||
impl MatrixMockServer { | ||
/// Create a new mocked server specialized for Matrix usage. |
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.
"mocked server" -> "wiremock server"
|
||
/// Creates a new [`MatrixMockServer`] when both parts have been already | ||
/// created. | ||
pub fn from_pair(server: MockServer, client: Client) -> Self { |
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.
from_pair
-> from_server_and_client
or from_parts
?
/// Overrides the sync/ endpoint with knowledge that the given | ||
/// invited/joined/knocked/left room exists, runs a sync and returns the | ||
/// given room. | ||
pub async fn sync_room(&self, room_id: &RoomId, room_data: impl Into<AnyRoomBuilder>) -> Room { |
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.
Trying to think of a more informative name for this...
Maybe create_or_change_room
? yuck.
self.sync_room(room_id, JoinedRoomBuilder::new(room_id)).await | ||
} | ||
|
||
pub async fn verify_and_reset(&self) { |
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.
I personally have no idea what these methods do, so would be nice to have a comment.
} | ||
|
||
/// A wiremock Mock wrapping also the server, to not have to pass it around when | ||
/// mounting. |
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.
I don't understand this. Can you find a different way to write it? I think maybe it's because I don't know much about wiremock.
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.
some thoughts. Looks like a great improvement.
use super::logged_in_client; | ||
use crate::{Client, Room}; | ||
|
||
pub struct MatrixMockServer { |
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.
I think it would be good to give this a doc-comment explaining its purpose and scope.
Possibly some of the words from your excellent PR description could stand to end up here, as examples.
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.
come to that, could you add documentation to all of the pub struct
s being added here?
} | ||
|
||
// Specific mount endpoints. | ||
impl MatrixMockServer { |
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.
is dividing the impl
into two bits like this idiomatic? It's confusing for me: I see this and think "hold on, if this is impl MatrixMockServer
then what's the other bit?", and repeat a few times to check I'm not missing something.
|
||
pub struct MatrixMockServer { | ||
server: MockServer, | ||
client: Client, |
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.
Including a client
in MatrixMockServer feels a bit awkward to me. WDYT about making the caller responsible for instantiating the client (possibly by having client()
instantiate a new Client) and, say, passing a client as an argument into MockSync::ok_and_run
?
This introduces new test utilities, starting with
MatrixMockServer
. This is a pair of aMockServer
and aClient
(one can retrieve them respectively with.server()
and.client()
).It implements a few mock endpoints, expanding and limited the shared code as much as possible, so the mocks are still flexible to use as scoped/unscoped mounts, named, and so on.
It works like this:
mock_room_send()
. This returns a specializedMockSomething
data structure, with its own impl. For this example, it'sMockRoomSend
.MockRoomSend::error500()
; if you want it to succeed and return the event $42, callMockRoomSend::ok(event_id!("$42"))
. It's still possible to callrespond_with()
, as we do with wiremock MockBuilder, for maximum flexibility when the helpers aren't sufficient.MatrixMock
; this is a plainwiremock::Mock
with the server curried, so one doesn't have to pass it around when calling.mount()
or.mount_as_scoped()
. As such, it mostly defers its implementations towiremock::Mock
under the hood.Example
With this, we can go from that code:
To that code: