-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Use functools.lru_cache with _getconftest_pathlist #4227
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
Use functools.lru_cache with _getconftest_pathlist #4227
Conversation
For pytest's own suite the `cache_info()` looks as follows: > session.config._getconftest_pathlist.cache_info() CacheInfo(hits=231, misses=19, maxsize=None, currsize=19) While it does not really make a difference for me this might help with larger test suites / the case mentioned in pytest-dev#2206 (comment).
(Moving the discussion here) Yep. It helps, but there is some other big issue that overshadows this. |
Any objections to applying this only for PY3? |
No. |
Done! 👍 |
Codecov Report
@@ Coverage Diff @@
## features #4227 +/- ##
============================================
+ Coverage 95.82% 95.82% +<.01%
============================================
Files 109 109
Lines 24236 24313 +77
Branches 2390 2401 +11
============================================
+ Hits 23224 23298 +74
- Misses 718 719 +1
- Partials 294 296 +2
Continue to review full report at Codecov.
|
@@ -907,6 +908,10 @@ def _getconftest_pathlist(self, name, path): | |||
values.append(relroot) | |||
return values | |||
|
|||
if six.PY3: | |||
# once we drop Python 2, please change this to use the normal decorator syntax (#4227) | |||
_getconftest_pathlist = functools.lru_cache(maxsize=None)(_getconftest_pathlist) |
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.
Old school style! 😎
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.
\o/
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.
btw
if six.PY3:
is going to break when python4 comes out -- usually prefer if six.PY2: ...
/ if not six.PY2: ...
I've seen other projects do this in a different way:
if six.PY2:
def lru_cache(*_, **__):
def dec(fn):
return fn
return dec
else:
from functools import lru_cache
neat trick though (saw this from twitter, missed the PR notification)
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.
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.
All fair points. I don't mind leaving the code as is, and also don't mind to change to the ones suggested here too. 👍
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.
Will move it to compat now.
For pytest's own suite the
cache_info()
looks as follows:While it does not really make a difference for me this might help with
larger test suites / the case mentioned in
#2206 (comment).
/cc @boxed