Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add tests to verify that we can resolve member avatar/displayname state for historical messages (MSC2716) #206

Merged
merged 20 commits into from
Nov 11, 2021
Merged

Add tests to verify that we can resolve member avatar/displayname state for historical messages (MSC2716) #206

merged 20 commits into from
Nov 11, 2021

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Sep 21, 2021 •
edited
Loading

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

.map((event) => ({
	type: event.type,
	sender: event.sender,
	depth: event.depth,
	stream_ordering: event.stream_ordering,
	body: event.content.body,
	event_id: event.event_id,
}))

@MadLittleMods MadLittleMods marked this pull request as draft September 21, 2021 19:44
@MadLittleMods MadLittleMods changed the title Add tests to verify we can resolve member avatar/displayname state for historical messages (MSC2716) Draft: Add tests to verify we can resolve member avatar/displayname state for historical messages (MSC2716) Sep 21, 2021
@MadLittleMods MadLittleMods mentioned this pull request Sep 21, 2021
Merged
MadLittleMods
MadLittleMods commented Sep 21, 2021
View reviewed changes
tests/msc2716_test.go Outdated Show resolved Hide resolved
This was referenced Sep 21, 2021
Merged
Merged
@MadLittleMods MadLittleMods force-pushed the madlittlemods/msc2716-make-sure-member-state-is-resolved branch from 1a97a2d to 7dc7d5f Compare September 21, 2021 20:48
@MadLittleMods MadLittleMods changed the base branch from madlittlemods/2716-chunk-to-batch-rename to master September 21, 2021 20:49
@MadLittleMods MadLittleMods changed the title Draft: Add tests to verify we can resolve member avatar/displayname state for historical messages (MSC2716) Draft: Add tests to verify that we can resolve member avatar/displayname state for historical messages (MSC2716) Sep 21, 2021
MadLittleMods added a commit to matrix-org/synapse that referenced this pull request Oct 13, 2021
@MadLittleMods
Resolve and share state_groups for all historical events in batch (M…
35d6b91 
…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
MadLittleMods commented Oct 28, 2021
View reviewed changes
tests/msc2716_test.go
matcherJSONEventIDArrayInOrder("chunk",
expectedEventIDOrder,
relevantToScrollbackEventFilter,
),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made all of this logic re-usable into matcherJSONEventIDArrayInOrder which we also use validateBatchSendRes

MadLittleMods
MadLittleMods commented Oct 28, 2021
View reviewed changes
tests/msc2716_test.go Outdated Show resolved Hide resolved
@MadLittleMods MadLittleMods mentioned this pull request Oct 29, 2021
Merged
@MadLittleMods MadLittleMods changed the title Draft: Add tests to verify that we can resolve member avatar/displayname state for historical messages (MSC2716) Add tests to verify that we can resolve member avatar/displayname state for historical messages (MSC2716) Nov 3, 2021
@MadLittleMods MadLittleMods marked this pull request as ready for review November 3, 2021 08:11
kegsay
kegsay approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Broadly speaking LGTM. Most of my complaints are to do with naming as I constantly get confused as to the timings / orderings of events.

tests/msc2716_test.go Outdated Show resolved Hide resolved
tests/msc2716_test.go Outdated Show resolved Hide resolved
tests/msc2716_test.go Outdated Show resolved Hide resolved
tests/msc2716_test.go Outdated Show resolved Hide resolved
tests/msc2716_test.go Outdated Show resolved Hide resolved
MadLittleMods
MadLittleMods commented Nov 10, 2021
View reviewed changes
tests/msc2716_test.go Outdated Show resolved Hide resolved
@MadLittleMods
Merge branch 'master' into madlittlemods/msc2716-make-sure-member-sta…
9f1f471 
…te-is-resolved

Conflicts:
	tests/msc2716_test.go
MadLittleMods
MadLittleMods commented Nov 10, 2021
View reviewed changes
tests/msc2716_test.go Outdated Show resolved Hide resolved
@MadLittleMods
Fix homeserver typo
a9eca78
@MadLittleMods
Fix function naming to get relevant event debug strings
7c0e416 
> 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)
@MadLittleMods
More clear comments for validateBatchSendRes
76f184d 
 - #206 (comment)
 - #206 (comment)
@MadLittleMods
Better control flow to return early
94a379f 
See #206 (comment)
@MadLittleMods
Document function inputs and better name
d14fa07 
See #206 (comment)
@MadLittleMods
Use validateBatchSendRes for test merged in from master
6df5a53 
See #206 (comment)
@MadLittleMods
Copy link
Contributor Author

MadLittleMods commented Nov 11, 2021 •
edited
Loading

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!

@MadLittleMods MadLittleMods merged commit 581cb0b into master Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kegsay kegsay kegsay approved these changes

Assignees

@kegsay kegsay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MadLittleMods @kegsay