-
Notifications
You must be signed in to change notification settings - Fork 62
MSC4354: Sticky Events #806
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
base: main
Are you sure you want to change the base?
Conversation
…ectly in the test itself due to lack of GMSL support
if e.StateKey != nil { | ||
paths = []string{"_matrix", "client", "v3", "rooms", roomID, "state", e.Type, *e.StateKey} | ||
} | ||
qps := url.Values{} |
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.
What does this stand for? query parameter xxx (string? strings?)
An LLM said "query parameters" which is probably the right answer ⏩
Maybe too opaque
"lists": map[string]any{ | ||
"any-key": map[string]any{ | ||
"timeline_limit": 10, | ||
"required_state": [][]string{{"*", "*"}}, |
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.
Might as well request no state since it's never used
"required_state": [][]string{{"*", "*"}}, | |
"required_state": [][]string{}, |
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.
Perhaps this is because we actually create sticky state events (see makeStickyEvent(...)
) but we don't ever look for it in required_state
so we can still get rid of this.
}, | ||
}, | ||
} | ||
qps := url.Values{"timeout": []string{"5000"}} |
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.
We could align with the same timeout we use for normal /sync
Line 160 in 091cbf5
"timeout": []string{"1000"}, |
qps := url.Values{"timeout": []string{"5000"}} | |
qps := url.Values{"timeout": []string{"1000"}} |
return respBody, newPos | ||
} | ||
|
||
// standardised response format for /sync and SSS |
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.
// standardised response format for /sync and SSS | |
// Normalized response format for /sync and SSS |
🤷
resp, nextSince = cli.MustSync(t, client.SyncReq{Since: since}) | ||
timeline = resp.Get("rooms.join." + client.GjsonEscape(roomID) + ".timeline.events").Array() | ||
sticky = resp.Get("rooms.join." + client.GjsonEscape(roomID) + ".msc4354_sticky.events").Array() | ||
// t.Logf("%s\b", resp.Raw) |
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.
Leftover debug log
Fine to leave it if you want
if hasStickyEvent(syncResp.timelineEvents) { | ||
ct.Fatalf(t, "timeline had the sticky event, is delayed events supported?") | ||
} | ||
must.Equal(t, len(syncResp.stickyEvents), 0, "events were in sticky events when they shouldn't have been") |
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.
must.Equal(t, len(syncResp.stickyEvents), 0, "events were in sticky events when they shouldn't have been") | |
must.Equal(t, len(syncResp.stickyEvents), 0, "events were listed in sticky events, is delayed events supported?") |
// wait for the sticky event to send | ||
time.Sleep(4 * time.Second) | ||
|
||
for i := 0; i < 25; i++ { |
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.
(explain why 25
, related to discussion above)
} | ||
} | ||
|
||
func TestUnsignedTTL(t *testing.T) { |
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.
Test description would be nice
We're just checking that unsigned.msc4354_sticky_duration_ttl_ms
shows up. Useful for clients because ...
|
||
syncResp := gatherSyncResults(t, bob, useSimplifiedSlidingSync, roomID, stopEventID) | ||
mustHaveStickyEventID(t, stickyEventIDNotInTimeline, syncResp.stickyEvents) | ||
mustHaveStickyEventID(t, stickyEventIDInTimeline, syncResp.stickyEvents) |
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.
For my own understanding, this is more like stickyEventIDThatWouldBeInTimelineIfHistoryVisibilityWasBroken
Or perhaps, stickyEventIDInTimeline
actually shows up in the timeline not just in the sticky list 🤔 - I'm guessing it's this.
⏩
// Test that newly joined users to history_visibility: joined rooms correctly see sticky events | ||
// in the `sticky` section. | ||
func TestStickyEventsIgnoreHistoryVisibility(t *testing.T) { |
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.
We seem to be missing some tests for some expired sticky events and whether they should show up
Synapse PR: element-hq/synapse#18968
MSC4354
Pull Request Checklist