Skip to content

Reproduce matrix-org/synapse#9768#237

Closed
DMRobertson wants to merge 7 commits intomainfrom
dmr/duplicate-room-events
Closed

Reproduce matrix-org/synapse#9768#237
DMRobertson wants to merge 7 commits intomainfrom
dmr/duplicate-room-events

Conversation

@DMRobertson
Copy link
Contributor

No description provided.

})

// Bob receives the invite and joins.
next_batch := bob.SyncUntilInvitedTo(t, roomID)
Copy link
Member

Choose a reason for hiding this comment

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

Please use camelCase instead for variable names.

}
bob.SyncUntilTimelineHas(t, roomID, isBobJoinEvent)

// Repeat the sync, but in a form where we can inspect the output.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense because SyncUntilTimelineHas returns each event in the callback func(ev gjson.Result). You can do any inspection or assertion you like on the event, and indeed you already do on :30 to check the type/state_key/membership.

events := eventRes.Array()
bobJoinEvents := 0
for _, ev := range events {
if isBobJoinEvent(ev) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense, why are you asserting the same thing twice?

Copy link
Member

Choose a reason for hiding this comment

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

This makes more sense now that I know you're testing for duplicate events.

"github.com/tidwall/gjson"
)

func TestDirectMessageCreate(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Not a good test name. I had to read matrix-org/synapse#9768 to figure out what the regression test here is for. Please at the very least mention duplicate membership event somewhere.

@kegsay
Copy link
Member

kegsay commented Nov 30, 2021

The test file needs to be formatted with go fmt and it isn't. Please set up your IDE to auto-format on save.

In addition, it's not clear if this test case is race-free. We assume in the test currently that the duplicate event will appear immediately after the /sync returns the first event. It would be better if we instead sent a sentinel event (i.e some random other event in the same room) and then only treat this test as a success when that sentinel event is received down /sync, as that guarantees that the member event has been fully processed.

@DMRobertson
Copy link
Contributor Author

The test file needs to be formatted with go fmt and it isn't.

The required incantation was

goimports -local github.com/matrix-org/complement -w internal tests

Which I suggest we write down in ONBOARDING.md.

@kegsay
Copy link
Member

kegsay commented Dec 17, 2021

@DMRobertson please update this PR to use the new functional options for syncing as shown in #272 - you shouldn't need to modify client.go anymore now! :D

@kegsay
Copy link
Member

kegsay commented May 11, 2022

@DMRobertson what is the status of this PR?

@DMRobertson
Copy link
Contributor Author

DMRobertson commented May 11, 2022

@DMRobertson what is the status of this PR?

It's on my wishlist of things to get to if I find time. I'm not planning to actively work on the linked Synapse issue any time soon, though; feel free to close this PR if it helps manage the repo. (I can always reopen.)

@kegsay
Copy link
Member

kegsay commented Sep 20, 2022

Closing on the basis that you can re-open it later. This PR is just adding noise.

@kegsay kegsay closed this Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants