Repository navigation
fix: allow non-present avatar_url in addition to explicit null - #939
RasmusRendal wants to merge 3 commits into
Conversation
| match.JSONKeyPresent("joined." + authedClient.UserID), | ||
| match.JSONKeyPresent("joined." + authedClient.UserID + ".display_name"), | ||
| match.JSONKeyPresent("joined." + authedClient.UserID + ".avatar_url"), | ||
| match.JSONKeyMissingOrNull("joined." + authedClient.UserID + ".avatar_url"), |
There was a problem hiding this comment.
What's spawning this change? Why did you create the PR?
Another homeserver implementation?
There was a problem hiding this comment.
Oh right, this change is motivated by work on the Arminius homeserver implementation :)
We use ruma types, which tend to prefer leaving keys out rather than setting them to null, which is causing us to fail this test in particular.
I haven't looked at it in depth, but I could imagine that Continuwuity fails this test for similar reasons.
| match.JSONKeyPresent("joined." + authedClient.UserID), | ||
| match.JSONKeyPresent("joined." + authedClient.UserID + ".display_name"), | ||
| match.JSONKeyPresent("joined." + authedClient.UserID + ".avatar_url"), | ||
| match.JSONKeyMissingOrNull("joined." + authedClient.UserID + ".avatar_url"), |
There was a problem hiding this comment.
The test doesn't have a description but if the point of this test is to assert the shape of the response, feels like we should either a) add an actual avatar_url and assert JSONKeyPresent or b) no avatar and expect JSONKeyMissing.
Option a) would allow Synapse to keep passing with this specific test and have the best coverage if we only have a single test to cover this 🤷
There was a problem hiding this comment.
I went for a), because it feels quite mean to make Synapse (and the other homeservers that already pass here) fail a test when they're not strictly speaking breaking the spec
| match.JSONKeyPresent("joined." + authedClient.UserID), | ||
| match.JSONKeyPresent("joined." + authedClient.UserID + ".display_name"), | ||
| match.JSONKeyPresent("joined." + authedClient.UserID + ".avatar_url"), | ||
| match.JSONKeyMissingOrNull("joined." + authedClient.UserID + ".avatar_url"), |
There was a problem hiding this comment.
As a note, right now, the test is half-baked since it doesn't assert the type of the value either (we could use JSONKeyEqual or JSONKeyTypeEqual if we don't know the expected value) but prior art so we don't have to dwell on this ⏩
There was a problem hiding this comment.
I included a commit here fixing it for the last two keys :)
6e7e676 to
a6a8326
Compare
Previously the test would expect `avatar_url` to be `null`, failing on tests where the `avatar_url` would be non-present when unset. Now we set an `avatar_url` to make sure that the key is set. Signed-off-by: Rasmus Rendal <r.kjaer@famedly.com>
…hip test We know the type and the entire key in these two cases, so we might as well test it. Signed-off-by: Rasmus Rendal <r.kjaer@famedly.com>
a6a8326 to
17a41b5
Compare
When no avatar is set, Synapse might still return
{"avatar_url": null}in response to/joined_members. But{}contains the same information, and should therefore also be accepted by Complement.Pull Request Checklist