-
Notifications
You must be signed in to change notification settings - Fork 63
Add TestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels
#836
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: timedout <[email protected]>
| doTestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels(t, "12", "restricted") | ||
| } | ||
|
|
||
| func doTestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels(t *testing.T, roomVersion string, joinRule string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the reason other tests are written this way is so that it can be shared with tests/knock_restricted_test.go. I'm guessing we should also have knock_restricted test there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(needs change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added knock variants in 3cb69dd. Do you want them all to have docstrings or is TestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevelsV12's just being within view fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can say this:
// See docstring on `TestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevelsV12`
tests/restricted_rooms_test.go
Outdated
| // an appropriate creator cannot be found. | ||
| // | ||
| // ref: https://github.com/element-hq/synapse/issues/19120 | ||
| func TestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to write a test! It looks great overall 🤩
tests/restricted_rooms_test.go
Outdated
| // an appropriate creator cannot be found. | ||
| // | ||
| // ref: https://github.com/element-hq/synapse/issues/19120 | ||
| func TestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested to make sure it fails with the latest develop with Synapse and passes with element-hq/synapse#19321 ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again to make sure all of the new tests pass locally ✅ :
- Checkout Complement:
- Switch to the correct branch (this PR):
git fetch origin refs/pull/836/head:pr-836git checkout pr-836
- Checkout Synapse:
- Switch to the correct branch (Fall back to checking power levels when sourcing local restricted join users element-hq/synapse#19321):
git fetch origin refs/pull/19321/head:pr-19321git checkout pr-19321
COMPLEMENT_DIR=../complement ./scripts-dev/complement.sh ./tests -run TestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevelsVCOMPLEMENT_DIR=../complement ./scripts-dev/complement.sh ./tests -run TestKnockRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevelsV
Co-authored-by: Eric Eastwood <[email protected]>
Signed-off-by: timedout <[email protected]>
| doTestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels(t, "12", "restricted") | ||
| } | ||
|
|
||
| func doTestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels(t *testing.T, roomVersion string, joinRule string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(needs change)
Signed-off-by: timedout <[email protected]>
Signed-off-by: timedout <[email protected]>
Signed-off-by: timedout <[email protected]>
| doTestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels(t, "12", "restricted") | ||
| } | ||
|
|
||
| func doTestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevels(t *testing.T, roomVersion string, joinRule string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can say this:
// See docstring on `TestRestrictedRoomsLocalJoinNoCreatorsUsesPowerLevelsV12`| // Alice restricts the invite power level to moderators and promotes Bob to | ||
| // moderator. | ||
| state_key := "" | ||
| if roomVersion == "12" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this works but doesn't feel the best given future room versions will come along. I guess we will leave the refactor until then. And most of these tests should be running across most of the room versions anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a more proper way to do it let me know, I'm used to something like mautrix's RoomVersion type which has feature flags, but I'm not familiar with complement's codebase all that much and I'm equally short on time so I haven't looked to see if there's a more appropriate solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a good strategy to address this kind of thing in Complement yet.
We can leave it to future refactors of the the tests that make them run across all room versions.
Signed-off-by: timedout <[email protected]>
…n users (#19321) Fix #19120 by always falling back to checking power levels for local users if a local creator cannot be found in a v12 room. Complement tests: matrix-org/complement#836
Adds a test to prevent regressions fixed by element-hq/synapse#19321.
Reproduce element-hq/synapse#19120
Pull Request Checklist