-
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
Handle rest_framework.throttling.SimpleRateThrottle #459
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
""" | ||
Lazy module aims to reproduce the context of late loaded modules | ||
Eg. testing a django view using the test client | ||
""" | ||
|
||
def load(): | ||
import time | ||
|
||
class TimeAsClassAttribute: | ||
""" | ||
Reproduce the behaviour of: rest_framework.throttling.SimpleRateThrottle | ||
see: https://github.com/encode/django-rest-framework/blob/3.13.1/rest_framework/throttling.py#L63 | ||
""" | ||
_time = time.time | ||
|
||
def call_time(self): | ||
return self._time() | ||
|
||
globals()["TimeAsClassAttribute"] = TimeAsClassAttribute |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,8 @@ | ||
import time | ||
import sys | ||
|
||
import pytest | ||
|
||
from .fake_module import ( | ||
fake_date_function, | ||
fake_datetime_function, | ||
|
@@ -9,6 +12,7 @@ | |
fake_time_function, | ||
) | ||
from . import fake_module | ||
from . import fake_module_lazy | ||
from freezegun import freeze_time | ||
from freezegun.api import ( | ||
FakeDatetime, | ||
|
@@ -130,6 +134,17 @@ def test_fake_strftime_function(): | |
assert fake_strftime_function() == '2012' | ||
|
||
|
||
@freeze_time("2022-01-01") | ||
def test_fake_time_function_as_class_attribute(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is great that you added tests, but it is also important to explain on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can also add some description of this change on |
||
local_time = datetime.datetime(2022, 1, 1) | ||
utc_time = local_time - datetime.timedelta(seconds=time.timezone) | ||
expected_timestamp = time.mktime(utc_time.timetuple()) | ||
JulianMaurin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
fake_module_lazy.load() | ||
|
||
assert fake_module_lazy.TimeAsClassAttribute().call_time() == expected_timestamp | ||
|
||
|
||
def test_import_after_start(): | ||
with freeze_time('2012-01-14'): | ||
assert 'tests.another_module' not in sys.modules.keys() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why *args, **kwargs here? That seems very wrong to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @boxed ,
Thanks for your review. May I humbly ask you to elaborate?
I think I made an effort to explain the reproduction of the bug, both in the description and in the test.
If I'm off track, can you possibly guide me to a solution that you think is more appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JulianMaurin this is breaking the API typing on
api.pyi
, so users that have mypy will have problems. I also think you can add typed arguments to satisfy your needs with defaultarguments=None
so it will also not break current applications and maintain the types well defined.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this version does not break the application, in addition,
fake_time
function is not defined inapi.pyi
, but maybe I did not understand something.Would you prefer this version (with a docstring ofc):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JulianMaurin putting an argument of type
Any
makes the method still untyped. Also as @boxed adding*args, **kwargs
here does not make sense to me also, since to fix the bug you one need to be able to call time it withself
.You can check in the definition of builtin
time.time
there is NO argument whatsoever: https://github.com/python/cpython/blob/v3.9.6/Modules/timemodule.c#L86Have to tried, instead of adding arguments, wrapping fake_time with @classmethod? Maybe it will fix this usage. If not I am sure some kind of wrapper like this will be needed.