From 922973bf050e62feee40dd922c3e5e1b4d076d99 Mon Sep 17 00:00:00 2001 From: Zac-HD Date: Mon, 15 Nov 2021 23:38:32 +1100 Subject: [PATCH] Fix negative-DST bug --- hypothesis-python/RELEASE.rst | 7 ++ .../strategies/_internal/datetime.py | 10 ++- .../tests/datetime/test_pytz_timezones.py | 64 ++++++++++--------- 3 files changed, 47 insertions(+), 34 deletions(-) create mode 100644 hypothesis-python/RELEASE.rst diff --git a/hypothesis-python/RELEASE.rst b/hypothesis-python/RELEASE.rst new file mode 100644 index 0000000000..ec693dbe45 --- /dev/null +++ b/hypothesis-python/RELEASE.rst @@ -0,0 +1,7 @@ +RELEASE_TYPE: patch + +This patch fixes a rare internal error in the :func:`hypothesis.strategies.datetimes` +strategy, where the implementation of ``allow_imaginary=False`` crashed when checking +a time during the skipped hour of a DST transition *if* the DST offset is negative - +only true of ``Europe/Dublin``, who we presume have their reasons - and the ``tzinfo`` +object is a :pypi:`pytz` timezone (which predates :pep:`495`). diff --git a/hypothesis-python/src/hypothesis/strategies/_internal/datetime.py b/hypothesis-python/src/hypothesis/strategies/_internal/datetime.py index c820813655..8fd22b5d4b 100644 --- a/hypothesis-python/src/hypothesis/strategies/_internal/datetime.py +++ b/hypothesis-python/src/hypothesis/strategies/_internal/datetime.py @@ -97,6 +97,13 @@ def datetime_does_not_exist(value): # meaningless before ~1900 and subject to a lot of change by # 9999, so it should be a very small fraction of possible values. return True + + if value.tzinfo.utcoffset != roundtrip.tzinfo.utcoffset: + # This only ever occurs during imaginary (i.e. nonexistent) datetimes, + # and only for pytz timezones which do not follow PEP-495 semantics. + # (may exclude a few other edge cases, but you should use zoneinfo anyway) + return True + assert value.tzinfo is roundtrip.tzinfo, "so only the naive portions are compared" return value != roundtrip @@ -196,9 +203,6 @@ def datetimes( which did not (or will not) occur due to daylight savings, leap seconds, timezone and calendar adjustments, etc. Imaginary datetimes are allowed by default, because malformed timestamps are a common source of bugs. - Note that because :pypi:`pytz` predates :pep:`495`, this does not work - correctly with timezones that use a negative DST offset (such as - ``"Europe/Dublin"``). Examples from this strategy shrink towards midnight on January 1st 2000, local time. diff --git a/hypothesis-python/tests/datetime/test_pytz_timezones.py b/hypothesis-python/tests/datetime/test_pytz_timezones.py index f62fbf0699..35b2f9d3c2 100644 --- a/hypothesis-python/tests/datetime/test_pytz_timezones.py +++ b/hypothesis-python/tests/datetime/test_pytz_timezones.py @@ -23,6 +23,7 @@ from hypothesis.errors import InvalidArgument from hypothesis.extra.pytz import timezones from hypothesis.strategies import data, datetimes, just, sampled_from, times +from hypothesis.strategies._internal.datetime import datetime_does_not_exist from tests.common.debug import ( assert_all_examples, @@ -124,36 +125,37 @@ def test_datetimes_stay_within_naive_bounds(data, lo, hi): assert lo <= out.replace(tzinfo=None) <= hi -@pytest.mark.xfail(reason="is_dst not equivalent to fold when DST offset is negative") -def test_datetimes_can_exclude_imaginary(): - # The day of a spring-forward transition; 2am is imaginary - australia = { - "min_value": dt.datetime(2020, 10, 4), - "max_value": dt.datetime(2020, 10, 5), - "timezones": just(pytz.timezone("Australia/Sydney")), - } - # Ireland uses *negative* offset DST, which means that our sloppy interpretation - # of "is_dst=not fold" bypasses the filter for imaginary times. This is basically - # unfixable without redesigning pytz per PEP-495, and it's much more likely to be - # replaced by dateutil or PEP-615 zoneinfo in the standard library instead. - # (we use both so an optimistic `is_dst=bool(fold)` also fails the test) - ireland = { - "min_value": dt.datetime(2019, 3, 31), - "max_value": dt.datetime(2019, 4, 1), - "timezones": just(pytz.timezone("Europe/Dublin")), - } +@pytest.mark.parametrize( + "kw", + [ + # Ireland uses *negative* offset DST, which means that our sloppy interpretation + # of "is_dst=not fold" bypasses the filter for imaginary times. This is basically + # unfixable without redesigning pytz per PEP-495, and it's much more likely to be + # replaced by dateutil or PEP-615 zoneinfo in the standard library instead. + { + "min_value": dt.datetime(2019, 3, 31), + "max_value": dt.datetime(2019, 4, 1), + "timezones": just(pytz.timezone("Europe/Dublin")), + }, + # The day of a spring-forward transition in Australia; 2am is imaginary + # (the common case so an optimistic `is_dst=bool(fold)` also fails the test) + { + "min_value": dt.datetime(2020, 10, 4), + "max_value": dt.datetime(2020, 10, 5), + "timezones": just(pytz.timezone("Australia/Sydney")), + }, + ], +) +def test_datetimes_can_exclude_imaginary(kw): # Sanity check: fail unless those days contain an imaginary hour to filter out - find_any( - datetimes(**australia, allow_imaginary=True), - lambda x: not datetime_exists(x), - ) - find_any( - datetimes(**ireland, allow_imaginary=True), - lambda x: not datetime_exists(x), - ) + find_any(datetimes(**kw, allow_imaginary=True), lambda x: not datetime_exists(x)) + # Assert that with allow_imaginary=False we only generate existing datetimes. - assert_all_examples( - datetimes(**australia, allow_imaginary=False) - | datetimes(**ireland, allow_imaginary=False), - datetime_exists, - ) + assert_all_examples(datetimes(**kw, allow_imaginary=False), datetime_exists) + + +def test_really_weird_tzinfo_case(): + x = dt.datetime(2019, 3, 31, 2, 30, tzinfo=pytz.timezone("Europe/Dublin")) + assert x.tzinfo is not x.astimezone(dt.timezone.utc).astimezone(x.tzinfo) + # And that weird case exercises the rare branch in our helper: + assert datetime_does_not_exist(x)