Update tests that assumed PL50 for room upgrade, fix others #805
Conversation
anoadragon453
changed the title
tests/30rooms/60version_upgrade.pl
Outdated
| @@ -510,6 +422,9 @@ sub upgrade_room_synced { | |||
| foreach my $k ( keys %STATE_DICT ) { | |||
| $levels->{events}->{$k} = 80; | |||
| } | |||
| # Modify tombstone requirements to 50 so we are able to upgrade the room | |||
| $levels->{events}->{"m.room.tombstone"} = 50; | |||
There was a problem hiding this comment.
Why? Doesn't the creator have 100?
There was a problem hiding this comment.
Just a couple lines below we modify creator to be PL50.
There was a problem hiding this comment.
Hmm, ok, but why? If its trying to test that moderators can upgrade rooms by default then that sounds we're making the wrong change here.
There was a problem hiding this comment.
Originally moderators were allowed to update rooms by default, and that caused a bug where power levels could split brain. Technically someone could put a room back into this state, so it may still be worth testing?
Originally this PR just deleted this test, but rich thought it good to keep it as it's still a possibility.
Would you modify it in another way?
There was a problem hiding this comment.
Just don't bother changing the creator to a mod? The test name suggests its trying to test that state gets copied over correctly, so testing that they can do so as a mod was probably just a cheeky extra test
There was a problem hiding this comment.
AH. Sorry, I was thinking of "/upgrade preserves the power level of the upgrading user in old and new rooms". This is a separate test. Yes, we can just pull the PL50 change out of here.
tests/30rooms/60version_upgrade.pl
Outdated
|
|
||
| # Note that this test assumes that moderators by default are allowed to upgrade rooms | ||
| # Change the PL rules to allow moderators to send tombstones | ||
| $pl_content->{events}->{"m.room.tombstone"} = JSON::number(50); |
There was a problem hiding this comment.
I'd be tempted to instead have the creator upgrade tbh
There was a problem hiding this comment.
I should stick this in a comment, but the bug that this tested for was a moderator getting upgraded to an admin in the old room. So changing the upgrader to be an admin initially may not preserve the test case?
There was a problem hiding this comment.
I'll add a comment.
…elease-v1.10.x * commit '86ceaf920b21c4405794c867d60ad13552fec98e': Update tests that assumed PL50 for room upgrade, fix others (#805) Test /user/devices/ on fed reader (#799) Update tests to match removal of Synapse sending events when creating or deleting aliases. Route GET /groups queries to workers (#798) Don't push Dendrite images to Docker Hub (#797) Test that remote device list is refetched on leave/rejoin (#793) Update build.sh Update dendrite.Dockerfile Apply suggestions from code review add test for device key signatures over federation
Synapse PR: matrix-org/synapse#6834