Conversation
This is to serve as regression tests for: - rejoining a room not working because we skipped seen events during processing - rejected events (due to cascading) being accepted because we didn't persist rejection status in-memory alongside the state group
devonh
left a comment
devonh
left a comment
There was a problem hiding this comment.
Love the very thorough test descriptions and diagrams. They are very helpful to understand the test intent.
I found just a couple things after puzzling my way through each test.
| state := currentRoomState(t, alice, room.RoomID) | ||
| // Both forks should be in the current state | ||
| mustHaveStateEventContent( | ||
| t, state, "m.room.topic", "", "topic", "fork containing rejected events", |
There was a problem hiding this comment.
| t, state, "m.room.topic", "", "topic", "fork containing rejected events", | |
| t, state, spec.MRoomTopic, "", "topic", "fork containing rejected events", |
|
|
||
| // The room works after the rejoin: an event sent by the remote server arrives. | ||
| msg := srv.MustCreateEvent(t, room, federation.Event{ | ||
| Type: "m.room.message", |
There was a problem hiding this comment.
| Type: "m.room.message", | |
| Type: spec.MRoomMember, |
| mustHaveStateEventContent( | ||
| t, state, spec.MRoomName, "", "name", "fork containing rejoin", | ||
| "current state not calculated correctly", | ||
| ) |
There was a problem hiding this comment.
Is this check deterministic?
Say a hs accepts both name and bobName, but when merging the two branches their ordering comes down to either origin_server_ts or event_id, origin_server_ts is likely to be identical, so ordering is likely based on event_id which is random. Such a hs will pass this test ~50% of the time.
The following test doesn't run into this because m.room.name is only ever set on the rejected branch, and it asserts mustNotHaveStateEvent.
Could this test be changed slightly to replace bobName with bobAvatar and set an empty avatar? Then assert that mustNotHaveStateEvent(t, state, spec.MRoomAvatar, "", ...)
The test should keep the current assertion that the room name has been set in that case.
| ) | ||
|
|
||
| // Alice leaves, wait for it to propagate. | ||
| alice.MustLeaveRoom(t, room.RoomID) |
There was a problem hiding this comment.
Are we certain that the leave event isn't pulling in the sideName here with it's prev_state_events?
It would be good to add an assertion that the DAG hasn't been prematurely merged at this point somehow.
Or maybe have bob's homeserver kick alice out of the room instead with manually set prev_events to just be alice's join?
2 participants
Spawning from the discussions in element-hq/synapse#20194
This is to serve as regression tests for:
This cargo cults some chunks of code from #841
Pull Request Checklist