Skip to content

Fix race in MSC3083 TestRestrictedRoomsRemoteJoin#149

Merged
erikjohnston merged 3 commits into
masterfrom
erikj/fix_msc3083_test
Jul 7, 2021
Merged

Fix race in MSC3083 TestRestrictedRoomsRemoteJoin#149
erikjohnston merged 3 commits into
masterfrom
erikj/fix_msc3083_test

Conversation

@erikjohnston
Copy link
Copy Markdown
Member

This was broken by matrix-org/synapse#10272

@erikjohnston erikjohnston requested a review from a team July 5, 2021 15:31
Comment thread tests/msc3083_test.go
bob.LeaveRoom(t, room)
bob.LeaveRoom(t, space)

alice.SyncUntilTimelineHas(t, space, func(ev gjson.Result) bool {
Copy link
Copy Markdown
Member

@kegsay kegsay Jul 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if having LeaveRoom is a mistake, and we should make a LeaveRoomSynced to do this always.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, though the subtlety here is that we need to wait for it to be processed on the remote server, rather than the local one.

Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it kinda looks fine, but I don't quite follow why alice needs to see the leave for the test to continue?

Comment thread tests/msc3083_test.go
@erikjohnston
Copy link
Copy Markdown
Member Author

it kinda looks fine, but I don't quite follow why alice needs to see the leave for the test to continue?

I've added a comment, but basically Bob would leave the space and then try to join the original room again before the remote server saw him leave the space, so the remote server accepted the join when we were expecting it to fail.

Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment thread tests/msc3083_test.go Outdated
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants