Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+6 −1
Open

Fix test with Django 5 when pytz is available #9715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
656d835
Fix test with Django 5 when pytz is available
kulikjak Jun 4, 2025
4e42975
fix formatting
kulikjak Jun 4, 2025
5b25346
remove original condition
kulikjak Jun 5, 2025
3817fb9
remove trailing whitespace
kulikjak Jun 5, 2025
d3ff351
further improvements
kulikjak Jun 5, 2025
a0fbdbd
let's not skip the pytz test - it should always be executed when test…
kulikjak Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions requirements/requirements-testing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ pytest-django>=4.5.2,<5.0
importlib-metadata<5.0
# temporary pin of attrs
attrs==22.1.0
pytz
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ulgens ulgens Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Which kind of problems?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ulgens ulgens Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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"

Copy link
Author

Choose a reason for hiding this comment

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

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.

6 changes: 5 additions & 1 deletion tests/test_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
except ImportError:
pytz = None

import django
from django.core.exceptions import ValidationError as DjangoValidationError
from django.db.models import IntegerChoices, TextChoices
from django.http import QueryDict
Expand Down Expand Up @@ -1624,7 +1625,10 @@ def test_should_render_date_time_in_default_timezone(self):
assert rendered_date == rendered_date_in_timezone


@pytest.mark.skipif(pytz is None, reason="Django 5.0 has removed pytz; this test should eventually be able to get removed.")
@pytest.mark.skipif(
condition=django.VERSION >= (5,),
reason="Django 5.0 has removed pytz; this test should eventually be able to get removed.",
)
class TestPytzNaiveDayLightSavingTimeTimeZoneDateTimeField(FieldValues):
"""
Invalid values for `DateTimeField` with datetime in DST shift (non-existing or ambiguous) and timezone with DST.
Expand Down
Loading