Add tests for MSC4429: Profile Updates for Legacy Sync#849
Add tests for MSC4429: Profile Updates for Legacy Sync#849anoadragon453 wants to merge 9 commits into
Conversation
MadLittleMods
changed the title
| // No filter = no profile fields returned. | ||
| _, since := alice.MustSync(t, client.SyncReq{}) | ||
| mustSetProfileField(t, bob, "m.status", map[string]interface{}{ | ||
| "text": "away", | ||
| }) |
There was a problem hiding this comment.
As a sanity check, we should have another observer user that sees the m.status with a filter, and then alice syncs without a filter
There was a problem hiding this comment.
Addressed in fd4b5fd84fd325aa9c922ff70fe534ba0c1e7be4.
| // No filter = no profile fields returned. | ||
| _, since := alice.MustSync(t, client.SyncReq{}) |
There was a problem hiding this comment.
We don't actually assert anything for the initial sync request
There was a problem hiding this comment.
I believe this is just to get a since token, so we can perform an incremental sync afterwards.
Clarified the comments in 4b93b71a59ca431a1f10cefe714c44f2384055bc.
| assertNoProfileUpdate(t, res, bob.UserID, "m.status") | ||
| }) | ||
|
|
||
| t.Run("Incremental sync returns the latest update", func(t *testing.T) { |
There was a problem hiding this comment.
Perhaps we should have another test to make sure more subsequent incremental /sync include new updates for the new user.
This ensures that any caching (remembering which users were sent down before) is correct.
There was a problem hiding this comment.
I guess, we kinda have this already with "Cleared profile field is returned as null"
There was a problem hiding this comment.
I guess, we kinda have this already with "Cleared profile field is returned as null"
Sort of. The data structure of the /sync response JSON doesn't allow to return two subsequent status updates for the same user and field:
{
"users": {
"@user:example.org": {
"profile_updates": {
"m.status": {
"text": "Swimming in the Great Lakes!",
"emoji": "🏊️"
}
}
}
}
}What might be useful is an additional test with:
- Bob updating their
m.statusfield - Alice syncing without a sync filter which includes the field
- Bob updating their
displaynamefield - Alice syncing with a
m.statusanddisplaynamefilter - Alice should only receive the
displaynameupdate
Add various comments throughout for clarification.
|
The tests in this PR weren't actually running in CI 🤦 I've fixed that in 4cddfea. All were expected to succeed (despite all failing), other than the newly added Still, the tests here should be ready for another round of review. |
| repo: element-hq/synapse | ||
| tags: synapse_blacklist | ||
| packages: ./tests/msc3874 ./tests/msc3902 ./tests/msc4306 | ||
| packages: ./tests/msc3874 ./tests/msc3902 ./tests/msc4306 ./tests/msc4429 |
There was a problem hiding this comment.
The tests in this PR weren't actually running in CI 🤦 I've fixed that in 4cddfea.
I assume you meant 2b67137 but in any case, we don't really worry about updating this list.
It's already heavily out of sync with the list we run in the Synapse project, element-hq/synapse -> scripts-dev/complement.sh#L272-L289
At this point, I think we just rely on it as a general sanity check that Complement is still working with Synapse.
| // Assert that charlie receives bob's profile update in an incremental sync | ||
| // with the appropriate filter set. | ||
| filter := mustBuildMSC4429Filter(t, []string{"m.status"}) | ||
| charlie.MustSyncUntil(t, client.SyncReq{Filter: filter}, syncHasProfileUpdate(bob.UserID, "m.status", map[string]interface{}{ | ||
| "text": "away", | ||
| "emoji": "🟡", | ||
| })) |
There was a problem hiding this comment.
Kinda feels like this should also be an incremental sync to better demonstrate the data is there with the right filter.
|
|
||
| // syncHasProfileUpdatesNull checks whether a sync response contains a null | ||
| // profile_updates value for the given user. | ||
| func syncHasProfileUpdatesNull(userID string) client.SyncCheckOpt { |
There was a problem hiding this comment.
Seems like the usage could be replaced by syncHasProfileUpdate(...)
Or at-least this function re-uses syncHasProfileUpdate(...)
| // Join all of the given users to the room. | ||
| for _, user := range users { | ||
| user.MustSyncUntil(t, client.SyncReq{}, client.SyncJoinedTo(user.UserID, roomID)) | ||
| user.MustJoinRoom(t, roomID, nil) |
There was a problem hiding this comment.
This should be before the user.MustSyncUntil afaict.
| res, _ := alice.MustSync(t, client.SyncReq{Filter: filter}) | ||
|
|
||
| // We should see the m.status profile update. | ||
| alice.MustSyncUntil(t, client.SyncReq{Filter: filter}, syncHasProfileUpdate(alice.UserID, "m.status", map[string]interface{}{ |
There was a problem hiding this comment.
| alice.MustSyncUntil(t, client.SyncReq{Filter: filter}, syncHasProfileUpdate(alice.UserID, "m.status", map[string]interface{}{ | |
| alice.MustSyncUntil(t, client.SyncReq{Filter: filter}, syncHasProfileUpdate(bob.UserID, "m.status", map[string]interface{}{ |
This should check for bob's update, not alice's.
3 participants
This PR introduces some initial Complement tests for the current draft of MSC4429: Profile Updates for Legacy Sync.
Note: This PR was almost entirely written by OpenAI Codex, but has been reviewed manually. Extra review scrutiny is advised.
Tests are expected to fail until the associated homeserver implementation has been written.
Pull Request Checklist