-
Notifications
You must be signed in to change notification settings - Fork 269
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
Comments
Oh, and Freezegun's fake date/time lacks milliseconds compared to the default update. I've tested on Python 3.4.2. |
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'm also quite worried about how the documentation will read. |
Staying completely backwards-compatible probably won't work.
I'd say this should be the local time. If it should be UTC, either a separate function with some
Passing an optional timezone via a keywords argument should be possible. If that is the case and the time value already specifies timezone, a |
I agree it should be the local time. Caller can specify UTC as the timezone if they want that. |
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. |
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
Whatcha think @spulec? |
@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 |
Yeah - people will be upset either way. I think it boils down to which side of the divide is larger? Folks who use Either way - we can respect the system timezone fairly simply with: import time
time.tzname If you provide The tricky part is naive |
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 @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 |
Ah - that may be - thanks for the heads up! I wasn't even aware of I'm traveling this week but I'll slap together a PR when I'm back. |
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: |
Woah @homeworkprod - the more you know! |
@bryanhelmig, you're welcome. :) |
Is this project still maintained? It would be good to get something merged for this. |
Yes, open to a PR with a clean merge and tests passing. |
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. |
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. |
@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 |
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 I personally believe that the system timezone should be respected even when time is frozen. If that's not deemed appropriate I guess By the way, the |
@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:
...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. |
Hey @boxed.
I'd be interested in more details about that. We use Any thoughts about allowing a |
@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. |
@boxed out of curiosity, was it taking 8 minutes with |
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 |
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? |
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. |
I'm fine with not considering the system time zone, yes. I expect the requested fake date to be reflected both by
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 Sorry in case my post is a bit confusing. My impression is that we basically agree. |
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).
It seems so. That's pretty good news! :P |
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. |
Good. Then you now think that the example you started this issue with is in fact what you would expect? |
Yeah, it starts to sink in. Let me try this change: Given I would have explicitly specified |
Cool. Then we are all in agreement what should happen at least. |
Can't agree more! |
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? |
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 EDIT: I get the following error running the above.
|
@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. |
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. |
I've been experimenting a bit, and I'm a little bit confused about the idea of test case
Why should the latter, given that 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. |
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? |
Here is a doodle sjlehtin@8237820, look like something worth taking forward? @boxed |
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:
Output without Freezegun (system timezone is "Europe/Berlin"):
Output with Freezegun (method decorated with
@freeze_time('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.
The text was updated successfully, but these errors were encountered: