-
Notifications
You must be signed in to change notification settings - Fork 61
Test failover logic for MSC3083 #172
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
Conversation
tests/msc3083_test.go
Outdated
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 should probably be synced with alice.SyncUntilTimelineHas as well right?
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 there any reason this is just always synced? Seems like it would be an improvement.
tests/msc3083_test.go
Outdated
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.
Why don't you use setupRestrictedRoom in the previous test?
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.
The joiners need to be on the same server while the creator needs to be on a separate server. Unfortunately BlueprintFederationTwoLocalOneRemote has alice and bob on one server and charlie on another.
tests/msc3083_test.go
Outdated
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.
Shouldn't we wait for these to come down alice sync stream?
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.
It doesn't matter if alice sees them.
tests/msc3083_test.go
Outdated
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.
Fairly sure there won't be any guarantee that hs2 would've seen the join from hs3 because the join was between hs3 and hs1. Therefore, doing this will be racey.
0209430 to
231d6be
Compare
231d6be to
11ddf9c
Compare
|
I rebased this, sorry about that. Wanted to remove the commits from the previous PR and forgot there had been a review on this already. 🤦 |
anoadragon453
left a comment
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 looks pretty sensible to me now from a test coverage perspective. Just one minor comment on a previous discussion.
This tests what happens if a server is unable to authorize a room join (due to not being able to invite users or not being able to check the proper allowed rooms).
Corresponds to matrix-org/synapse#10447
Based on #145