Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Fix test with Django 5 when pytz is available#9715
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
browniebroke left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense 👍🏻
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Ülgen Sarıkavak <ulgens@users.noreply.github.com>
auvipy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please make the failing test pass as well
kulikjak commentedJun 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 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 commentedJun 5, 2025
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 commentedJun 5, 2025
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 commentedJun 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedJun 5, 2025
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 commentedJun 5, 2025
@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. |
…ing against Django 4
kulikjak commentedJun 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Bruno Alla <browniebroke@users.noreply.github.com>
853969c intoencode:masterUh oh!
There was an error while loading.Please reload this page.
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.
As such, I propose to modify the skip check as follows - after this change, the test passes.