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

make sure fromtimestamp always return FakeDateTime #475

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

Koswu
Copy link
Contributor

@Koswu Koswu commented Oct 27, 2022

This fix #344

This is my first PR, if something goes wrong, please let me know, PTAL

@boxed
Copy link
Contributor

boxed commented Oct 27, 2022

The proposed code change seems to be no change of behavior. Could you point out what it changes?

@Koswu
Copy link
Contributor Author

Koswu commented Oct 27, 2022

In the fromtimestamp method, when the tz parameter is None, It will return the real datetime object. This object's .timestamp() behavior will depend on the system timezone, even it tzinfo is None. For example:

# my system timezone is UTC+8
>>> dt = datetime.datetime(1970, 1, 1, 8, 0, 1)
>>> dt.tzinfo
None
>>> dt.timestamp()
1.0

However, the fake one depend on the freeze time zone:

>>> freezer = freeze_time(datetime.datetime(1970, 1, 1, 0, 0, 1), tz_offset=-1)
>>> freezer.start()
>>> datetime.datetime(1970, 1, 1, 8, 0, 1).timestamp()
32401.0

So if we make these steps:

  1. make a fake datetime object
  2. convert it to timestamp
  3. convert it to datetime
  4. convert it to timestamp

we will have a type chain like this:

FakeDatetime -> int -> RealDatetime -> int

Since we know there are different behavior between FakeDatetime.timestamp() and RealDatetime.timestamp(), it will cause problem that two timestamp in the type chain are not equal.

That's why I added the test.

freezegun/api.py Outdated
tz = dateutil.tz.tzoffset("freezegun", cls._tz_offset())
result = real_datetime.fromtimestamp(t, tz=tz).replace(tzinfo=None)
else:
result = datetime_to_fakedatetime(real_datetime.fromtimestamp(t, tz))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result = datetime_to_fakedatetime(real_datetime.fromtimestamp(t, tz))
result = real_datetime.fromtimestamp(t, tz)

The datetime_to_fakedatetime already happens at the end - no need to duplicate this call.

Happy to merge once that's fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed. thanks for pointing this out.

Copy link
Collaborator

@bblommers bblommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great - thanks for the fix @Koswu!

@bblommers bblommers merged commit c65f4db into spulec:master Mar 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using tz_offset breaks t == fromtimestamp(t).timestamp()
3 participants