-
Notifications
You must be signed in to change notification settings - Fork 586
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
Store reference to time.perf_counter for gc accounting #4057
Conversation
@@ -421,6 +421,10 @@ def find(self, condition: Callable[[T], bool]) -> T: | |||
_gc_start = 0 | |||
_gc_cumulative_time = 0 | |||
|
|||
# Since gc_callback potentially runs in test context, and perf_counter | |||
# might be monkeypatched, we store a reference to the real one. | |||
_perf_counter = time.perf_counter |
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.
from time import perf_counter
would of course be possible, but I think it's wise to make the purpose more obvious by an explicit assignment.
On module level, because we want to patch it in our self-tests.
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.
Looks good, thanks @jobh!
This patch addresses the issue of hypothesis potentially accessing | ||
mocked ``time.perf_counter`` during test execution (:issue:`4051`). |
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 think this is technically too general a claim, because we also use perf_counter()
when drawing from a strategy and that can happen inline - e.g. via st.functions()
or st.data()
. But being super-precise here that it addresses some cases doesn't seem like a big deal.
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.
A case for centralizing and replacing import time
with from hypothesis.internal import time
perhaps? Later, anyway.
Closes #4051.