Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
browniebroke merged 9 commits intoencode:masterfromkulikjak:pytz-test-fix
Jul 24, 2025

Conversation

@kulikjak
Copy link
Contributor

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_inputsself.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_valuereturnself.enforce_timezone(parsed)../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/fields.py:1168: in enforce_timezoneraise e../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/fields.py:1162: in enforce_timezoneifnot valid_datetime(dt):../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/utils/timezone.py:23: in valid_datetimeifisinstance(dt.tzinfo, tzinfo)andnot datetime_ambiguous(dt):../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/utils/timezone.py:16: in datetime_ambiguousreturn datetime_exists(dt)and (../../build/prototype/sparc/usr/lib/python3.13/vendor-packages/rest_framework/utils/timezone.py:9: in datetime_existsreturn dt.astimezone(timezone.utc)== dtE   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
browniebroke previously approved these changesJun 4, 2025
Copy link
Member

@browniebrokebrowniebroke left a comment

Choose a reason for hiding this comment

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

Makes sense 👍🏻

Co-authored-by: Ülgen Sarıkavak <ulgens@users.noreply.github.com>
Copy link
Collaborator

@auvipyauvipy left a 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
Copy link
ContributorAuthor

kulikjak commentedJun 5, 2025
edited
Loading

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 thedjango42 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
Copy link
Contributor

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
Copy link
ContributorAuthor

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
Copy link
Contributor

ulgens commentedJun 5, 2025
edited
Loading

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
Copy link
ContributorAuthor

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
Copy link
Contributor

@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
Copy link
ContributorAuthor

kulikjak commentedJun 5, 2025
edited
Loading

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, thebase anddist 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 inrequirements-testing.txt seems cleaner - all Django 5+ targets will skip the test anyway. Alternatively, CI can be probably modified to executebase anddist with 3.13).

ulgens reacted with rocket emoji

browniebroke
browniebroke previously approved these changesJul 2, 2025
auvipy
auvipy previously approved these changesJul 2, 2025
@browniebrokebrowniebroke added this to the3.16 milestoneJul 6, 2025
Co-authored-by: Bruno Alla <browniebroke@users.noreply.github.com>
@browniebrokebrowniebroke merged commit853969c intoencode:masterJul 24, 2025
7 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@browniebrokebrowniebrokebrowniebroke approved these changes

@auvipyauvipyauvipy left review comments

+1 more reviewer

@ulgensulgensulgens approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.16

Development

Successfully merging this pull request may close these issues.

4 participants

@kulikjak@ulgens@browniebroke@auvipy

[8]ページ先頭

©2009-2025 Movatter.jp