Repository navigation
test(django): make DRF DST regression exercise TIME_ZONE - #21436
vlad-scherbich wants to merge 1 commit into
Conversation
vlad-scherbich
added
the
changelog/no-changelog
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codeowners resolved asResolved from the full PR diff against |
Circular import analysis
|
Dependency direction analysis
|
![datadog-prod-us1-4[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Setting the DRF test app to UTC removes the DST ambiguity in date formatting. The added regression test exercises a timestamp from the repeated fall-back hour.
🤖 Bits Code Review · Commit 2f94deb
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the test-only change addressing daylight-saving timestamp ambiguity.
No open issues 🎉
What changed in this PR
This test-only change stabilizes the Django REST Framework integration suite by avoiding daylight-saving fall-back ambiguity in Django’s date formatting.
Changes:
- Set the test app’s time zone to UTC to prevent
pytz.AmbiguousTimeError. - Add a regression test that formats a timestamp from the US fall-back hour.
| File | Description |
|---|---|
| tests/contrib/djangorestframework/test_djangorestframework.py | Adds a regression test for fall-back timestamp formatting. |
| tests/contrib/djangorestframework/app/settings.py | Sets TIME_ZONE = "UTC" and explains the DST safeguard. |
Review effort: Balanced
|
/code blockers |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e987294359
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
View all feedbacks in Devflow UI.
Detected 3 merge blocker(s) to address: 🔴 Blocking
🟠 Pending
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The naive date_format check on #21436 stayed green under zoneinfo even without UTC. Pin TIME_ZONE=UTC and exercise |date "r" / make_aware so Chicago still fails on pytz Django in this suite.
e987294 to
192833e
Compare
vlad-scherbich
changed the title
There was a problem hiding this comment.
More details
The explicit UTC assertion catches configuration regressions even on zoneinfo-backed Django, where ambiguous local times do not raise. The completed static review identified no reportable defect.
🤖 Bits Code Review · Commit 192833e
@wconti27 Done, please take a look at the new test. |
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 2 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 192833e | Docs | View more details | Give us feedback! |
| @pytest.mark.skipif(django.VERSION < (1, 10), reason="requires django version >= 1.10") | ||
| def test_dst_fall_back_ambiguous_local_time_formats() -> None: | ||
| # Django's 500 log-email templates format "now" with |date "r". With USE_TZ off, | ||
| # that is a naive wall-clock stamp make_aware'd in TIME_ZONE. Under pytz, | ||
| # America/Chicago raises AmbiguousTimeError for 01:xx on US fall-back night; | ||
| # UTC has no DST, so the same stamp formats cleanly. zoneinfo (Django 4+) does | ||
| # not raise on ambiguous times, so we also pin TIME_ZONE=UTC explicitly. | ||
| ambiguous_local: datetime = datetime(2026, 11, 1, 1, 51, 26, 582942) | ||
|
|
||
| assert settings.TIME_ZONE == "UTC" | ||
| formatted: str = date_format(ambiguous_local, "r") | ||
| assert formatted | ||
|
|
||
| # Confirm Chicago still blows up on pytz Django (2.2/3.2 in this suite). Without | ||
| # this, removing TIME_ZONE=UTC would leave Django 4 rows green forever. | ||
| if django.VERSION < (4, 0): | ||
| with override_settings(TIME_ZONE="America/Chicago"): | ||
| get_default_timezone.cache_clear() | ||
| with pytest.raises(Exception) as exc_info: | ||
| timezone.make_aware(ambiguous_local, get_default_timezone()) | ||
| assert type(exc_info.value).__name__ == "AmbiguousTimeError" | ||
| get_default_timezone.cache_clear() | ||
|
|
There was a problem hiding this comment.
Do we need to do all of that for something that will happen once a year ?
There was a problem hiding this comment.
Like what are we really testing there ?
Description
William Conti asked for this test fix: "looks good tho should fix the test", and on the review note that the old check never looked at
TIME_ZONE.The version he reviewed formatted a fixed
datetime(2026, 11, 1, 1, 51, 26, 582942)and only checked that the string was non-empty.test_dst_fall_back_ambiguous_local_time_formatsstill uses that timestamp. It assertssettings.TIME_ZONE == "UTC", assertsdate_format(..., "r")is non-empty, and on Django before 4.0 assertstimezone.make_awareraisesAmbiguousTimeErrorfor that timestamp withTIME_ZONEset toAmerica/Chicago.Testing