Skip to content

Reproduce matrix-org/synapse#9768 - #237

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

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

Conversation

@DMRobertson

Copy link
Copy Markdown
Contributor

No description provided.

})

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

kegsay commented Nov 30, 2021

Copy link
Copy Markdown
Member

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
Copy Markdown
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

kegsay commented Dec 17, 2021

Copy link
Copy Markdown
Member

@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

kegsay commented May 11, 2022

Copy link
Copy Markdown
Member

@DMRobertson what is the status of this PR?

@DMRobertson

DMRobertson commented May 11, 2022

Copy link
Copy Markdown
Contributor Author

@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

kegsay commented Sep 20, 2022

Copy link
Copy Markdown
Member

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