Add tests to verify that we can resolve member avatar/displayname state for historical messages (MSC2716)#206
Merged
MadLittleMods merged 20 commits intomasterfrom Nov 11, 2021
Conversation
MadLittleMods
commented
Sep 21, 2021
This was referenced Sep 21, 2021
1a97a2d to
7dc7d5f
Compare
While working on matrix-org/synapse#10948
MadLittleMods
added a commit
to matrix-org/synapse
that referenced
this pull request
Oct 13, 2021
…SC2716) (#10975) Resolve and share `state_groups` for all historical events in batch. This also helps for showing the appropriate avatar/displayname in Element and will work whenever `/messages` has one of the historical messages as the first message in the batch. This does have the flaw where if you just insert a single historical event somewhere, it probably won't resolve the state correctly from `/messages` or `/context` since it will grab a non historical event above or below with resolved state which never included the historical state back then. For the same reasions, this also does not work in Element between the transition from actual messages to historical messages. In the Gitter case, this isn't really a problem since all of the historical messages are in one big lump at the beginning of the room. For a future iteration, might be good to look at `/messages` and `/context` to additionally add the `state` for any historical messages in that batch. --- How are the `state_groups` shared? To illustrate the `state_group` sharing, see this example: **Before** (new `state_group` for every event 😬, very inefficient): ``` # Tests from matrix-org/complement#206 $ COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestBackfillingHistory/parallel/should_resolve_member_state_events_for_historical_events create_new_client_event m.room.member event=$_JXfwUDIWS6xKGG4SmZXjSFrizhARM7QblhATVWWUcA state_group=None create_new_client_event org.matrix.msc2716.insertion event=$1ZBfmBKEjg94d-vGYymKrVYeghwBOuGJ3wubU1-I9y0 state_group=9 create_new_client_event org.matrix.msc2716.insertion event=$Mq2JvRetTyclPuozRI682SAjYp3GqRuPc8_cH5-ezPY state_group=10 create_new_client_event m.room.message event=$MfmY4rBQkxrIp8jVwVMTJ4PKnxSigpG9E2cn7S0AtTo state_group=11 create_new_client_event m.room.message event=$uYOv6V8wiF7xHwOMt-60d1AoOIbqLgrDLz6ZIQDdWUI state_group=12 create_new_client_event m.room.message event=$PAbkJRMxb0bX4A6av463faiAhxkE3FEObM1xB4D0UG4 state_group=13 create_new_client_event org.matrix.msc2716.batch event=$Oy_S7AWN7rJQe_MYwGPEy6RtbYklrI-tAhmfiLrCaKI state_group=14 ``` **After** (all events in batch sharing `state_group=10`) (the base insertion event has `state_group=8` which matches the `prev_event` we're inserting next to): ``` # Tests from matrix-org/complement#206 $ COMPLEMENT_ALWAYS_PRINT_SERVER_LOGS=1 COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh TestBackfillingHistory/parallel/should_resolve_member_state_events_for_historical_events create_new_client_event m.room.member event=$PWomJ8PwENYEYuVNoG30gqtybuQQSZ55eldBUSs0i0U state_group=None create_new_client_event org.matrix.msc2716.insertion event=$e_mCU7Eah9ABF6nQU7lu4E1RxIWccNF05AKaTT5m3lw state_group=9 create_new_client_event org.matrix.msc2716.insertion event=$ui7A3_GdXIcJq0C8GpyrF8X7B3DTjMd_WGCjogax7xU state_group=10 create_new_client_event m.room.message event=$EnTIM5rEGVezQJiYl62uFBl6kJ7B-sMxWqe2D_4FX1I state_group=10 create_new_client_event m.room.message event=$LGx5jGONnBPuNhAuZqHeEoXChd9ryVkuTZatGisOPjk state_group=10 create_new_client_event m.room.message event=$wW0zwoN50lbLu1KoKbybVMxLbKUj7GV_olozIc5i3M0 state_group=10 create_new_client_event org.matrix.msc2716.batch event=$5ZB6dtzqFBCEuMRgpkU201Qhx3WtXZGTz_YgldL6JrQ state_group=10 ```
MadLittleMods
commented
Oct 28, 2021
MadLittleMods
commented
Oct 28, 2021
MadLittleMods
commented
Oct 28, 2021
| matcherJSONEventIDArrayInOrder("chunk", | ||
| expectedEventIDOrder, | ||
| relevantToScrollbackEventFilter, | ||
| ), |
Collaborator
Author
There was a problem hiding this comment.
Made all of this logic re-usable into matcherJSONEventIDArrayInOrder which we also use validateBatchSendRes
MadLittleMods
commented
Oct 28, 2021
MadLittleMods
commented
Oct 28, 2021
kegsay
approved these changes
Nov 10, 2021
Member
kegsay
left a comment
There was a problem hiding this comment.
Broadly speaking LGTM. Most of my complaints are to do with naming as I constantly get confused as to the timings / orderings of events.
MadLittleMods
commented
Nov 10, 2021
…te-is-resolved Conflicts: tests/msc2716_test.go
MadLittleMods
commented
Nov 10, 2021
> Naming: In cases where you have "do a thing" vs "do a thing and fail the test if there's an error" the general keyword to use here is `Must`. I would rename: > - `getRelevantEventDebugStringsFromMessagesResponse` -> `mustGetRelevantEventDebugStringsFromMessagesResponse` > - `_getRelevantEventDebugStringsFromMessagesResponse` -> `getRelevantEventDebugStringsFromMessagesResponse` > > -- #206 (comment)
Collaborator
Author
|
Thanks for the review @kegsay! 🐼 Added some more comments to make this more clear. I'm striving to make this digestible for future onlookers so your pokes there are very welcome! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add tests to verify that we can resolve member avatar/displayname state for historical messages (MSC2716)
Part of testing the batch importing script for Gitter https://gitlab.com/gitterHQ/webapp/-/merge_requests/2229#note_654645018
Dev notes