Fix test with Django 5 when pytz is available

[kulikjak]

When running tests against Django 5.2, I am getting the following test failure when pytz is available on the machine. When I remove it, the issue is gone.

_______________________ TestPytzNaiveDayLightSavingTimeTimeZoneDateTimeField.test_invalid_inputs _______________________
test_fields.py:682: in test_invalid_inputs
    self.field.run_validation(input_value)
../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/fields.py:538: in run_validation
    value = self.to_internal_value(data)
../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/fields.py:1190: in to_internal_value
    return self.enforce_timezone(parsed)
../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/fields.py:1168: in enforce_timezone
    raise e
../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/fields.py:1162: in enforce_timezone
    if not valid_datetime(dt):
../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/utils/timezone.py:23: in valid_datetime
    if isinstance(dt.tzinfo, tzinfo) and not datetime_ambiguous(dt):
../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/utils/timezone.py:16: in datetime_ambiguous
    return datetime_exists(dt) and (
../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/utils/timezone.py:9: in datetime_exists
    return dt.astimezone(timezone.utc) == dt
E   NotImplementedError: a tzinfo subclass must implement utcoffset()

As such, I propose to modify the skip check as follows - after this change, the test passes.

[browniebroke]

Makes sense 👍🏻

[auvipy]

please make the failing test pass as well

[kulikjak]

I played with the CI a little and concluded that the following is the best:

Currently, the test is never executed in CI (tox) because pytz is never installed - I added it as one of the django42 dependencies, so it should now get executed with that one.

Also, I split the skip into two - now I get the original message with Django 5.0+, and "pytz is not available" when testing against Django 4.x without pytz.

[ulgens]

If using pytz with a specific version of Django is a common enough thing that we need to test, I think that "pass the test if a dependency is missing" is an anti-pattern. +1 for adding it to the dependencies, -1 for the "skip if not pytz".

[kulikjak]

That is certainly true for CI here, but I can imagine somebody who uses Django 4, doesn't use pytz (it's deprecated there and not necessary) and who wants to run django-rest-framework test suite to ensure that everything works - this skip is there for those.

[ulgens]

doesn't use pytz (it's deprecated there and not necessary) and who wants to run django-rest-framework test suite to ensure that everything works - this skip is there for those.

I'm missing the case here. Let's say someone forks the repository, makes some changes to it, and wants to run the test suite. They will run the test suite as a whole and that's all *. I can't imagine a case that could end up with "Oh I don't need pytz so the repo I'm contributing to shouldn't test it either."

* This is already automated and handled by the CI.

[kulikjak]

It's our case. We download django-rest-framework source, build it, make sure that it works with Solaris and then package it such that it's installable via our package manager (the same will likely apply to many Linux distros). One of the steps is to run the test suite.

We execute the test suite with pytest and thus use dependencies installed on the build machine - if you use tox (or anything else that isolates the test execution in a new virtual environment where it installs all the dependencies), pytz is always brought in, so this wouldn't matter.

But testing against the machine installed bits makes sense in our case and if we for some reason stop delivering pytz (it's not really used today as most libraries moved to native tzinfo), we would still want to make sure django-rest-framework works without it and this skip would allow us to do so.

All that said, I don't really have a strong feeling about this and I am happy to remove the skip :).

[ulgens]

@kulikjak Oh, thanks for all the details. In general, my main assumption is that all contributors should work with the same standardized environment. Custom redistribution is an interesting case that I hadn't considered, but I'd expect to have an active fork in that scenario. All in all, I was more interested in understanding why it's needed, and I also don't have strong opinions about it.

[kulikjak]

Custom redistribution is an interesting case that I hadn't considered, but I'd expect to have an active fork in that scenario.

I agree, and we sometimes do modify sources slightly in cases exactly like this one.

But you are right that contributors here should not skip the pytz test, so I removed it.

(when I remove just the skip, the base and dist targets fail as they are executed with Python 3.9. I can add pytz as a dependency to those two as well, but placing it in requirements-testing.txt seems cleaner - all Django 5+ targets will skip the test anyway. Alternatively, CI can be probably modified to execute base and dist with 3.13).

[browniebroke]

I'm not sure about that one... Would it risk hiding some problem when pytz is NOT installed?

Ideally, I would like to have:

• pytz not explicitly installed by default
• Have a couple/few tox envs testing with pytz installed

Not sure if that would make the matrix too complex

[ulgens]

Would it risk hiding some problem when pytz is NOT installed?

Which kind of problems?

[browniebroke]

Well, I'm talking about an unknown unknown, so I don't have a concrete example, I'm merely asking the question 😄

[ulgens]

Got it. @kulikjak had a case that I haven't even considered before and now I'm asking for all the details I can learn 😄

To answer the question, I can't think of a case. I agree that we could implement a better way than adding pytz to the default set of requirements (could only be installed if Django<5), but I can't imagine a test case that validates "this feature should be working even if x package is not installed"

[kulikjak]

I guess the matrix can be extended to run tests against Django 4 once with pytz and again without it (that would also mean that the second skip has to return :)) but I wonder whether that's necessary.