Use SyncLeftFrom to check charlie leave event synced to hs1#808
Use SyncLeftFrom to check charlie leave event synced to hs1#808MadLittleMods merged 3 commits intomatrix-org:mainfrom
Conversation
tests/restricted_rooms_test.go
Outdated
| }) | ||
| charlie.MustLeaveRoom(t, room) | ||
|
|
||
| time.Sleep(time.Second) |
There was a problem hiding this comment.
I don't think this should be necessary (and it seems strange to hardcode to 1 second), because MustSyncUntil will repeat the /sync several times until the condition becomes true.
tests/restricted_rooms_test.go
Outdated
|
|
||
| time.Sleep(time.Second) | ||
| // Ensure the events have synced to hs1. | ||
| alice.MustSyncUntil(t, client.SyncReq{}, client.SyncTimelineHas( |
There was a problem hiding this comment.
This is alice sync first time, if charlie leave event is not sync to alice's local server, Charelie join member event will be in timeline events list?
I notice Dendrite skipped this test, I am not sure why.
synapse can pass this test, Charelie join event is not exists in timeline events when alice sync first time. seems synapse marked Charelie join event as outlier on alice local server.
There was a problem hiding this comment.
I think you may be on the right track that there is some smoke here. We need to update this to keep checking for the Charlie membership to be leave. Currently, it checks until any Charlie membership exists.
The better fix here is to use client.SyncLeftFrom(...) instead of client.SyncTimelineHas(...) for this kind of thing.
There was a problem hiding this comment.
I created a PR to fix up more flawed membership checks in #813
chrislearn
changed the title
|
According to @MadLittleMods's suggestion, use |
Same fix as applied in #808
…813) *Spawning from #808 per my suggestion on #808 (comment) We have other spots that have flawed membership checks that need to be fixed. For example, when our goal is to wait for the user's membership to be `leave`, we should keep checking until it is `leave`. Currently, there are some spots that wait until *any* membership exists for the user and then asserts `leave` on that which is flawed because that user may have previous membership events that may be picked up first instead of waiting for the `leave`. This PR fixes those flawed checks and aligns our membership checks so we don't cargo cult this bad pattern elsewhere. - Generalize our `client.SyncXXX` helpers to use `syncMembershipIn` utility - More robust - Standardize extra `checks` on event (previously, only available with `client.SyncJoinedTo`) - Introduce `client.SyncBannedFrom` so we can differentiate ban/leave
3 participants
In the test related to TestRestrictedRoomsRemoteJoinLocalUser, I am not sure if my understanding is correct. Is it necessary to wait for a while to ensure that the leave event on hs2 are synchronized to hs1?
Signed-off-by: Chrislearn Young chris@acroidea.com
Pull Request Checklist