Skip to content

_getconftestmodules: avoid isfile()/dirpath() #4224

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

Merged
merged 1 commit into from
Oct 26, 2018

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Oct 24, 2018

Ref: #2206 (comment)

I cannot see a real difference myself with clock time (1.2s vs 1.15s with some project and -k doesnotmatch), but I think it makes sense anyway, and might help with the above comment.

For pytest itself:
new: 1 loop, best of 5: 1.73 sec per loop
old: 1 loop, best of 5: 1.61 sec per loop

However, pytest itself displays "2287 deselected in 1.06 seconds" vs "2287 deselected in 1.22 seconds".

@codecov
Copy link

codecov bot commented Oct 24, 2018

Codecov Report

Merging #4224 into master will increase coverage by 2.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4224      +/-   ##
==========================================
+ Coverage   93.38%   95.86%   +2.47%     
==========================================
  Files         109      109              
  Lines       24585    24590       +5     
  Branches     2389     2392       +3     
==========================================
+ Hits        22959    23573     +614     
+ Misses       1308      721     -587     
+ Partials      318      296      -22
Flag Coverage Δ
#docs 29.06% <100%> (ø) ⬆️
#doctesting 29.06% <100%> (ø) ⬆️
#linting 29.06% <100%> (ø) ⬆️
#linux 95.64% <100%> (?)
#nobyte 92% <100%> (?)
#numpy 93% <100%> (?)
#pexpect 41.68% <100%> (?)
#py27 94.05% <100%> (+2.89%) ⬆️
#py34 92.35% <100%> (?)
#py35 92.36% <100%> (+0.65%) ⬆️
#py36 94% <100%> (+2.28%) ⬆️
#py37 92.37% <100%> (?)
#trial 93% <100%> (?)
#windows 94.13% <100%> (+0.74%) ⬆️
#xdist 93.88% <100%> (?)
Impacted Files Coverage Δ
src/_pytest/config/__init__.py 94.98% <100%> (+0.33%) ⬆️
testing/code/test_code.py 94.2% <0%> (-1.32%) ⬇️
testing/code/test_source.py 97.04% <0%> (ø) ⬆️
testing/python/fixture.py 99.24% <0%> (+0.1%) ⬆️
src/_pytest/fixtures.py 97.37% <0%> (+0.13%) ⬆️
testing/test_config.py 99.42% <0%> (+0.38%) ⬆️
testing/test_assertrewrite.py 83.38% <0%> (+0.46%) ⬆️
src/_pytest/assertion/rewrite.py 95.7% <0%> (+0.66%) ⬆️
testing/test_runner.py 96.88% <0%> (+0.66%) ⬆️
... and 29 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 041044e...63691f5. Read the comment docs.

@blueyed
Copy link
Contributor Author

blueyed commented Oct 24, 2018

The 2nd commit also uses lru_cache missing in py27.
Needs a solution via #4227.

@blueyed blueyed force-pushed the _getconftestmodules branch from 1acb18b to b46d42d Compare October 24, 2018 21:06
@blueyed
Copy link
Contributor Author

blueyed commented Oct 24, 2018

I am taking out the 2nd commit (1acb18b).

@@ -399,7 +399,7 @@ def _getconftestmodules(self, path):
mod = self._importconftest(conftestpath)
clist.append(mod)

self._path2confmods[directory] = clist
self._path2confmods[path] = clist
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that with 1acb18b I've noticed that some other code might expect a dirpath in self._path2confmods - haven't checked if I broke this with the previous optimization already though.

@blueyed blueyed merged commit 9cde67c into pytest-dev:master Oct 26, 2018
@blueyed blueyed deleted the _getconftestmodules branch October 26, 2018 12:44
@blueyed blueyed mentioned this pull request Oct 26, 2018
3 tasks
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