Repository navigation
fix: set avatar_url in TestRoomState test - #939
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
a6a8326 to
17a41b5
Compare
RasmusRendal
changed the title
avatar_url in addition to explicit nullavatar_url in TestRoomState test
17a41b5 to
92d2d3f
Compare
| t.Run("GET /rooms/:room_id/joined_members fetches my membership", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| avatarURL := "mxc://example.com/SEsfnsuifSDFSSEF" |
There was a problem hiding this comment.
Matches the mxc:// URL we've already used elsewhere 👍
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>
92d2d3f to
601d1df
Compare
Context is that when no avatar is set, Synapse still returns
{"avatar_url": null}in response to/joined_members. Butnullisn't really a validavatar_urlvalue according to the Matrix spec and Arminius (another homeserver implementation) aligns with the Matrix spec here (omitavatar_urlwhen not set).As @MadLittleMods explained below, we should either:
To play nice with Synapse and Arminius, we go with option a.
Pull Request Checklist