Skip to content

Commit 4424fb0

Browse files
hughnshughns
committed
Incorporate review feedback
1 parent b6e1929 commit 4424fb0

File tree

2 files changed

+48
-55
lines changed

2 files changed

+48
-55
lines changed

‎internal/client/client.go

Lines changed: 16 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -266,14 +266,14 @@ func (c *CSAPI) SetPushRule(t *testing.T, scope string, kind string, ruleID stri
266266
func (c *CSAPI) SendEventUnsynced(t *testing.T, roomID string, e b.Event) string {
267267
t.Helper()
268268
txnID := int(atomic.AddInt64(&c.txnID, 1))
269-
return c.SendEventUnsyncedWithTxnID(t, roomID, e, txnID)
269+
return c.SendEventUnsyncedWithTxnID(t, roomID, e, strconv.Itoa(txnID))
270270
}
271271

272272
// SendEventUnsynced sends `e` into the room.
273273
// Returns the event ID of the sent event.
274-
func (c *CSAPI) SendEventUnsyncedWithTxnID(t *testing.T, roomID string, e b.Event, txnID int) string {
274+
func (c *CSAPI) SendEventUnsyncedWithTxnID(t *testing.T, roomID string, e b.Event, txnID string) string {
275275
t.Helper()
276-
paths := []string{"_matrix", "client", "v3", "rooms", roomID, "send", e.Type, strconv.Itoa(txnID)}
276+
paths := []string{"_matrix", "client", "v3", "rooms", roomID, "send", e.Type, txnID}
277277
if e.StateKey != nil {
278278
paths = []string{"_matrix", "client", "v3", "rooms", roomID, "state", e.Type, *e.StateKey}
279279
}
@@ -421,8 +421,16 @@ func (c *CSAPI) MustSyncUntil(t *testing.T, syncReq SyncReq, checks ...SyncCheck
421421
}
422422
}
423423

424+
type LoginOpt func(map[string]interface{})
425+
426+
func WithDeviceID(deviceID string) LoginOpt {
427+
return func(loginBody map[string]interface{}) {
428+
loginBody["device_id"] = deviceID
429+
}
430+
}
431+
424432
// LoginUser will log in to a homeserver and create a new device on an existing user.
425-
func (c *CSAPI) LoginUser(t *testing.T, localpart, password string) (userID, accessToken, deviceID string) {
433+
func (c *CSAPI) LoginUser(t *testing.T, localpart, password string, opts ...LoginOpt) (userID, accessToken, deviceID string) {
426434
t.Helper()
427435
reqBody := map[string]interface{}{
428436
"identifier": map[string]interface{}{
@@ -432,31 +440,11 @@ func (c *CSAPI) LoginUser(t *testing.T, localpart, password string) (userID, acc
432440
"password": password,
433441
"type": "m.login.password",
434442
}
435-
res := c.MustDoFunc(t, "POST", []string{"_matrix", "client", "v3", "login"}, WithJSONBody(t, reqBody))
436443

437-
body, err := ioutil.ReadAll(res.Body)
438-
if err != nil {
439-
t.Fatalf("unable to read response body: %v", err)
444+
for _, opt := range opts {
445+
opt(reqBody)
440446
}
441447

442-
userID = gjson.GetBytes(body, "user_id").Str
443-
accessToken = gjson.GetBytes(body, "access_token").Str
444-
deviceID = gjson.GetBytes(body, "device_id").Str
445-
return userID, accessToken, deviceID
446-
}
447-
448-
// LoginUserWithDeviceID will log in to a homeserver on an existing device
449-
func (c *CSAPI) LoginUserWithDeviceID(t *testing.T, localpart, password, deviceID string) (userID, accessToken string) {
450-
t.Helper()
451-
reqBody := map[string]interface{}{
452-
"identifier": map[string]interface{}{
453-
"type": "m.id.user",
454-
"user": localpart,
455-
},
456-
"device_id": deviceID,
457-
"password": password,
458-
"type": "m.login.password",
459-
}
460448
res := c.MustDoFunc(t, "POST", []string{"_matrix", "client", "v3", "login"}, WithJSONBody(t, reqBody))
461449

462450
body, err := ioutil.ReadAll(res.Body)
@@ -466,10 +454,8 @@ func (c *CSAPI) LoginUserWithDeviceID(t *testing.T, localpart, password, deviceI
466454

467455
userID = gjson.GetBytes(body, "user_id").Str
468456
accessToken = gjson.GetBytes(body, "access_token").Str
469-
if gjson.GetBytes(body, "device_id").Str != deviceID {
470-
t.Fatalf("device_id returned by login does not match the one requested")
471-
}
472-
return userID, accessToken
457+
deviceID = gjson.GetBytes(body, "device_id").Str
458+
return userID, accessToken, deviceID
473459
}
474460

475461
// RegisterUser will register the user with given parameters and

‎tests/csapi/txnid_test.go

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55

66
"github.com/matrix-org/complement/internal/b"
77
"github.com/matrix-org/complement/internal/client"
8+
"github.com/matrix-org/complement/internal/must"
89
"github.com/matrix-org/complement/runtime"
910
"github.com/tidwall/gjson"
1011
)
@@ -23,30 +24,43 @@ func TestTxnInEvent(t *testing.T) {
2324
// Create a room where we can send events.
2425
roomID := c.CreateRoom(t, map[string]interface{}{})
2526

27+
txnId := "abcdefg"
2628
// Let's send an event, and wait for it to appear in the timeline.
27-
eventID := c.SendEventSynced(t, roomID, b.Event{
29+
eventID := c.SendEventUnsyncedWithTxnID(t, roomID, b.Event{
2830
Type: "m.room.message",
2931
Content: map[string]interface{}{
3032
"msgtype": "m.text",
3133
"body": "first",
3234
},
33-
})
35+
}, txnId)
3436

3537
// The transaction ID should be present on the GET /rooms/{roomID}/event/{eventID} response.
3638
res := c.MustDoFunc(t, "GET", []string{"_matrix", "client", "v3", "rooms", roomID, "event", eventID})
3739
body := client.ParseJSON(t, res)
3840
result := gjson.ParseBytes(body)
3941
if !result.Get("unsigned.transaction_id").Exists() {
40-
t.Fatalf("Event did not have a 'transaction_id' on the GET /rooms/%s/event/%s response", roomID, eventID)
42+
t.Fatalf("Event did not have a 'unsigned.transaction_id' on the GET /rooms/%s/event/%s response", roomID, eventID)
43+
}
44+
45+
txnIdFromResult:= result.Get("unsigned.transaction_id").Str
46+
47+
if txnIdFromResult != txnId {
48+
t.Fatalf("Event did not have a 'unsigned.transaction_id' of %s on GET /rooms/%s/event/%s response. Found %s instead", txnId, eventID, roomID, txnIdFromResult)
4149
}
4250
}
4351

4452

45-
func mustHaveTransactionID(t *testing.T, roomID, eventID string) client.SyncCheckOpt {
53+
func mustHaveTransactionID(t *testing.T, roomID, eventID, expectedTxnId string) client.SyncCheckOpt {
4654
return client.SyncTimelineHas(roomID, func(r gjson.Result) bool {
4755
if r.Get("event_id").Str == eventID {
4856
if !r.Get("unsigned.transaction_id").Exists() {
49-
t.Fatalf("Event %s in room %s should have a 'transaction_id', but it did not", eventID, roomID)
57+
t.Fatalf("Event %s in room %s should have a 'unsigned.transaction_id', but it did not", eventID, roomID)
58+
}
59+
60+
txnIdFromSync := r.Get("unsigned.transaction_id").Str
61+
62+
if txnIdFromSync != expectedTxnId {
63+
t.Fatalf("Event %s in room %s should have a 'unsigned.transaction_id' of %s but found %s", eventID, roomID, expectedTxnId, txnIdFromSync)
5064
}
5165

5266
return true
@@ -61,7 +75,7 @@ func mustNotHaveTransactionID(t *testing.T, roomID, eventID string) client.SyncC
6175
if r.Get("event_id").Str == eventID {
6276
res := r.Get("unsigned.transaction_id")
6377
if res.Exists() {
64-
t.Fatalf("Event %s in room %s should NOT have a 'transaction_id', but it did (%s)", eventID, roomID, res.Str)
78+
t.Fatalf("Event %s in room %s should NOT have a 'unsigned.transaction_id', but it did (%s)", eventID, roomID, res.Str)
6579
}
6680

6781
return true
@@ -89,21 +103,22 @@ func TestTxnScopeOnLocalEcho(t *testing.T) {
89103
// Create a room where we can send events.
90104
roomID := c1.CreateRoom(t, map[string]interface{}{})
91105

106+
txnId := "abdefgh"
92107
// Let's send an event, and wait for it to appear in the timeline.
93-
eventID := c1.SendEventUnsynced(t, roomID, b.Event{
108+
eventID := c1.SendEventUnsyncedWithTxnID(t, roomID, b.Event{
94109
Type: "m.room.message",
95110
Content: map[string]interface{}{
96111
"msgtype": "m.text",
97112
"body": "first",
98113
},
99-
})
114+
}, txnId)
100115

101116
// When syncing, we should find the event and it should have a transaction ID on the first client.
102-
c1.MustSyncUntil(t, client.SyncReq{}, mustHaveTransactionID(t, roomID, eventID))
117+
c1.MustSyncUntil(t, client.SyncReq{}, mustHaveTransactionID(t, roomID, eventID, txnId))
103118

104119
// Create a second client, inheriting the first device ID.
105120
c2 := deployment.Client(t, "hs1", "")
106-
c2.UserID, c2.AccessToken = c2.LoginUserWithDeviceID(t, "alice", "password", c1.DeviceID)
121+
c2.UserID, c2.AccessToken, _ = c2.LoginUser(t, "alice", "password", client.WithDeviceID(c1.DeviceID))
107122
c2.DeviceID = c1.DeviceID
108123

109124
// When syncing, we should find the event and it should *not* have a transaction ID on the second client.
@@ -128,7 +143,7 @@ func TestTxnIdempotencyScopedToClientSession(t *testing.T) {
128143
// Create a room where we can send events.
129144
roomID := c1.CreateRoom(t, map[string]interface{}{})
130145

131-
txnId := 1
146+
txnId := "abcdef"
132147
event := b.Event{
133148
Type: "m.room.message",
134149
Content: map[string]interface{}{
@@ -141,16 +156,14 @@ func TestTxnIdempotencyScopedToClientSession(t *testing.T) {
141156

142157
// Create a second client, inheriting the first device ID.
143158
c2 := deployment.Client(t, "hs1", "")
144-
c2.UserID, c2.AccessToken = c2.LoginUserWithDeviceID(t, "alice", "password", c1.DeviceID)
159+
c2.UserID, c2.AccessToken, _ = c2.LoginUser(t, "alice", "password", client.WithDeviceID(c1.DeviceID))
145160
c2.DeviceID = c1.DeviceID
146161

147162
// send another event with the same txnId
148163
eventID2 := c2.SendEventUnsyncedWithTxnID(t, roomID, event, txnId)
149164

150165
// the two events should have different event IDs as they came from different clients
151-
if eventID1 == eventID2 {
152-
t.Fatalf("Expected event IDs to be different from two clients sharing the same device ID")
153-
}
166+
must.NotEqualStr(t, eventID2, eventID1, "Expected eventID1 and eventID2 to be different from two clients sharing the same device ID")
154167
}
155168

156169
// TestTxnIdempotency tests that PUT requests idempotency follows required semantics
@@ -169,7 +182,7 @@ func TestTxnIdempotency(t *testing.T) {
169182
roomID2 := c1.CreateRoom(t, map[string]interface{}{})
170183

171184
// choose a transaction ID
172-
txnId := 1
185+
txnId := "abc"
173186
event1 := b.Event{
174187
Type: "m.room.message",
175188
Content: map[string]interface{}{
@@ -191,21 +204,15 @@ func TestTxnIdempotency(t *testing.T) {
191204
// we send the identical event again and should get back the same event ID
192205
eventID2 := c1.SendEventUnsyncedWithTxnID(t, roomID1, event1, txnId)
193206

194-
if eventID1 != eventID2 {
195-
t.Fatalf("Expected event IDs to be the same, but they were not")
196-
}
207+
must.EqualStr(t, eventID2, eventID1, "Expected eventID1 and eventID2 to be the same, but they were not")
197208

198209
// even if we change the content we should still get back the same event ID as transaction ID is the same
199210
eventID3 := c1.SendEventUnsyncedWithTxnID(t, roomID1, event2, txnId)
200211

201-
if eventID1 != eventID3 {
202-
t.Fatalf("Expected event IDs to be the same even with different content, but they were not")
203-
}
212+
must.EqualStr(t, eventID3, eventID1, "Expected eventID3 and eventID2 to be the same even with different content, but they were not")
204213

205214
// if we change the room ID we should be able to use the same transaction ID
206215
eventID4 := c1.SendEventUnsyncedWithTxnID(t, roomID2, event1, txnId)
207216

208-
if eventID4 == eventID3 {
209-
t.Fatalf("Expected event IDs to be the different, but they were not")
210-
}
217+
must.NotEqualStr(t, eventID4, eventID3, "Expected eventID4 and eventID3 to be different, but they were not")
211218
}

0 commit comments

Comments
 (0)