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

Change MEDIA_URL to an absolute URL in tests #4460

Merged
merged 3 commits into from
Aug 28, 2023

Conversation

itisnotyourenv
Copy link
Contributor

Description

I recently noticed that in cases where we have a default value for models' ImageField like in the example below

class Book(models.Model):
    cover = models.ImageField(upload_to="images/", default=DEFAULT_IMAGE, verbose_name="Изображение")

and try to compare the field from the response with the expected field from a serializer

class TestUserViewSet:
    def test_get_user_by_id(self, api_client: APIClient, user: User, admin_user: User):
        url = reverse("api:users-detail", kwargs={"id": admin_user.id})
        expected_data = UserSerializer(admin_user).data

        response = api_client.get(url)

        assert response.status_code == status.HTTP_200_OK
        assert response.data.get("image") == expected_data.get("image")

we encounter a failure because Django adds a prefix "http://testserver" to the ImageField and for other areas that use MEDIA_URL.
{'image': 'http://testserver/media/default/user-logo.jpg'} != {'image': '/media/default/user-logo.jpg'}

Checklist:

  • I've made sure that tests are updated accordingly (especially if adding or updating a template option)
  • I've updated the documentation or confirm that my change doesn't require any updates

Rationale

This will help to avoid incomprehensible errors in tests and these changes can be quite in demand.

@browniebroke
Copy link
Member

browniebroke commented Jul 15, 2023

Thanks for your contribution.

Is there a reason to use a fixture to set this? Since it's a constant string, it's better to set it in the dedicated setting file for tests, as it's where other similar ones are set. The media_storage defined above is set via a fixture because it uses the pytest fixture tmpdir.

Small note that this URL prefix is actually added by DRF (https://github.com/encode/django-rest-framework/blob/7658ffd71d4ad07ddada20b2b9538b889ec02403/rest_framework/fields.py#L1539-L1554), to make the URL absolute. Although, it's true that it's calling an API from Django: https://github.com/django/django/blob/0016a4299569a8f09ff24053ff2b8224f7fa4113/django/http/request.py#L201.

Which leads me to the fact that I'm slightly against this change, for the following reasons:

  • By setting the MEDIA_URL value to an absolute URI, you're hiding the fact that this transformation happens in the serialization, which is a kind of detail that I find useful to see in my tests.
  • I'd argue that your test should compare the result of the serialization to something more static: assert response.data["image"] == "http://testserver/media/default/user-logo.jpg". The way it's written in your example feels to me like testing that 1 + 1 == 1 + 1, if that makes sense.

Although the counter argument to the 1st point here is that the MEDIA_URL is often absolute on prod anyway... I guess we could set it to an absolute URL different from the default server URL? e.g. https://testassets/media/.

@browniebroke
Copy link
Member

Hey @itisnotyourenv 👋🏻 What do you think of my suggestions I mentioned above? Do you want to make any edits to this PR?

@browniebroke browniebroke changed the title fixture for changing media url Set MEDIA_URL to an absolute URL in tests Aug 28, 2023
@browniebroke browniebroke changed the title Set MEDIA_URL to an absolute URL in tests Change MEDIA_URL to an absolute URL in tests Aug 28, 2023
@browniebroke browniebroke merged commit b117f00 into cookiecutter:master Aug 28, 2023
12 checks passed
@itisnotyourenv
Copy link
Contributor Author

Hey @itisnotyourenv 👋🏻 What do you think of my suggestions I mentioned above? Do you want to make any edits to this PR?

Sorry bro. I forgot about this PR, and didn't notice your last question. Thanks for merging

@itisnotyourenv itisnotyourenv deleted the media-url branch August 29, 2023 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants