Add some tests for push rule behavior on room upgrade#819
Add some tests for push rule behavior on room upgrade#819MadLittleMods merged 22 commits intomainfrom
Conversation
This reverts commit 3fdbd02.
Synapse looks for the connection in order to run the upgrade logic for remote room joins
| // FIXME: We have to skip this test on Synapse until | ||
| // https://github.com/element-hq/synapse/issues/19199 is resolved. | ||
| if useManualRoomUpgrade { | ||
| runtime.SkipIf(t, runtime.Synapse) | ||
| } |
There was a problem hiding this comment.
We have to skip with Synapse until element-hq/synapse#19199 is resolved ⏩
| for _, client := range []*client.CSAPI{bob, bob2} { | ||
| pushRulesAfter := client.GetAllPushRules(t) | ||
| t.Logf("Checking push rules (after upgrade) for %s", client.UserID) | ||
| must.MatchGJSON(t, pushRulesAfter, |
There was a problem hiding this comment.
These tests appear flaky with Dendrite so we may need to sync until the push rules appear.
There was a problem hiding this comment.
Making the tests wait for push rules from /sync did not help Dendrite 😅
Seems like the push rules aren't being returned from /sync when handleRoomUpgrade -> copyPushrules is used.
There was a problem hiding this comment.
Skipping tests on Dendrite for now ⏩
There was a problem hiding this comment.
For the future: @S7evinK (former Dendrite dev) mentioned,
Looks like we don't let the syncapi know we changed the account data. Should be trivial to fix.
The relevant code is probably in syncapi/streams/stream_accountdata.go#L41-L108 but I don't immediatly see the problem. There is a skip condition which is a bit suspect but supposedly only applies when from == 0 (initial syncs) vs the incremental syncs that's happening here in the test (and the other conditions should make the skip only apply to room scoped account data).
Or I'm looking in the wrong place 🤷
Given that Dendrite doesn't have a convenient way to run Complement locally like we have with Synapse's complement.sh, this takes more effort than necessary to debug.
There was a problem hiding this comment.
Would be the right place, but I didn't have time to check if we need to/should s.syncProducer.SendAccountData after each function call.
anoadragon453
left a comment
There was a problem hiding this comment.
Tests look good to me. Gold star for writing tests to validate broken behaviour seen in the wild 🌟
| for _, client := range []*client.CSAPI{bob, bob2} { | ||
| pushRulesAfter := client.GetAllPushRules(t) | ||
| t.Logf("Checking push rules (after upgrade) for %s", client.UserID) | ||
| must.MatchGJSON(t, pushRulesAfter, |
|
Thanks for the review @anoadragon453 🦙 element-hq/dendrite#3668 looks incredibly promising for unlocking these tests on the Dendrite side! Thank you @S7evinK for jumping on a fix for this! ❤️ Can't wait to remove the Dendrite skips once this lands ⏩ The broken scenario in Synapse (re: the other skipped test) is tracked by element-hq/synapse#19199 |
) This should help with matrix-org/complement#819 ### Pull Request Checklist <!-- Please read https://matrix-org.github.io/dendrite/development/contributing before submitting your pull request --> * [x] I have added Go unit tests or [Complement integration tests](https://github.com/matrix-org/complement) for this PR _or_ I have justified why this PR doesn't need tests * [x] Pull request includes a [sign off below](https://element-hq.github.io/dendrite/development/contributing#sign-off) _or_ I have already signed off privately --------- Signed-off-by: Till Faelligen <[email protected]>
Add some tests for push rule behavior on room upgrade.
In Synapse, this behavior is handled by
transfer_room_state_on_room_upgrade->copy_user_state_on_room_upgrade->copy_push_rules_from_room_to_room_for_user.In Dendrite, this behavior is handled by
handleRoomUpgrade->copyPushrules.This behavior is not explained in the Matrix spec but perhaps that just needs a clarification/MSC to document the state of things. Synapse and Dendrite do this for example.
This is spawning from wanting to see whether I can reproduce an issue I was experiencing during the recent room upgrades to v12. I previously had my notification settings for a room as "Mentions & keywords" in Element Web but then after the upgrade, it was using the "Match default settings" and I was receiving notifications for all new messages. The tests do now reproduce the problem!
See element-hq/synapse#19199
My Element Web notification settings for reference:
Dev notes
m.room.tombstonepredecessorin them.room.createeventDendrite:
internal/perform/perform_upgrade.gohandleRoomUpgrade->copyPushrulesPull Request Checklist
Signed-off-by: Eric Eastwood [email protected]