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

Faked time shouldn't always be the same. #89

Open
homeworkprod opened this issue Mar 9, 2015 · 43 comments
Open

Faked time shouldn't always be the same. #89

homeworkprod opened this issue Mar 9, 2015 · 43 comments

Comments

@homeworkprod
Copy link

Excerpt from the docs:
"[…] all calls to datetime.datetime.now(), datetime.datetime.utcnow(), […] will return the time that has been frozen."

But this is not the behaviour I expect.

Example code:

from datetime import datetime

def test_something():
    print('now():   ', datetime.now())
    print('utcnow():', datetime.utcnow())

Output without Freezegun (system timezone is "Europe/Berlin"):

now():    2015-03-10 00:46:40.465879
utcnow(): 2015-03-09 23:46:40.466031

Output with Freezegun (method decorated with @freeze_time('2012-03-11 19:21:23')):

now():    2012-03-11 19:21:23
utcnow(): 2012-03-11 19:21:23

However, I expected the output with Freezegun to reflect the offset (one hour, in this case) and, thus, to return two different date/time instances.

@homeworkprod
Copy link
Author

Oh, and Freezegun's fake date/time lacks milliseconds compared to the default update. I've tested on Python 3.4.2.

@spulec
Copy link
Owner

spulec commented Mar 14, 2015

Thank you for opening this. Your point is very valid. I would like to fix this somehow, but have a few thoughts/questions I'm trying to think through. Any feedback appreciated.

  • I think we would need to have some sort of backwards-compatibility. No idea how this would work.
  • Should the time passed into freezegun be used as the local time or utc time?
  • What if the time passed in has a timezone as well or a tz_offset is passed?

I'm also quite worried about how the documentation will read.

@homeworkprod
Copy link
Author

I think we would need to have some sort of backwards-compatibility. No idea how this would work.

Staying completely backwards-compatible probably won't work.

Should the time passed into freezegun be used as the local time or utc time?

I'd say this should be the local time. If it should be UTC, either a separate function with some _utc suffix should be provided (just like datetime.now() and datetime.utcnow() in the stdlib), or a keyword argument like utc=True or similar could be used.

What if the time passed in has a timezone as well or a tz_offset is passed?

Passing an optional timezone via a keywords argument should be possible. If that is the case and the time value already specifies timezone, a ValueError could be raised.

@agriffis
Copy link
Contributor

agriffis commented Jan 4, 2016

I agree it should be the local time. Caller can specify UTC as the timezone if they want that.

@agriffis
Copy link
Contributor

agriffis commented Jan 4, 2016

Actually, it should probably default to UTC, so that developers in different timezones working on the same project won't see different results in their tests.

@bryanhelmig
Copy link

We ran into this in the wild and fought it for a good while - I have some thoughts on how we might fix and document it. @spulec I'd be happy to fix this in a PR if you could confirm there is a good chance it'll merge.

By "past behavior" I mean now() ~= utcnow() which is the bug. By "future behavior" I mean now() and utcnow() behave correctly when frozen.

  1. For backwards compatibility - an opt-in switch (both global & kwarg) that enables future behavior. We log a single warning if utcnow() is used in under freezegun. The warning can have a link to a full doc page. In a pre-decided future version (say when we go 1.0.0) we drop the warning and drop the bug.
  2. Presumption of local vs. utc and naivety - I think the time passed in should be presumed "local" if naive. We have to decide one way or another and disappoint someone. If utc default is undesirable (because you use now() in code) - the workaround depends on your (and team members') timezone. If local default is undesirable (because you use utcnow() in code) it seems simpler to document a workaround that says "add -00:00 to your freeze argument".
  3. If TZ is supplied (which is the presumption of point 2) - you'd have to hold onto that timezone and convert to system local and UTC for both now() and utcnow().

Whatcha think @spulec?

@spulec
Copy link
Owner

spulec commented Jan 24, 2016

@bryanhelmig happy to accept a fix.

Agree with thoughts on 1 and 3.

2 makes me a bit nervous. I fear that this would force the majority of users to change their code (that is just a hunch). If we wanted to keep utc default, we could add a local argument to override it. As you noted, we will make people upset either way.

@bryanhelmig
Copy link

Yeah - people will be upset either way. I think it boils down to which side of the divide is larger? Folks who use now() exclusively? Or folks who use utcnow() exclusively?

Either way - we can respect the system timezone fairly simply with:

import time
time.tzname

If you provide freeze_time('2015-01-16T20:00:00+00:00') or any other time with a timezone - code should work either way.

The tricky part is naive freeze_time('2015-01-16T20:00:00'). People 100% have to workaround this bug now if you mix now() and utcnow() as it diverges - so fixing it means we 100% will break stuff. So, just gotta break stuff in the right direction. :-)

@spulec
Copy link
Owner

spulec commented Jan 25, 2016

One thing I didn't realize until now is that our existing support for tz offsets already makes the assumption that we are passing the time for utc use. From the README:

@freeze_time("2012-01-14 03:21:34", tz_offset=-4)
def test():
    assert datetime.datetime.utcnow() == datetime.datetime(2012, 01, 14, 03, 21, 34)
    assert datetime.datetime.now() == datetime.datetime(2012, 01, 13, 23, 21, 34)

If we wanted to keep this all working, would the fix be a matter of setting tz_offset to default to time.timezone / 3600?

@bryanhelmig
Copy link

Ah - that may be - thanks for the heads up!

I wasn't even aware of tz_offset - so if the datetime string has a TZ in it too, feels like we could set that to tz_offset as well which is more natural and smooths some edges.

I'm traveling this week but I'll slap together a PR when I'm back.

@homeworkprod
Copy link
Author

@freeze_time("2012-01-14 03:21:34", tz_offset=-4)
def test():
    assert datetime.datetime.utcnow() == datetime.datetime(2012, 01, 14, 03, 21, 34)
    assert datetime.datetime.now() == datetime.datetime(2012, 01, 13, 23, 21, 34)

BTW, did you notice that the above code uses octal values? This works for integers up to 7, but will fail for 8 and 9 (see github-linguist/linguist#1939 on why the syntax highlighting of the console output currently doesn't work):

>>> 07
7
>>> 08
  File "<input>", line 1
    08
     ^
SyntaxError: invalid token

See here:

@bryanhelmig
Copy link

Woah @homeworkprod - the more you know!

@homeworkprod
Copy link
Author

@bryanhelmig, you're welcome. :)

@bitdivision
Copy link

Is this project still maintained? It would be good to get something merged for this.

@spulec
Copy link
Owner

spulec commented May 13, 2017

Yes, open to a PR with a clean merge and tests passing.

@tsiq-dastle
Copy link

Our solution to this is to use Arrow.utcnow(), which correctly does timezone conversion using the local system time. I'd be willing to put a PR for this, but not sure you guys are comfortable with adding another dependency in requirement.txt.

@boxed
Copy link
Contributor

boxed commented Sep 10, 2018

I disagree with the premise at the start ofthis ticket. It's MUCH nicer if freezegun locks timezone to UTC. The entire reason we use freezegun is to make time predictable in tests, so having it freezing the time but not the timezone would be a big mistake. This is a documentation issue.

@WhyNotHugo
Copy link

@boxed I disagree with this.

If the TZ is locked to UTC, it suddently becomes impossible for me to run tests under different TZ settings. For calendaring apps and alike (which is where I use freezetime), these tests are critical to find issues that only ocurr under certain scenarios.

If you need to lock the TZ to a specific one, having a second argument to @freeze_time would probably be the best compromise.

@boxed
Copy link
Contributor

boxed commented Sep 10, 2018

The tz_offset argument isn't enough for you?

If you want to run with the local timezone the right thing to do would be to freeze_time with the existing timezone imo. Just like if you want to lock the current time you lock to datetime.now().

@boxed
Copy link
Contributor

boxed commented Sep 29, 2018

This ticket is also in direct conflict with #204 as far as I can tell. Users seem to expect the time zone to be locked to UTC by default. I would and so does @azmeuk .

@boxed
Copy link
Contributor

boxed commented Sep 30, 2018

I've been trying to make a PR to fix some issues from @azmeuk . It seems all of #205
#204
#209
#208
#240

and this ticket are basically the same underlying issue. The tickets should be probably be merged.

@charettes
Copy link

Users seem to expect the time zone to be locked to UTC by default.

@boxed I personally believe that the system timezone should be respected even when time is frozen. If that's not deemed appropriate I guess freeze_time could be adapted to accept a tzinfo object that would be used to localize dates and times. If users really expect the simulated system timezone to change to UTC on time freezing this tzinfo kwarg could default to UTC.

By the way, the tzoffset argument should really be deprecated as it makes little sense in the marvellous world of DST and timezone rules changing every now and then . A good deprecation path would be to error out if both tzoffset and tzinfo are provided and convert tzoffset to a fixed offset tzinfo while raising a warning during the deprecation period.

@boxed
Copy link
Contributor

boxed commented Sep 30, 2018

@charettes For me the point of freeze_time is that you create a world where time related things are predictable and portable across machines. Leaving the time zone as is will not satisfy this demand.

I'm all for having a nicer way to freeze but still respect the time zone if that's what you want though. I'd also like a better way to "freeze" and still respect the local time! This is super useful to initialize the monkey patching just once for your entire test suite for example. We do this at work (and we use my fork that is also an active PR) and it has some pretty dramatic performance advantages if you freeze time a lot.

I'd suggest this:

  • freeze_time('local', tzinfo='local', tick=True) for respecting time zone, local time and keeping the clock moving
  • freeze_time('2001-01-02', tzinfo='local') for respecting local time zone but locking the rest
  • freeze_time('2001-01-02') for locking everything

...as for tzinfo vs tzoffset I'm all for it. The only problem I see with it is that if you specify a time zone by name that can mean something different at some later point. tzoffset doesn't have this problem, but it also doesn't model DST properly, so I think maybe a third option is needed here? In addition to your suggestion that is.

@charettes
Copy link

Hey @boxed.

We do this at work (and we use my fork that is also an active PR) and it has some pretty dramatic performance advantages if you freeze time a lot.

I'd be interested in more details about that. We use freeze_time quite extensively and have noticed significant speedups since we moved to 0.3.10 which includes #175.

Any thoughts about allowing a datetime.datetime.tzinfo object to be passed instead of a string for tzinfo? That would make testing against a non-local but determined timezone way more reliable than tzoffset.

@boxed
Copy link
Contributor

boxed commented Sep 30, 2018

@charettes You should try my fork! https://github.com/boxed/freezegun/ We freeze time on all tests by default (see also https://medium.com/@boxed/flaky-tests-part-3-freeze-the-world-e4929a0da00e). Our unit test suite took 8 minutes when I introduced this freeze-everything. My fork puts the time back to the ~20 seconds it takes without freezing time at all. Initializing freezegun just once is crucial though since monkey patching back and forth is pretty expensive. That wasn't the biggest performance issue, but it was one we had to fix too.

@charettes
Copy link

@boxed out of curiosity, was it taking 8 minutes with 0.3.10 as well?

@boxed
Copy link
Contributor

boxed commented Sep 30, 2018

Seems it takes ~2.2 minutes compared to with my fork 20.2 seconds. So better than 8 for sure, but still pretty bad I'd say. Give my fork a try! And if it works as well for you, give a shout out in my PR :P

@homeworkprod
Copy link
Author

I disagree with the premise at the start ofthis ticket.

Really?

My main point is that the faked times returned by Freezegun should not be the same for UTC and time zones with an offset against UTC.

Are you saying basically ignoring the time zone offset (in whatever direction) is a good thing?

@boxed
Copy link
Contributor

boxed commented Sep 30, 2018

Are you saying basically ignoring the time zone offset (in whatever direction) is a good thing?

The system time zone? Absolutely. I would even consider it a critical bug if it did not (which it doesn't currently in a bunch of cases). The point of freezegun is to not respect the system time. Time zones are a part of the system time.

You should not be able to accidentally write tests, when using freezegun, that pass on one machine and then does not pass on another.

If you do want to write tests dealing with time zones other than UTC you should freeze to that time zone. I realize that this is currently rather broken, but we should fix that and not break freezegun more.

@homeworkprod
Copy link
Author

homeworkprod commented Sep 30, 2018

I'm fine with not considering the system time zone, yes.

I expect the requested fake date to be reflected both by now() and utcnow() (with the appropriate offset between them).

If you do want to write tests dealing with time zones other than UTC you should freeze to that time zone.

That's what I would do, yes.

I don't know how Freezegun's behavior is influenced by the system time zone, and I think I agree that it shouldn't be.

Regarding the code in my initial post: I have only tested on a single machine with a single (the system) time zone. Maybe the results (in offset between now() and utcnow()) are different with other system time zones (or daylight saving time) or other situations. But the values being equal is definitely wrong in that situation.

Sorry in case my post is a bit confusing. My impression is that we basically agree.

@boxed
Copy link
Contributor

boxed commented Sep 30, 2018

I expect the requested fake date to be reflected both by now() and utcnow() (with the appropriate offset between them).

But what would the appropriate offset be if a timezone is not specified? To me it seems there are only two alternatives when you haven't specified a time zone: UTC or -13 (a time zone that is uninhabited!). Currently it's UTC (or it's broken, but let's ignore that for now).

Sorry in case my post is a bit confusing. My impression is that we basically agree.

It seems so. That's pretty good news! :P

@homeworkprod
Copy link
Author

UTC seems like the single reasonable default to me to avoid unexpected results on different systems.

Requiring the user to specify a zone is probably a bit too much.

@boxed
Copy link
Contributor

boxed commented Sep 30, 2018

Good. Then you now think that the example you started this issue with is in fact what you would expect?

@homeworkprod
Copy link
Author

Yeah, it starts to sink in.

Let me try this change: Given I would have explicitly specified Europe/Berlin as the zone as part of my test/Freezegun setup (rather expecting that zone to be considered because it's the system default), then I would expect the results I've described in my 3.5 years old initial post.

@boxed
Copy link
Contributor

boxed commented Sep 30, 2018

Cool. Then we are all in agreement what should happen at least.

@azmeuk
Copy link
Contributor

azmeuk commented Oct 1, 2018

For me the point of freeze_time is that you create a world where time related things are predictable and portable across machines. Leaving the time zone as is will not satisfy this demand.

Can't agree more!

@ccrisan
Copy link

ccrisan commented Dec 25, 2020

So how are we supposed to test code that travels in time in a certain timezone which is not UTC (and has DST)?

Supposing I have a function that returns the Unix timestamp of the beginning of the sixth month from now (1st, 00:00:00), localtime. How would I test it? More specifically, how would I test that it actually takes localtime into account?

@sjlehtin
Copy link

sjlehtin commented Jan 28, 2021

with freeze_time(datetime.datetime(year=2020, month=1, day=1,
                                       microsecond=400000),
                     tz_offset=4) as frozen:
        dt1 = datetime.datetime.now(tz=datetime.timezone.utc)
        dt2 = datetime.datetime.utcnow().replace(tzinfo=datetime.timezone.utc)
        assert dt1 == dt2

I assume the above assertion should hold, but it does not. Considering how long the original issue has been open a quick resolution does not seem likely, but could the now() take the tz given as argument into account somehow? Is this discussion ongoing in some other venues?

EDIT:

I get the following error running the above.

Expected :FakeDatetime(2020, 1, 1, 0, 0, 0, 400000, tzinfo=datetime.timezone.utc)
Actual   :FakeDatetime(2020, 1, 1, 4, 0, 0, 400000, tzinfo=datetime.timezone.utc)
<Click to see difference>

def test_freezegun():
        with freeze_time(datetime.datetime(year=2020, month=1, day=1,
                                           microsecond=400000),
                         tz_offset=4) as frozen:
            dt1 = datetime.datetime.now(tz=datetime.timezone.utc)
            dt2 = datetime.datetime.utcnow().replace(tzinfo=datetime.timezone.utc)
>           assert dt1 == dt2
E           assert FakeDatetime(2020, 1, 1, 4, 0, 0, 400000, tzinfo=datetime.timezone.utc) == FakeDatetime(2020, 1, 1, 0, 0, 0, 400000, tzinfo=datetime.timezone.utc)

tests/test_logging.py:25: AssertionError

@boxed
Copy link
Contributor

boxed commented Jan 28, 2021

@sjlehtin The problem is that no one has implemented what we decided. We do welcome PRs though if you want to give it a shot.

@sjlehtin
Copy link

From this thread we can get some test cases on how it should work, and from the other duplicates as well. I guess no one actually did a full summary of the related issues?

I guess I can give this a shot as I need to work around this right now.

@sjlehtin
Copy link

I've been experimenting a bit, and I'm a little bit confused about the idea of test case test_datetime_timezone_real_with_offset:

def test_datetime_timezone_real():
    now = datetime.datetime.now(tz=GMT5())
    assert now == datetime.datetime(2012, 1, 14, 7, tzinfo=GMT5())
    assert now.utcoffset() == timedelta(0, 60 * 60 * 5)


@freeze_time("2012-01-14 2:00:00", tz_offset=-4)
def test_datetime_timezone_real_with_offset():
    now = datetime.datetime.now(tz=GMT5())
    assert now == datetime.datetime(2012, 1, 14, 3, tzinfo=GMT5())
    assert now.utcoffset() == timedelta(0, 60 * 60 * 5)

Why should the latter, given that datetime.datetime.now() is passed real tz object, care about the tz_offset?

Elsewhere, freeze_time is treated as UTC and tz_offset the offset from that (UTC). Now, in this case, tz_offset and the given TZ are combined to arrive at a different value for the UTC time.

@sjlehtin
Copy link

I think I'm hitting the gist of @homeworkprod and @boxed have been talking in this thread. What was the earlier decision and did you have some test cases or examples to make those intuitions concrete?

@sjlehtin
Copy link

Here is a doodle sjlehtin@8237820, look like something worth taking forward? @boxed

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

No branches or pull requests