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.

Sign up for GitHub

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

Jump to bottom

fix ignoring packages that are imported after freezing #430

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix ignoring packages that are imported after freezing #430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions freezegun/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,11 @@ def __sub__(self, other):

@classmethod
def today(cls):
result = cls._date_to_freeze() + cls._tz_offset()
if _should_use_real_time():
result = real_date.today() + cls._tz_offset()
else:
result = cls._date_to_freeze() + cls._tz_offset()
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this added logic is for. cls._date_to_freeze() calls get_current_time() which already calls _should_use_real_time() and does this type of thing so this change seems redundant?

freezegun/freezegun/api.py

Line 174 in 92624e4

if _should_use_real_time():

Copy link
Contributor

Choose a reason for hiding this comment

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

@boxed I'm happy to submit my change which should be smaller if that would make it easier to get this in.

I could add the test case from this PR to make sure it covers the issue mentioned in this PR, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

@immerrr that looks wrong to me too honestly. It's too complex. Something is fishy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure we are talking about the same thing, you mean my patch that touches these two conditionals is too complex, right?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Correct. I feel like this logic should be fully contained within get_current_time itself. It just wouldn't be DRY otherwise, and it would also mean there's a ton of duplicate code and every place there isn't duplication would be a bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are also performance considerations here, e.g. some blocks like

if _should_use_real_time():
    return call_some_builtin_function()

may work much faster than converting the result of get_current_time() back to the required format.

DRY is a valid principle, but repetition only manifests itself if the code needs to change. If it is a small module that is feature complete, what difference does it make if the code repeats one time, two times or ten times, the interpreter doesn't care.

That said, let me see if I can hide should_use_real_time inside get_current_time...

Copy link
Contributor

@boxed boxed Mar 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

may work much faster than converting the result of get_current_time() back to the required format.

There is no conversion in this code though.

That said, let me see if I can hide should_use_real_time inside get_current_time..

...but get_current_time already calls should_use_real_time! That's my point.

Copy link
Contributor

@immerrr immerrr Mar 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is no conversion in this code though.

There are conversions in functions like fake_monotonic whose "faking" versions use get_current_time

freezegun/freezegun/api.py

Lines 221 to 225 in be4127b

def fake_monotonic():
if _should_use_real_time():
return real_monotonic()
return _get_fake_monotonic()

So if we continue with the DRY idea of moving _should_use_real_time inside get_current_time, functions like fake_monotonic will look like

def fake_monotonic():
    current_time = get_current_time()
    return (
        calendar.timegm(current_time.timetuple()) +
        current_time.microsecond / 1e6
    )

which means they would take a performance hit even in cases when are using the "real" function. Do you have some other idea in mind?

but get_current_time already calls should_use_real_time!

I don't think it does, or maybe i'm looking somewhere else

freezegun/freezegun/api.py

Lines 169 to 170 in be4127b

def get_current_time():
return freeze_factories[-1]()


return date_to_fakedate(result)

@staticmethod
Expand Down Expand Up @@ -383,7 +387,11 @@ def timestamp(self):

@classmethod
def now(cls, tz=None):
now = cls._time_to_freeze() or real_datetime.now()
if _should_use_real_time():
now = real_datetime.now()
else:
now = cls._time_to_freeze() or real_datetime.now()

if tz:
result = tz.fromutc(now.replace(tzinfo=tz)) + cls._tz_offset()
else:
Expand All @@ -407,7 +415,10 @@ def today(cls):

@classmethod
def utcnow(cls):
result = cls._time_to_freeze() or real_datetime.utcnow()
if _should_use_real_time():
result = real_datetime.utcnow()
else:
result = cls._time_to_freeze() or real_datetime.utcnow()
return datetime_to_fakedatetime(result)

@staticmethod
Expand Down
15 changes: 15 additions & 0 deletions tests/another_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,18 @@ def get_fake_gmtime():

def get_fake_strftime():
return fake_strftime


# Getters


def get_datetime_now():
return datetime.now()


def get_datetime_utcnow():
return datetime.utcnow()


def get_date_today():
return date.today()
23 changes: 22 additions & 1 deletion tests/test_class_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,28 @@ def test_import_after_start():
assert another_module.get_fake_localtime() is fake_localtime
assert another_module.get_fake_gmtime() is fake_gmtime
assert another_module.get_fake_strftime() is fake_strftime
del sys.modules['tests.another_module']

if "tests.another_module" in sys.modules:
del sys.modules["tests.another_module"]


def test_ignore_import_after_start():
with freeze_time("2012-01-14", ignore=["tests.another_module"]):
assert "tests.another_module" not in sys.modules.keys()
from tests import another_module

utcnow = another_module.get_datetime_utcnow()
assert utcnow != FakeDatetime(2012, 1, 14)

now = another_module.get_datetime_now()
assert now != FakeDatetime(2012, 1, 14)

today = another_module.get_date_today()
assert today != FakeDate(2012, 1, 14)

if "tests.another_module" in sys.modules:
del sys.modules["tests.another_module"]


def test_none_as_initial():
with freeze_time() as ft:
Expand Down