Skip to content

Add test for non-member state events in /batch_send state_events_at_start#354

Merged
MadLittleMods merged 3 commits into
mainfrom
madlittlemods/synapse-12110-add-test-for-non-member-state
Apr 4, 2022
Merged

Add test for non-member state events in /batch_send state_events_at_start#354
MadLittleMods merged 3 commits into
mainfrom
madlittlemods/synapse-12110-add-test-for-non-member-state

Conversation

@MadLittleMods
Copy link
Copy Markdown
Collaborator

@MadLittleMods MadLittleMods commented Mar 30, 2022

Add test for non-member state events in /batch_send state_events_at_start

Tests for matrix-org/synapse#12110

Synapse changes: matrix-org/synapse#12329

Comment thread tests/msc2716_test.go
// historical batch
"limit": []string{fmt.Sprintf("%d", len(expectedEventIDOrder))},
// We add these options to the filter so we get member events in the state field
"filter": []string{"{\"lazy_load_members\":true,\"include_redundant_members\":true}"},
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

/messages was only returning member state events and only because we passed this special filter. Now that we're testing with non-member state events, we need an endpoint which will show us them.

We can instead simplify and just use /context directly 🎉

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.

Bear in mind that /context does not work over federation. That is, if server A is in room X and sends 100 messages, then server B joins room X and hits /context for an event early in the timeline (so not backfilled) it will 404.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I moved the validateState block (which calls /context) after we call /messages which will backfill any of the potential gaps 👍

MadLittleMods added a commit to matrix-org/synapse that referenced this pull request Mar 30, 2022
Part of #12110

Complement test: matrix-org/complement#354

Previously, they didn't resolve because async `filter_events_for_client`
removes all outlier state except for out-of-band membership.
@MadLittleMods MadLittleMods requested a review from kegsay March 30, 2022 17:08
Comment thread tests/msc2716_test.go Outdated
eventIdBefore := eventIDsBefore[0]
timeAfterEventBefore := time.Now()

state_events_at_start := createJoinStateEventsForBatchSendRequest([]string{virtualUserID}, timeAfterEventBefore)
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.

camelCase please.

Copy link
Copy Markdown
Collaborator Author

@MadLittleMods MadLittleMods Apr 1, 2022

Choose a reason for hiding this comment

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

Is there a linter/autoformatter to use to avoid these kinds of mistakes?

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.

There is, but I don't think it's built into CI. Added it to #290

Comment thread tests/msc2716_test.go
// historical batch
"limit": []string{fmt.Sprintf("%d", len(expectedEventIDOrder))},
// We add these options to the filter so we get member events in the state field
"filter": []string{"{\"lazy_load_members\":true,\"include_redundant_members\":true}"},
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.

Bear in mind that /context does not work over federation. That is, if server A is in room X and sends 100 messages, then server B joins room X and hits /context for an event early in the timeline (so not backfilled) it will 404.

Copy link
Copy Markdown
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

Merge whenever you want!

MadLittleMods added a commit to matrix-org/synapse that referenced this pull request Apr 4, 2022
…nts (MSC2716) (#12329)

Part of #12110

Complement test: matrix-org/complement#354

Previously, they didn't resolve because async `filter_events_for_client`
removes all outlier state except for out-of-band membership.

And fundamentally, we have the state at these events so they shouldn't be marked as outliers.
@MadLittleMods MadLittleMods merged commit 2180cc5 into main Apr 4, 2022
@MadLittleMods MadLittleMods deleted the madlittlemods/synapse-12110-add-test-for-non-member-state branch April 4, 2022 19:58
@MadLittleMods
Copy link
Copy Markdown
Collaborator Author

Thanks for the review @kegsay 🐬

Was waiting for matrix-org/synapse#12329 to merge 🚢

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