tests: Refactor the widget tests to use MockMatrixServer

[toger5]

This is a follow up PR on: #3987
And tries to use the MockMatrixServer wherever reasonable in the widget integration tests.

• Public API changes documented in changelogs (optional)

Signed-off-by:

[codecov]

Codecov Report

Attention: Patch coverage is 78.94737% with 8 lines in your changes missing coverage. Please review.

Project coverage is 85.13%. Comparing base (d49d122) to head (46bdcd9).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/test_utils/mocks.rs	78.94%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4236      +/-   ##
==========================================
- Coverage   85.18%   85.13%   -0.05%     
==========================================
  Files         280      280              
  Lines       30729    30764      +35     
==========================================
+ Hits        26176    26192      +16     
- Misses       4553     4572      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[toger5]

How do i test /test_utils/mocks.rs?

[bnjbvr]

How do i test /test_utils/mocks.rs?

We usually don't test the test utilities; they should be straightforward enough that using them in tests would reveal their issues.

[poljar]

How do i test /test_utils/mocks.rs?

Examples in the docstrings will be run as the doc tests which can run and test the mocks. At least that's how I wrote all the examples for the MatrixMockServer.

[poljar]

Thanks for working on this. Left a couple of nits about linkifying doc references and a couple more examples would be neat.

[bnjbvr]

No, we should not use impl Into<Value>, otherwise we lose all the usefulness of these mocking APIs by reverting to "we don't know what the return value looks like, it can be an arbitrary JSON". Instead we should use high-level types. We have another place where we do mock messages, grep for fn mock_messages (…it's even better, we do have two implementations :D).

[bnjbvr]

Super nice, thanks. Feel free to address the last comments, or let me know if you're bored with this PR, and then I can do it myself as a followup :)

[bnjbvr]

I think here it would be better to generate the path regex at the end, just before building the final MatrixMock.

[toger5]

Where would that be?
There are numerous places we build the final MatrixMock. Do you want to implement all the methods from MockEndpoint<'a, T> in
MockEndpoint<'a, RoomSendStateEndpoint>. What if someone adds a new method to the generic version that can build a matrix mock but forgets to update this version to build the regex?

[bnjbvr]

You can take a look at RoomEventEndpoint as an example: it stores an optional room, set to None first:

matrix-rust-sdk/crates/matrix-sdk/src/test_utils/mocks.rs

Line 431 in 80a48f5

endpoint: RoomEventEndpoint { room: None, match_event_id: false },

Then, we have a builder method to optionally set it:

matrix-rust-sdk/crates/matrix-sdk/src/test_utils/mocks.rs

Lines 995 to 998 in 80a48f5

	pub fn room(mut self, room: impl Into<OwnedRoomId>) -> Self {
	self.endpoint.room = Some(room.into());
	self
	}

Then, we create the final path when we want to get the final MatrixMock:

matrix-rust-sdk/crates/matrix-sdk/src/test_utils/mocks.rs

Lines 1009 to 1023 in 80a48f5

	let event_path = if self.endpoint.match_event_id {
	let event_id = event.kind.event_id().expect("an event id is required");
	event_id.to_string()
	} else {
	// Event is at the end, so no need to add anything.
	"".to_owned()
	};
	let room_path = self.endpoint.room.map_or_else(|| ".*".to_owned(), |room| room.to_string());
	let mock = self
	.mock
	.and(path_regex(format!("^/_matrix/client/v3/rooms/{room_path}/event/{event_path}")))
	.respond_with(ResponseTemplate::new(200).set_body_json(event.into_raw().json()));
	MatrixMock { server: self.server, mock }

If this code were to be duplicated in multiple methods, then it could be factored out as an helper method, like generate_path_regexp does, in fact.

Does it make sense now?

[bnjbvr]

Yeah, same here: we should fill the state_key field in RoomSendStateEndpoint, but not make the matcher more precise there.

[bnjbvr]

note: doc comment for the ok_with_event_id is incorrect, as it duplicates the doc comment for mock_room_send without any change in the example code.

[toger5]

You mean the doc comment for ok?
I suppose this is still helpful to have for ok. What other test du you have in mind?

[bnjbvr]

Yes. The doc comment below is incorrect because it mentions mock_room_send while this is mock_room_send_state.

[bnjbvr]

We're getting there!

[bnjbvr]

Yes. The doc comment below is incorrect because it mentions mock_room_send while this is mock_room_send_state.

[bnjbvr]

You can take a look at RoomEventEndpoint as an example: it stores an optional room, set to None first:

matrix-rust-sdk/crates/matrix-sdk/src/test_utils/mocks.rs

Line 431 in 80a48f5

endpoint: RoomEventEndpoint { room: None, match_event_id: false },

Then, we have a builder method to optionally set it:

matrix-rust-sdk/crates/matrix-sdk/src/test_utils/mocks.rs

Lines 995 to 998 in 80a48f5

	pub fn room(mut self, room: impl Into<OwnedRoomId>) -> Self {
	self.endpoint.room = Some(room.into());
	self
	}

Then, we create the final path when we want to get the final MatrixMock:

matrix-rust-sdk/crates/matrix-sdk/src/test_utils/mocks.rs

Lines 1009 to 1023 in 80a48f5

	let event_path = if self.endpoint.match_event_id {
	let event_id = event.kind.event_id().expect("an event id is required");
	event_id.to_string()
	} else {
	// Event is at the end, so no need to add anything.
	"".to_owned()
	};
	let room_path = self.endpoint.room.map_or_else(|| ".*".to_owned(), |room| room.to_string());
	let mock = self
	.mock
	.and(path_regex(format!("^/_matrix/client/v3/rooms/{room_path}/event/{event_path}")))
	.respond_with(ResponseTemplate::new(200).set_body_json(event.into_raw().json()));
	MatrixMock { server: self.server, mock }

If this code were to be duplicated in multiple methods, then it could be factored out as an helper method, like generate_path_regexp does, in fact.

Does it make sense now?

[bnjbvr]

To respond to my own suggestion in #4236 (comment): this is not possible, because it hinders usage of error500() and error_too_large(), which never define the more restricted path. I've reverted to the previous code, with a code comment explaining that it's fine to redefine a path regex, since it ought to be more specialized than the previous ones.

[bnjbvr]

Thanks, let's revisit the approach of finishing to construct a path regexp, because it's a bit calling for misuse, but that one is on me 😇