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

_getconftestmodules: use functools.lru_cache #4247

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Oct 26, 2018

Also renames _path2confmods to _dirpath2confmods for clarity (it is
expected to be a dirpath in _importconftest).

Merges master to avoid conflict with 63691f5 (which should have been done on features probably).

@nicoddemus
Copy link
Member

There's a simple failure related to renaming the private attribute

@nicoddemus
Copy link
Member

So the failing test on py27 is testing that we are caching _getconftestmodules. This patch makes the test pass:

diff --git a/src/_pytest/config/__init__.py b/src/_pytest/config/__init__.py
index 20c1e3ec..1124304a 100644
--- a/src/_pytest/config/__init__.py
+++ b/src/_pytest/config/__init__.py
@@ -395,6 +395,11 @@ class PytestPluginManager(PluginManager):
         else:
             directory = path

+        try:
+            return self._dirpath2confmods[directory]
+        except KeyError:
+            pass
+

But it kind of reduces the point of using lru_cache a bit, doesn't it?

I guess it is OK to apply the patch above (perhaps inside a if six.PY2 block to make it clear that is py2-only).

Also renames `_path2confmods` to `_dirpath2confmods` for clarity (it is
expected to be a dirpath in `_importconftest`).

Uses an explicit maxsize, since it appears to be only relevant for a
short period [1].

Removes the lru_cache on _getconftest_pathlist, which makes no
difference when caching _getconftestmodules, at least with the
performance test of 100x10 files (pytest-dev#4237).

1: pytest-dev#4237 (comment)
@blueyed
Copy link
Contributor Author

blueyed commented Oct 31, 2018

Thanks, added it with if six.PY2.

@blueyed blueyed requested a review from nicoddemus October 31, 2018 22:18
@codecov
Copy link

codecov bot commented Oct 31, 2018

Codecov Report

Merging #4247 into features will decrease coverage by 3.53%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           features   #4247      +/-   ##
===========================================
- Coverage     95.83%   92.3%   -3.54%     
===========================================
  Files           111     111              
  Lines         24777   24769       -8     
  Branches       2417    2414       -3     
===========================================
- Hits          23746   22862     -884     
- Misses          734    1512     +778     
- Partials        297     395      +98
Flag Coverage Δ
#docs ?
#doctesting ?
#linting ?
#linux 92.3% <100%> (-3.33%) ⬇️
#nobyte ?
#numpy ?
#pexpect ?
#py27 ?
#py34 92.1% <100%> (-0.07%) ⬇️
#py35 ?
#py36 92.09% <100%> (-1.85%) ⬇️
#py37 ?
#trial ?
#windows ?
#xdist ?
Impacted Files Coverage Δ
src/_pytest/config/__init__.py 94.63% <100%> (-0.37%) ⬇️
testing/test_conftest.py 99.61% <100%> (ø) ⬆️
src/_pytest/_code/_py2traceback.py 20% <0%> (-71.12%) ⬇️
testing/test_pdb.py 42.19% <0%> (-52.95%) ⬇️
src/_pytest/debugging.py 53.19% <0%> (-31.92%) ⬇️
src/_pytest/unittest.py 74.01% <0%> (-20.34%) ⬇️
testing/python/approx.py 79.84% <0%> (-19.77%) ⬇️
src/_pytest/compat.py 78.07% <0%> (-18.72%) ⬇️
src/_pytest/recwarn.py 83.19% <0%> (-15.13%) ⬇️
src/_pytest/python_api.py 82.83% <0%> (-14.6%) ⬇️
... and 47 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 233c2a2...ce1cc3d. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented Nov 1, 2018

Codecov on AppVeyor failed to upload, otherwise it is green.

@blueyed blueyed merged commit a192e6b into pytest-dev:features Nov 1, 2018
@blueyed blueyed deleted the lru branch November 1, 2018 14:55
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.

2 participants