Add test case reproducing matrix-org/synapse#5677 for local users#199
Merged
Conversation
Contributor
Author
|
The new test case seems to fail when ran against dendrite, because the first request to |
kegsay
requested changes
Aug 23, 2021
| @@ -0,0 +1,64 @@ | |||
| // +build !synapse_blacklist | |||
|
|
|||
| // Rationale for being included in Synapse's blacklist: https://github.com/matrix-org/synapse/issues/5677 | |||
Member
There was a problem hiding this comment.
You probably don't want this on Synapse's blacklist.
Contributor
Author
There was a problem hiding this comment.
My thinking was that I wouldn't want synapse CI to suddenly light up red. But maybe your point is "it should do!"
|
|
||
| alice := deployment.Client(t, "hs1", "@alice:hs1") | ||
| bob := deployment.RegisterUser(t, "hs1", "bob", "bob-pw") | ||
| eve := deployment.RegisterUser(t, "hs1", "eve", "eve-pw") |
Member
There was a problem hiding this comment.
These passwords will be too short for some servers, which is probably why it 400s on Dendrite.
| alice.JoinRoom(t, privateRoom, nil) | ||
|
|
||
| // Alice reveals her private name to Bob | ||
| alice.MustDo( |
Member
There was a problem hiding this comment.
Prefer MustDoFunc. MustDo is the older format which doesn't allow for vargs and will be removed in the future. MustDoFunc also logs HTTP response bodies on error.
Contributor
Author
There was a problem hiding this comment.
Will do, thanks. Mind if I mark MustDo as deprecated?
| bob := deployment.RegisterUser(t, "hs1", "bob", "bob-pw") | ||
| eve := deployment.RegisterUser(t, "hs1", "eve", "eve-pw") | ||
|
|
||
| t.Run("Usernames specific to a room aren't leaked in the user directory", func(t *testing.T) { |
Member
There was a problem hiding this comment.
There's no real need to do this as a subtest, unless you plan to add more tests around username leaks?
Contributor
Author
There was a problem hiding this comment.
That's fair. Could add one for avatar leaks too perhaps.
DMRobertson
commented
Aug 23, 2021
Probably bloat that belongs in the specific test?
for consistency with other matchers
hopefully will now pass on dendrite
tried to make this just a warning but I couldn't figure out the config.
Contributor
Author
|
Dendrite now failing with Logs
|
It's not required that you can search for people by per-room names (and per-room names are kind of an accident anyway right now)
richvdh
approved these changes
Aug 26, 2021
Member
|
CI seems to be failing for both Synapse and Dendrite for an unrelated test. Any idea what's going on there? (Old branch? or have you accidentally broken something?) |
Contributor
Author
In both runs I see two failures, both in the newly written tests: TestRoomSpecificUsernameHandling and TestRoomSpecificUsernameHandlingOverFederation. Which ones are you seeing fail, and where? |
Member
oh, sorry. The output is confusing. I saw this at the end of the report: But that doesn't mean what I thought it means. |
kegsay
reviewed
Sep 6, 2021
kegsay
reviewed
Sep 6, 2021
kegsay
approved these changes
Sep 6, 2021
Member
kegsay
left a comment
kegsay
left a comment
There was a problem hiding this comment.
Looks great overall, thank you!
Member
|
Tests are failing however: |
this unfortunately got bundled in with changes I'd already started.
Contributor
Author
|
Running this against my changes in matrix-org/synapse#10695 yields dmr on titan in complement on dmr/per-room-nick-leakage via 🐹 v1.16.6 via 🐍 v3.9.6 (env) took 24s
2021-09-06 18:10:20 ✗ 1 ERROR $ COMPLEMENT_BASE_IMAGE=complement-synapse go test ./... -run TestRoomSpecificUsername | grep FAIL
318:--- FAIL: TestRoomSpecificUsernameHandlingOverFederation (19.90s)
332: --- FAIL: TestRoomSpecificUsernameHandlingOverFederation/Eve_can_find_Charlie_by_profile_display_name (0.03s)
335: --- FAIL: TestRoomSpecificUsernameHandlingOverFederation/Eve_can_find_Charlie_by_mxid (0.00s)
336: user_directory_federated_display_names_test.go:112: MatchResponse all checks failed:
340: --- FAIL: TestRoomSpecificUsernameHandlingOverFederation/Eve_cannot_find_Charlie_by_room-specific_name_that_Eve_is_not_privy_to (0.00s)
343: --- FAIL: TestRoomSpecificUsernameHandlingOverFederation/Bob_can_find_Charlie_by_profile_display_name (0.00s)
346: --- FAIL: TestRoomSpecificUsernameHandlingOverFederation/Bob_can_find_Charlie_by_mxid (0.00s)
347: user_directory_federated_display_names_test.go:155: MatchResponse all checks failed:
350:FAIL
351:FAIL github.com/matrix-org/complement/tests 21.022s
353:FAILEdit: and this is good, because it's only the federated tests failing; the other ones pass. |
Contributor
Author
We could maybe try not passing
|
kegsay
reviewed
Sep 7, 2021
I think "alice" rather than "@alice:example.com" is nicer to display in the UI until we've confirmed their public profile.
Member
|
Please add these tests to dendrite's blacklist, then fix the synapse issue then I'm happy to merge this. |
Contributor
Author
|
Thanks @kegsay, will do. Working on this in synapse now. |
Contributor
Author
The issue is fixed for Synapse for local users now. The other tests over federation ( How does that sound to everyone? |
Member
|
I don't like the idea of merging tests which fail on all HS implementations. I don't like it because:
Remove the failing tests and add them when the underlying issues are resolved. |
Member
|
@DMRobertson what's the status of this PR? Can we merge the bits that work and back out the things which don't? |
Contributor
Author
Ahh sorry, I missed this.
The tests in /csapi should all pass against Synapse now. I'll get that done now. |
Will return to these when I pick up matrix-org/synapse#5677 again.
Contributor
Author
|
The failed dendrite tests don't look related to this one: AFAICS these are failing on |
DMRobertson
changed the title
Member
|
Yep that's fine, @neilalexander is working on that now. |
Member
|
Happy for me to merge this @DMRobertson ? |
Contributor
Author
@kegsay Let's do it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Limited golang experience; feedback graciously accepted