Skip to content
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

Update thumbnail tests to include _matrix/client/v1/media/thumbnail endpoint #728

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Jul 2, 2024

No description provided.

@H-Shay H-Shay requested review from a team as code owners July 2, 2024 18:42
@H-Shay H-Shay changed the title Update thumbnail tests to include _matrix/client/v1/media endpoint Update thumbnail tests to include _matrix/client/v1/media/thumbnail endpoint Jul 2, 2024
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like an updated test :D

if you have any trouble trying to apply the subtest idea then let me know, don't want to make things unnecessarily hard, it just seemed like a clean thing to do

@@ -29,7 +30,10 @@ func TestLocalPngThumbnail(t *testing.T) {

uri := alice.UploadContent(t, data.LargePng, fileName, contentType)

fetchAndValidateThumbnail(t, alice, uri)
fetchAndValidateThumbnail(t, alice, uri, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's nothing wrong with this, but for this and the Remote test, I think it'd be better if the old and new endpoints were separate tests.

Supposing you had a defect in one of the versions of endpoints, you'd want the test results to show which version was working fine and which version wasn't.

Equally, if a new cutting-edge homeserver Syndruit wants to skip implementing the legacy endpoints, they may want to blacklist the tests just for the old version.

I asked in the Complement room and it seems doing this as subtests would work for this and it looks fairly easy to do: seems like you just wrap each part of the test like this:

Suggested change
fetchAndValidateThumbnail(t, alice, uri, false)
t.Run("unauthenticated", func(t *testing.T) {
fetchAndValidateThumbnail(t, alice, uri, false)
})

& similar for the other ones (The string argument to t.Run is a subtest name)

reivilibre pushed a commit to element-hq/synapse that referenced this pull request Jul 8, 2024
…ticated `_matrix/client/v1/media/thumbnail` endpoint (#17388)

[MSC3916](matrix-org/matrix-spec-proposals#3916)
added the endpoints `_matrix/federation/v1/media/thumbnail` and the
authenticated `_matrix/client/v1/media/thumbnail`.

This PR implements those endpoints, along with stabilizing
`_matrix/client/v1/media/config` and
`_matrix/client/v1/media/preview_url`.

Complement tests are at
matrix-org/complement#728
@H-Shay
Copy link
Contributor Author

H-Shay commented Jul 9, 2024

I've added subtests (thanks for the example!) @reivilibre do you think you could merge this for me if the changes are satisfying?

@reivilibre reivilibre merged commit 0d14432 into matrix-org:main Jul 10, 2024
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants