Add tests for MSC3771 / MSC3773: threaded receipts & notifications #496
Conversation
| func syncHasUnreadNotifs(roomID string, check func(gjson.Result, gjson.Result) bool) client.SyncCheckOpt { | ||
| return func(clientUserID string, topLevelSyncJSON gjson.Result) error { | ||
| unreadNotifications := topLevelSyncJSON.Get("rooms.join." + client.GjsonEscape(roomID) + ".unread_notifications") | ||
| unreadThreadNotifications := topLevelSyncJSON.Get("rooms.join." + client.GjsonEscape(roomID) + ".unread_thread_notifications") | ||
| if !unreadNotifications.Exists() { | ||
| return fmt.Errorf("syncHasUnreadNotifs(%s): missing notifications", roomID) | ||
| } | ||
| if check(unreadNotifications, unreadThreadNotifications) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("syncHasUnreadNotifs(%s): check function did not pass: %v / %v", roomID, unreadNotifications.Raw, unreadThreadNotifications.Raw) | ||
| } | ||
| } |
There was a problem hiding this comment.
We could craft much better errors if we changed the check function into a expectedMap and displayed a diff
(however to do this in Go)
syncHasUnreadNotifs(roomID, {
main: {
highlight_count: 2,
notification_count: 4
},
[firstEventID]: {
label: "firstEventIDThreadNotifications",
highlight_count: 1,
notification_count: 4
}
})syncHasUnreadNotifs for main: Expected 2 / 4 but actual was 3 / 4 - Format is (highlights / notifications)syncHasUnreadNotifs for firstEventIDThreadNotifications ($abcdef): Expected 1 / 2 but actual was 1 / 1 - Format is (highlights / notifications)
For comparison as a different, separate example, I made the errors for the MSC3030 tests a bit fancy to show the diff of what was expected vs actual in context of the room timeline so it's much easier to figure out what's going wrong, see #178
There was a problem hiding this comment.
I left this somewhat generic because:
- It seems to fit better how check functions are done with complement.
- I suspect we will eventually want to check multiple threads.
There was a problem hiding this comment.
Eric's already taken a look at this (thanks @MadLittleMods), but this is in the review queue so I also took a look.
I'd be happy to see this land as-is! I've left some thoughts and suggestions, but they're nitpicking and left to your discretion.
| // | ||
| // Notification counts and receipts are handled by bob. | ||
| func TestThreadedReceipts(t *testing.T) { | ||
| runtime.SkipIf(t, runtime.Dendrite) // not supported |
There was a problem hiding this comment.
Feels a bit icky to couple the test to the implementation, but I suppose we do this everywhere. Might be possible to do something cleaner with build tags, but that might be a faff too (would have to e.g. update Synapse CI and complement.sh...)
This is probably fine as it is.
There was a problem hiding this comment.
I think we only use build tags for unstable features?
Co-authored-by: David Robertson <davidr@element.io>
Related to #351.
Tests for the bug fixed in matrix-org/synapse#14140 specifically, but also a bit more general.