From fd3a555c83c7ccedd44f1c9e571c10e8387c977d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 29 Nov 2021 16:38:19 +0000 Subject: [PATCH 1/7] Change SyncUntil* functions to return next_batch --- internal/client/client.go | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/internal/client/client.go b/internal/client/client.go index 04556088..9c8cdcb8 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -133,22 +133,28 @@ func (c *CSAPI) SendEventSynced(t *testing.T, roomID string, e b.Event) string { // - we see an event in the room for which the `check` function returns True // If the `check` function fails the test, the failing event will be automatically logged. // Will time out after CSAPI.SyncUntilTimeout. -func (c *CSAPI) SyncUntilTimelineHas(t *testing.T, roomID string, check func(gjson.Result) bool) { +// +// Returns the `next_batch` token from the last /sync response. This can be passed as +// `since` to sync from this point forward only. +func (c *CSAPI) SyncUntilTimelineHas(t *testing.T, roomID string, check func(gjson.Result) bool) string { t.Helper() - c.SyncUntil(t, "", "", "rooms.join."+GjsonEscape(roomID)+".timeline.events", check) + return c.SyncUntil(t, "", "", "rooms.join."+GjsonEscape(roomID)+".timeline.events", check) } // SyncUntilInvitedTo is a wrapper around SyncUntil. // It blocks and continually calls `/sync` until we've been invited to the given room. // Will time out after CSAPI.SyncUntilTimeout. -func (c *CSAPI) SyncUntilInvitedTo(t *testing.T, roomID string) { +// +// Returns the `next_batch` token from the last /sync response. This can be passed as +// `since` to sync from this point forward only. +func (c *CSAPI) SyncUntilInvitedTo(t *testing.T, roomID string) string { t.Helper() check := func(event gjson.Result) bool { return event.Get("type").Str == "m.room.member" && event.Get("content.membership").Str == "invite" && event.Get("state_key").Str == c.UserID } - c.SyncUntil(t, "", "", "rooms.invite."+GjsonEscape(roomID)+".invite_state.events", check) + return c.SyncUntil(t, "", "", "rooms.invite."+GjsonEscape(roomID)+".invite_state.events", check) } // SyncUntil blocks and continually calls /sync until @@ -157,7 +163,10 @@ func (c *CSAPI) SyncUntilInvitedTo(t *testing.T, roomID string) { // - some element in that array makes the `check` function return true. // If the `check` function fails the test, the failing event will be automatically logged. // Will time out after CSAPI.SyncUntilTimeout. -func (c *CSAPI) SyncUntil(t *testing.T, since, filter, key string, check func(gjson.Result) bool) { +// +// Returns the `next_batch` token from the last /sync response. This can be passed as +// `since` to sync from this point forward only. +func (c *CSAPI) SyncUntil(t *testing.T, since, filter, key string, check func(gjson.Result) bool) string { t.Helper() start := time.Now() checkCounter := 0 @@ -199,7 +208,7 @@ func (c *CSAPI) SyncUntil(t *testing.T, since, filter, key string, check func(gj for i, ev := range events { lastEvent = &events[i] if check(ev) { - return + return since } wasFailed = t.Failed() checkCounter++ From dc22aaa56bef699152b32479b4e4f888097909ad Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 29 Nov 2021 16:38:38 +0000 Subject: [PATCH 2/7] Reproduce matrix-org/synapse#9768 --- tests/csapi/direct_message_create_test.go | 56 +++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 tests/csapi/direct_message_create_test.go diff --git a/tests/csapi/direct_message_create_test.go b/tests/csapi/direct_message_create_test.go new file mode 100644 index 00000000..c74746d3 --- /dev/null +++ b/tests/csapi/direct_message_create_test.go @@ -0,0 +1,56 @@ +package csapi_tests + +import ( + "net/url" + "testing" + + "github.com/matrix-org/complement/internal/b" + "github.com/matrix-org/complement/internal/client" + "github.com/tidwall/gjson" +) + +func TestDirectMessageCreate(t *testing.T) { + deployment := Deploy(t, b.BlueprintOneToOneRoom) + defer deployment.Destroy(t) + alice := deployment.Client(t, "hs1", "@alice:hs1") + bob := deployment.Client(t, "hs1", "@bob:hs1") + + // Alice invites Bob to a room for direct messaging. + roomID := alice.CreateRoom(t, map[string]interface{}{ + "invite": []string{bob.UserID}, + "is_direct": true, + }) + + // Bob receives the invite and joins. + next_batch := bob.SyncUntilInvitedTo(t, roomID) + bob.JoinRoom(t, roomID, []string{"hs1"}) + + // Wait until Bob sees that he's joined. + isBobJoinEvent := func(ev gjson.Result) bool { + return ev.Get("type").Str == "m.room.member" && ev.Get("state_key").Str == bob.UserID && ev.Get("content").Get("membership").Str == "join" + } + bob.SyncUntilTimelineHas(t, roomID, isBobJoinEvent) + + // Repeat the sync, but in a form where we can inspect the output. + query := url.Values{ + "timeout": []string{"1000"}, + "since": []string{next_batch}, + } + + res := bob.MustDoFunc(t, "GET", []string{"_matrix", "client", "r0", "sync"}, client.WithQueries(query)) + body := client.ParseJSON(t, res) + eventRes := gjson.GetBytes(body, "rooms.join").Get(roomID).Get("timeline.events") + if !eventRes.IsArray() { + t.Fatal("events in timeline was not an array: ", eventRes.Raw) + } + events := eventRes.Array() + bobJoinEvents := 0 + for _, ev := range events { + if isBobJoinEvent(ev) { + bobJoinEvents += 1 + } + } + if bobJoinEvents != 1 { + t.Fatalf("Saw %d join events for Bob; expected 1", bobJoinEvents) + } +} From 502c195e3796b376ce3103e1d096d36417f06ddf Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 30 Nov 2021 12:05:53 +0000 Subject: [PATCH 3/7] camelCase --- tests/csapi/direct_message_create_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/csapi/direct_message_create_test.go b/tests/csapi/direct_message_create_test.go index c74746d3..68d911af 100644 --- a/tests/csapi/direct_message_create_test.go +++ b/tests/csapi/direct_message_create_test.go @@ -22,7 +22,7 @@ func TestDirectMessageCreate(t *testing.T) { }) // Bob receives the invite and joins. - next_batch := bob.SyncUntilInvitedTo(t, roomID) + nextBatch := bob.SyncUntilInvitedTo(t, roomID) bob.JoinRoom(t, roomID, []string{"hs1"}) // Wait until Bob sees that he's joined. @@ -34,7 +34,7 @@ func TestDirectMessageCreate(t *testing.T) { // Repeat the sync, but in a form where we can inspect the output. query := url.Values{ "timeout": []string{"1000"}, - "since": []string{next_batch}, + "since": []string{nextBatch}, } res := bob.MustDoFunc(t, "GET", []string{"_matrix", "client", "r0", "sync"}, client.WithQueries(query)) From 0df4990624e12842f9c68663ea6758614b9413f1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 30 Nov 2021 12:06:25 +0000 Subject: [PATCH 4/7] Test name and description --- tests/csapi/direct_message_create_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/csapi/direct_message_create_test.go b/tests/csapi/direct_message_create_test.go index 68d911af..dbb0b2dd 100644 --- a/tests/csapi/direct_message_create_test.go +++ b/tests/csapi/direct_message_create_test.go @@ -9,7 +9,17 @@ import ( "github.com/tidwall/gjson" ) -func TestDirectMessageCreate(t *testing.T) { +// Synapse has a long-standing bug +// https://github.com/matrix-org/synapse/issues/9768 +// where membership events are duplicated in the timeline +// returned by a call to `/sync` with a `since` parameter. +// (AFAICS the spec doesn't mandate that there are no such +// duplicates, but pretty much everyone expects things to +// work that way.) +// +// This test reproduces the duplicated membership event for +// Bob after Alice invites him to a 1-1 room. +func TestMembershipNotDuplicatedWhenJoiningDirectMessage(t *testing.T) { deployment := Deploy(t, b.BlueprintOneToOneRoom) defer deployment.Destroy(t) alice := deployment.Client(t, "hs1", "@alice:hs1") From 95641b748a6c85c610f8ccf565b6cb268bb5db79 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 30 Nov 2021 12:14:40 +0000 Subject: [PATCH 5/7] `goimports` --- tests/csapi/direct_message_create_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/csapi/direct_message_create_test.go b/tests/csapi/direct_message_create_test.go index dbb0b2dd..8e9da77e 100644 --- a/tests/csapi/direct_message_create_test.go +++ b/tests/csapi/direct_message_create_test.go @@ -4,9 +4,10 @@ import ( "net/url" "testing" + "github.com/tidwall/gjson" + "github.com/matrix-org/complement/internal/b" "github.com/matrix-org/complement/internal/client" - "github.com/tidwall/gjson" ) // Synapse has a long-standing bug From 71fbecd05ba6fc7fbadbe932b202fc833c5d9e63 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Tue, 30 Nov 2021 14:31:43 +0000 Subject: [PATCH 6/7] Use sentinel event and describe the test steps --- tests/csapi/direct_message_create_test.go | 57 +++++++++++++---------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/tests/csapi/direct_message_create_test.go b/tests/csapi/direct_message_create_test.go index 8e9da77e..25b76122 100644 --- a/tests/csapi/direct_message_create_test.go +++ b/tests/csapi/direct_message_create_test.go @@ -1,7 +1,6 @@ package csapi_tests import ( - "net/url" "testing" "github.com/tidwall/gjson" @@ -32,35 +31,43 @@ func TestMembershipNotDuplicatedWhenJoiningDirectMessage(t *testing.T) { "is_direct": true, }) - // Bob receives the invite and joins. + // Bob receives the invite and joins. Extract the + // `next_batch` from the response for use later. nextBatch := bob.SyncUntilInvitedTo(t, roomID) bob.JoinRoom(t, roomID, []string{"hs1"}) - // Wait until Bob sees that he's joined. - isBobJoinEvent := func(ev gjson.Result) bool { - return ev.Get("type").Str == "m.room.member" && ev.Get("state_key").Str == bob.UserID && ev.Get("content").Get("membership").Str == "join" - } - bob.SyncUntilTimelineHas(t, roomID, isBobJoinEvent) - - // Repeat the sync, but in a form where we can inspect the output. - query := url.Values{ - "timeout": []string{"1000"}, - "since": []string{nextBatch}, - } + // To reproduce the bug, we ought to sync until we see + // a duplicate membership event. But actually seeing + // that event should fail the test, so we shouldn't + // expect it to happen. To account for both cases, + // send a dummy sentinel event after we've joined. + SENTINEL_EVENT_TYPE := "com.example.dummy" + bob.SendEventSynced(t, roomID, b.Event{ + Type: SENTINEL_EVENT_TYPE, + Sender: bob.UserID, + Content: map[string]interface{}{}, + }) - res := bob.MustDoFunc(t, "GET", []string{"_matrix", "client", "r0", "sync"}, client.WithQueries(query)) - body := client.ParseJSON(t, res) - eventRes := gjson.GetBytes(body, "rooms.join").Get(roomID).Get("timeline.events") - if !eventRes.IsArray() { - t.Fatal("events in timeline was not an array: ", eventRes.Raw) - } - events := eventRes.Array() + // Replay a sync from just before we joined until we see + // the sentinel event. Count the number of Join events we + // see. bobJoinEvents := 0 - for _, ev := range events { - if isBobJoinEvent(ev) { - bobJoinEvents += 1 - } - } + bob.SyncUntil( + t, + nextBatch, + "", + "rooms.join."+client.GjsonEscape(roomID)+".timeline.events", + func(ev gjson.Result) bool { + if ev.Get("type").Str == "m.room.member" && + ev.Get("state_key").Str == bob.UserID && + ev.Get("content.membership").Str == "join" { + bobJoinEvents += 1 + } + + return ev.Get("type").Str == SENTINEL_EVENT_TYPE + }, + ) + if bobJoinEvents != 1 { t.Fatalf("Saw %d join events for Bob; expected 1", bobJoinEvents) } From 0e2100db0150785aed99a717afce81e11109a0f1 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 3 Dec 2021 15:24:30 +0000 Subject: [PATCH 7/7] Use a simpler blueprint, so there's one room id in logs --- tests/csapi/direct_message_create_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/csapi/direct_message_create_test.go b/tests/csapi/direct_message_create_test.go index 25b76122..3f1ff17d 100644 --- a/tests/csapi/direct_message_create_test.go +++ b/tests/csapi/direct_message_create_test.go @@ -20,10 +20,10 @@ import ( // This test reproduces the duplicated membership event for // Bob after Alice invites him to a 1-1 room. func TestMembershipNotDuplicatedWhenJoiningDirectMessage(t *testing.T) { - deployment := Deploy(t, b.BlueprintOneToOneRoom) + deployment := Deploy(t, b.BlueprintAlice) defer deployment.Destroy(t) alice := deployment.Client(t, "hs1", "@alice:hs1") - bob := deployment.Client(t, "hs1", "@bob:hs1") + bob := deployment.RegisterUser(t, "hs1", "bob", "complement_meets_min_password_req_bob") // Alice invites Bob to a room for direct messaging. roomID := alice.CreateRoom(t, map[string]interface{}{