Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test failure: test_tzlocal_offset_equal[GMT-tzoff1] (dateutil 2.8.0) #918

Closed
koobs opened this issue May 27, 2019 · 18 comments · Fixed by #928
Closed

Test failure: test_tzlocal_offset_equal[GMT-tzoff1] (dateutil 2.8.0) #918

koobs opened this issue May 27, 2019 · 18 comments · Fixed by #928
Milestone

Comments

@koobs
Copy link
Contributor

koobs commented May 27, 2019

Running into the following test failure while QA'ing dateutil 2.8.0 to update the FreeBSD port:

tzvar = 'GMT', tzoff = tzoffset(u'GMT', 0)

    @mark_tzlocal_nix
    @pytest.mark.parametrize('tzvar, tzoff', [
        ('EST5', tz.tzoffset('EST', -18000)),
        ('GMT', tz.tzoffset('GMT', 0)),
        ('YAKT-9', tz.tzoffset('YAKT', timedelta(hours=9))),
        ('JST-9', tz.tzoffset('JST', timedelta(hours=9))),
    ])
    def test_tzlocal_offset_equal(tzvar, tzoff):
        with TZEnvContext(tzvar):
            # Including both to test both __eq__ and __ne__
>           assert tz.tzlocal() == tzoff
E           AssertionError: assert tzlocal() == tzoffset(u'GMT', 0)
E             -tzlocal()
E             +tzoffset(u'GMT', 0)

dateutil/test/test_tz.py:1008: AssertionError

System/test environment is:

platform freebsd13 -- Python 2.7.16, pytest-4.5.0, py-1.8.0, pluggy-0.11.0 -- /usr/local/bin/python2.7
plugins: xdist-1.28.0, mock-1.10.4, forked-1.0.2, cov-2.7.1, hypothesis-4.23.6

System timezone is Australia/Sydney:

# tail -n 1 /etc/localtime
AEST-10AEDT,M10.1.0,M4.1.0/3

I've tried setting TZ=UTC and TZ=other-non-utc-timezones environment variables without affect.

@pganssle
Copy link
Member

I would not expect the TZ variable to have any effect in that case, since TZEnvContext changes the TZ variable for the duration of the test. Does time.tzset work properly in FreeBSD?

@koobs
Copy link
Contributor Author

koobs commented May 30, 2019

@pganssle I run two FreeBSD buildbots for Python core, which pass all tests on all branches (less any known regressions). To the extent that the time module is tested, I can confidently say yes.

If there's some test case you'd like me to run on the system that exhibits the test failure reported here, I'm happy to run them and report back

@pganssle
Copy link
Member

@koobs If you have a checkout of dateutil with the tests in it, can you do:

from dateutil.test._common import TZEnvContext
from dateutil import tz

from datetime import datetime

ts = 1560000000
print(datetime.fromtimestamp(1560000000, tz.tzlocal()))
with TZEnvContext("Pacific/Kiritimati"):
    print(datetime.fromtimestamp(1560000000, tz.tzlocal()))

Assuming you are not in UTC+14:00 time zone, I would expect the two timestamps to give different offsets, do they?

@koobs
Copy link
Contributor Author

koobs commented Jun 1, 2019

[koobs@CURRENT-amd64:/usr/home/koobs/repos/dateutil] git status
On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean
[koobs@CURRENT-amd64:/usr/home/koobs/repos/dateutil] date
Sat Jun  1 13:33:17 AEST 2019
[koobs@CURRENT-amd64:/usr/home/koobs/repos/dateutil] tail -n1 /etc/localtime
AEST-10AEDT,M10.1.0,M4.1.0/3
[koobs@CURRENT-amd64:/usr/home/koobs/repos/dateutil] PYTHONPATH=`pwd` python2.7
Python 2.7.16 (default, May 28 2019, 22:56:37)
[GCC 4.2.1 Compatible FreeBSD Clang 8.0.0 (tags/RELEASE_800/final 356365)] on freebsd13
Type "help", "copyright", "credits" or "license" for more information.
>>> from dateutil.test._common import TZEnvContext
>>> from dateutil import tz
>>>
>>> from datetime import datetime
>>>
>>> ts = 1560000000
>>> print(datetime.fromtimestamp(1560000000, tz.tzlocal()))
2019-06-08 23:20:00+10:00
>>> with TZEnvContext("Pacific/Kiritimati"):
...     print(datetime.fromtimestamp(1560000000, tz.tzlocal()))
...
2019-06-09 03:20:00+14:00
>>>

@pganssle
Copy link
Member

@koobs Hm, so that's basically what I expected (though you're closer to UTC+14 than I expected 😉), which I find confusing.

Can you try this?

TZ="GMT" python -c 'from dateutil import tz; print(tz.tzlocal() == tz.tzoffset("GMT", 0))'

For me that prints True.

@koobs
Copy link
Contributor Author

koobs commented Jun 12, 2019

[koobs@CURRENT-amd64:/usr/home/koobs/repos/dateutil] PYTHONPATH=`pwd` TZ="GMT" python2.7  -c 'from dateutil import tz; print(tz.tzlocal() == tz.tzoffset("GMT", 0))'
False

For completeness I ran it under Python 3.6, in case the symptoms differed

[koobs@CURRENT-amd64:/usr/home/koobs/repos/dateutil] PYTHONPATH=`pwd` TZ="GMT" python3.6  -c 'from dateutil import tz; print(tz.tzlocal() == tz.tzoffset("GMT", 0))'
False

@pganssle
Copy link
Member

pganssle commented Jun 12, 2019

@koobs Interesting, so it seems like the issue is in the way equality between tzlocal and tzoffset is defined, which is here.

How about this:

from dateutil import tz
local = tz.tzlocal()
print(local._hasdst)
print(local._tznames[0])
print(local._std_offset)

Can you run that with TZ='GMT'. I imagine any version of dateutil where you're having this problem will be fine.

By the way, do you know of an easy way for me to get temporary access on a system like yours that is having this problem? It would probably be easier to debug than the high-latency method of "give you some debugging code and see how it works on your system" 😉.

@koobs
Copy link
Contributor Author

koobs commented Jun 13, 2019

@pganssle Flick me a ssh pubkey and I'll create you an account on the machine reproducing the issue

For the last high-latency debug request:

[koobs@CURRENT-amd64:/usr/home/koobs/repos/dateutil] PYTHONPATH=`pwd` TZ='GMT' python2.7
Python 2.7.16 (default, May 28 2019, 22:56:37)
[GCC 4.2.1 Compatible FreeBSD Clang 8.0.0 (tags/RELEASE_800/final 356365)] on freebsd13
Type "help", "copyright", "credits" or "license" for more information.
>>> from dateutil import tz
>>> local = tz.tzlocal()
>>> print(local._hasdst)
False
>>> print(local._tznames[0])
UTC
>>> print(local._std_offset)
0:00:00
>>>

@pganssle
Copy link
Member

@koobs Sorry, I forgot to respond (I obviously lost interest as soon as we figured out the answer). I'm guessing that your system for some reason interprets "GMT" as equal to "UTC", possibly with some symlink to something like /etc/zoneinfo/UTC or something. The code here is assuming that TZ='GMT' will have tzname set to GMT rather than UTC.

I'm wondering if this is actually a violation of the POSIX standard on your OS's part, but we may as well find some workaround for it. One option might be to change that test to use 'UTC' instead of 'GMT'. A shorter-term possible fix might be to track down if this is something you can easily configure by adjusting your zoneinfo files to some more standard configuration. If we do determine it's a violation of the POSIX standard it might be best to also report this to your OS provider.

@koobs
Copy link
Contributor Author

koobs commented Jun 17, 2019

@pganssle Analysis follows, thank you @RhodiumToad for your help digging/groking what we found:

Your comment "GMT as equal to UTC", was close, but not quite the cause. It's not that GMT was equal to UTC, it was that "any non-present TZ name is the default timezone name". In FreeBSD's case ...

Change localtime default from GMT -> UTC
https://svnweb.freebsd.org/changeset/base/130332

Original thread on the change:
https://lists.freebsd.org/pipermail/freebsd-standards/2004-June/000602.html

That change broke leapseconds because "If /usr/share/zoneinfo/GMT is absent, UTC leap seconds are loaded from /usr/share/zoneinfo/posixrules." until ...

Change tzdata default from GMT -> UTC
https://svnweb.freebsd.org/changeset/base/199405

But didn't also create a Link Etc/GMT GMT

POSIX does not recognize TZ=GMT or TZ=UTC as valid values because they neither start with a colon nor contain a numeric offset hour field:

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html

The default (upstream) tzdata comes with Etc/GMT and Etc/UTC, plus by default a link for GMT, plus a link for UTC if the "backward" file is installed, so UTC can't be relied upon in dateutil as a "works in all cases"
default.

We can take care of FreeBSD adding back a default GMT entry, which will take some time to make it into users hands for all supported releases.

I would strongly suggest changes to dateutil to make it more robust at least to this particular class of intended behaviour, namely: Any TZ name expected to exist (explicitly), but doesn't, defaults to the
default timezone and name, which may not be what you expect.

Certainly, it does not necessarily have the same name, nor can or should that be asserted in tests, implicitly or otherwise.

@pganssle
Copy link
Member

@koobs Thanks for getting to the bottom of that! That makes this way easier to solve from dateutil's perspective, since we can just change the test to: with TzEnvContext("GMT0"):.

And also luckily this is just an invalid assumption in the test, not an invalid assumption in the software itself.

@koobs
Copy link
Contributor Author

koobs commented Jun 17, 2019

@pganssle My pleasure. I can get it to pass with the following diff:

--- dateutil/test/test_tz.py.orig       2019-02-05 13:50:00 UTC
+++ dateutil/test/test_tz.py
@@ -998,7 +998,7 @@ def test_tzlocal_local_time_trim_colon():
 @mark_tzlocal_nix
 @pytest.mark.parametrize('tzvar, tzoff', [
     ('EST5', tz.tzoffset('EST', -18000)),
-    ('GMT', tz.tzoffset('GMT', 0)),
+    ('GMT0', tz.tzoffset('GMT', 0)),
     ('YAKT-9', tz.tzoffset('YAKT', timedelta(hours=9))),
     ('JST-9', tz.tzoffset('JST', timedelta(hours=9))),
 ])
dateutil/test/test_tz.py::test_tzlocal_offset_equal[EST5-tzoff0] PASSED                                                                                                                                                                                                                                            [ 25%]
dateutil/test/test_tz.py::test_tzlocal_offset_equal[GMT0-tzoff1] PASSED                                                                                                                                                                                                                                            [ 50%]
dateutil/test/test_tz.py::test_tzlocal_offset_equal[YAKT-9-tzoff2] PASSED                                                                                                                                                                                                                                          [ 75%]
dateutil/test/test_tz.py::test_tzlocal_offset_equal[JST-9-tzoff3] PASSED          

Is that the correct, full and complete change necessary?

@pganssle
Copy link
Member

@koobs Yes, that looks good to me, if you would like to make a PR. If you don't mind, could you also add the line:

    ('UTC', tz.tzoffset('UTC', 0)),

I think that it would be good to have a TZ environment variable that is not of the format XXX#, and the POSIX standard specifically says that missing time zones fall back to UTC, so I think that should be safe.

@koobs
Copy link
Contributor Author

koobs commented Jun 18, 2019

@pganssle Can do

@koobs
Copy link
Contributor Author

koobs commented Jun 18, 2019

I'm getting a test_tzoffset_weakref failure with that UTC TZ addition:

test_tzoffset_weakref

    @pytest.mark.smoke
    @pytest.mark.tzoffset
    def test_tzoffset_weakref():
        UTC1 = tz.tzoffset('UTC', 0)
        UTC_ref = weakref.ref(tz.tzoffset('UTC', 0))
        UTC1 is UTC_ref()
        del UTC1
        gc.collect()

        assert UTC_ref() is not None    # Should be in the strong cache
        assert UTC_ref() is tz.tzoffset('UTC', 0)

        # Fill the strong cache with other items
        for offset in range(5,15):
            tz.tzoffset('RandomZone', offset)

        gc.collect()
>       assert UTC_ref() is  None
E       AssertionError: assert tzoffset(u'UTC', 0) is None
E        +  where tzoffset(u'UTC', 0) = <weakref at 0x805040100; to 'tzoffset' at 0x803a3a510>()

dateutil/test/test_tz.py:757: AssertionError

@koobs
Copy link
Contributor Author

koobs commented Jun 18, 2019

Shall we just leave this as a fix to the reported (GMT) issue?

@pganssle
Copy link
Member

@koobs Yeah, we can save that for a separate issue.

@koobs
Copy link
Contributor Author

koobs commented Jun 19, 2019

I'll create a PR at some point this weekend. Thank you for the support @pganssle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants