bug 1173189: Force timezone-aware times for feeds #4621
Conversation
escattone
left a comment
There was a problem hiding this comment.
Wow. Fixes the DST issue, modernizes the tests, and takes coverage on kuma/wiki/feeds.py from 93% to 100%. Thanks @jwhitlock!
|
|
||
|
|
||
| def test_recent_documents_handles_ambiguous_time(root_doc, client): | ||
| """The recent_documents feed handles times during DST transition.""" |
There was a problem hiding this comment.
Super nit: recent documents rather than recent_documents?
Convert Document feed tests to use pytest and fixtures. Existing tests were converted (except for the silly parts), and then additional tests added to get complete coverage.
When a naive datetime is ambigious because of the transition from daylight saving time to standard time, assume that the date is DST. This avoids a month of feed errors in the second half of the year, until we can transition to timezone-aware dates.
d9090c4 to
e1c7b05
Compare
Codecov Report
@@ Coverage Diff @@
## master #4621 +/- ##
==========================================
+ Coverage 95.2% 95.28% +0.08%
==========================================
Files 262 262
Lines 23188 23287 +99
Branches 1673 1660 -13
==========================================
+ Hits 22076 22190 +114
+ Misses 893 886 -7
+ Partials 219 211 -8
Continue to review full report at Codecov.
|
|
Force-push to rebase and fix doc nit. |
| if is_naive(raw_pubdate): | ||
| # USE_TZ is False, database has naive timestamps | ||
| timezone = get_current_timezone() | ||
| pubdate = timezone.localize(raw_pubdate, is_dst=True) |
There was a problem hiding this comment.
I could not find any documentation of this localize in documentation.
by supplying is_dst=True, are we assuming that its always Daylight Saving time?
There was a problem hiding this comment.
@safwanrahman Yes, we're just going to assume it's always DST until we start storing timezone-aware datetimes.
There was a problem hiding this comment.
@safwanrahman I had similar questions that were not answered by the docs I read. I assumed it was OK because other tests outside the magic hour passed. I'm investigating a little more now that you've asked it. Results to come...
There was a problem hiding this comment.
localize is provided by pytz, which has pretty good narrative documentation, but doesn't go into details about the functions.
The details are in the source, hosted on launchpad:
https://git.launchpad.net/pytz/tree/src/pytz/tzinfo.py#n244
The code looks forward a day and back a day, to see if the time is around a transition. This can result in 0, 1, or 2 possible timezones:
- 0 cases: The time is an "impossible time", during the lost hour during the transition into daylight savings time.
is_dst=Noneraises an error,is_dst=Trueuses the timezone 6 hours later, andis_dst=Falseuses the timezone 6 hours earlier. - 1 case: There is no ambiguity, the one timezone is used
- 2 cases: The time is from the transition out of daylight savings time, when an hour is repeated.
is_dst=Noneraises an error,is_dst=Trueuses the DST timezone,is_dst=Falseuses the standard timezone.
I'm not Python, so I could have read the code wrong. Here's some test code in the Django shell:
>>> from datetime import datetime as dt
>>> from django.utils.timezone import get_current_timezone
>>> for month in range(1,13):
... test = dt(2017, month, 3, 1, 23)
... print(get_current_timezone().localize(test))
...
2017-01-03 01:23:00-08:00
2017-02-03 01:23:00-08:00
2017-03-03 01:23:00-08:00
2017-04-03 01:23:00-07:00
2017-05-03 01:23:00-07:00
2017-06-03 01:23:00-07:00
2017-07-03 01:23:00-07:00
2017-08-03 01:23:00-07:00
2017-09-03 01:23:00-07:00
2017-10-03 01:23:00-07:00
2017-11-03 01:23:00-07:00
2017-12-03 01:23:00-08:00
Here in the Central time zone, the dates from April through November are Central Daylight Time (-07:00), and the other months are Central Standard Time (-8:00).
That was a fun question, thanks!
Re-write all the feeds tests, to use
pytestand fixtures, for full code coverage ofkuma/wiki/feeds.py, and to avoid silly things like sleeping for a second to get a new timestamp.Then, add some additional handling of our naive timestamps, to turn them into timezone-aware timestamps needed for the feeds. This avoid errors in the second half of the year when the US moves from Daylight Savings Time to standard time, and 1 AM happens twice. When these ambiguous times are encountered, they currently cause the feed to fail with an error. The change is to assume that it is the first 1 AM, avoiding the conversion error.