Add a basic test fetching thumbnail from _matrix/federation/v1/media/thumbnail
#732
Conversation
H-Shay
force-pushed
the
shay/fed_thumbnail_length
branch
from
ec506d5 to
aec09cc
Compare
H-Shay
force-pushed
the
shay/fed_thumbnail_length
branch
from
aec09cc to
1a50e15
Compare
|
(note that Synapse failure would be expected since the test can't pull in my fork) |
tests/media_thumbnail_test.go
Outdated
| body, err := io.ReadAll(resp.Body) | ||
| length := len(body) | ||
|
|
||
| contentLength, err := strconv.Atoi(resp.Header.Get("Content-Length")) |
There was a problem hiding this comment.
Can we instead compare the received boddy to test.png? I don't think we necessarily should mandate that servers set Content-Length in the black box test suite.
There was a problem hiding this comment.
Will that still fail if the Content-Length is set, but incorrect? That's what the synapse bug is.
There was a problem hiding this comment.
I think it probably makes sense to move the content-length check to the existing Synapse-specific tests for this endpoint: https://github.com/element-hq/synapse/blob/70b0e386032ed2f3ecf25fcf9a4b6c31335ffdc4/tests/federation/test_federation_media.py#L152
There was a problem hiding this comment.
Complement should definitely be checking Content-Length for all requests on all implementations - maybe it already is and was just missing a call to the federation thumbnail endpoint, I don't know.
There was a problem hiding this comment.
To expand: HTTP servers can either respond with Content-Length or chunked transfer encoding, and I don't think we should be mandating one or the other. We could do this by explicitly checking for the two cases, but what we actually care about is that the file was correctly transmitted (and it won't be if the Content-Length is wrong, unless golang HTTP client does something non-HTTP-spec compliant).
| t.Fatalf("body does not match uploaded file") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we add a check after the loop to make sure that an image was actually included in the response, mostly to make it clear that we are actually checking the image. Just something like have_checked_image = true; ...; assert have_changed_image
There was a problem hiding this comment.
Right this is sorted - if you're happy with the change can you hit the merge button for me (as I no longer posses the power). Thanks so much!
As the title states. element-hq/synapse#17518 was missed because Synapse does not request thumbnails via this endpoint, rather fetches the media over the
/downloadendpoint and then generates the thumbnails. This PR adds a test to ensure the basic functionality of the/thumbnailendpoint.