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

Fix problems with running tests in package __init__ files (#4046) #4285

Merged
merged 5 commits into from
Nov 2, 2018
Merged

Fix problems with running tests in package __init__ files (#4046) #4285

merged 5 commits into from
Nov 2, 2018

Conversation

kchmck
Copy link
Contributor

@kchmck kchmck commented Nov 1, 2018

This fixes some behavior changes that were introduced in 3.7.0, detailed by #4046.

If you have a directory structure like

project/
    pytest.ini (has python_files = *.py)
    setup.py
    mypackage/
        __init__.py (contains tests)
        mymodule.py (contains tests)

then prior to 3.7.0 you could run pytest mypackage from the project directory, and it would pick up all the tests in __init__.py and mymodule.py. After 3.7.0 the tests in __init__.py started being ignored.

There was a partial fix for this in #3872, but it only works if the project directory and not the package directory is passed as the argument (commit 8485081 adds an example failing test for this.)

This PR seems to fix that and the other issue in #4046. I'm pretty sure the changes here aren't ideal and should probably be cleaned up by someone more familiar with the codebase, but hopefully this can at least serve as a starting point for the fixes.

The issue with __init__.py being ignored seems to stem from it being added to the duplicate paths set when it's visited in the root traversal loop, then when it's visited by the directory collection loop it gets ignored as a duplicate even though it was never checked for tests. Removing it from the duplicate set as a fix causes an additional issue of a duplicate nested package being created, which I also added a special-case fix for.

The issue with all package files being collected when only __init__.py was specified (test in 3f07f7e) appears to stem from __init__.py being treated as a Package (that gets traversed) when it should be treated as a Module.

With these changes, all the existing tests except one pass (commit 28f4155). The test failure seems legitimate to me, though, because it wasn't counting the package created by the __init__ file. Please let me know if I'm interpreting that wrong.

# actually yielded up to higher collection layers, so
# remove it to allow collection by later package
# traversal.
pm._duplicatepaths.discard(pkginit)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be necessary anymore with e041823 (#4241, in features).
It also adjusts the existing test like you did.
I.e. please try if your issue is fixed in the features branch already.

@blueyed
Copy link
Contributor

blueyed commented Nov 1, 2018

Created a backport for master: #4286

@kchmck
Copy link
Contributor Author

kchmck commented Nov 1, 2018

Thanks for looking into this! I can confirm that #4241/#4286 fixes the testcase added in 8485081 but not the one in 3f07f7e.

@blueyed
Copy link
Contributor

blueyed commented Nov 1, 2018

Cool!
Please cherry-pick #4286 here then (without the changelog), and add your other fixes on top.

@blueyed
Copy link
Contributor

blueyed commented Nov 1, 2018

Cherry-pick e041823, please.

@blueyed blueyed added type: bug problem that needs to be addressed topic: collection related to the collection phase labels Nov 1, 2018
@kchmck
Copy link
Contributor Author

kchmck commented Nov 1, 2018

Alright, I've updated on top of e041823

@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #4285 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4285      +/-   ##
==========================================
+ Coverage   95.65%   95.87%   +0.21%     
==========================================
  Files         109      109              
  Lines       24630    24639       +9     
  Branches     2396     2395       -1     
==========================================
+ Hits        23560    23622      +62     
+ Misses        759      721      -38     
+ Partials      311      296      -15
Flag Coverage Δ
#docs 28.96% <25%> (?)
#doctesting 28.96% <25%> (?)
#linting 28.96% <25%> (?)
#linux 95.65% <100%> (ø) ⬆️
#nobyte 92.02% <100%> (+0.63%) ⬆️
#numpy 93.02% <100%> (+51.4%) ⬆️
#pexpect 41.58% <25%> (-0.03%) ⬇️
#py27 94.05% <100%> (+0.11%) ⬆️
#py34 92.34% <100%> (+0.15%) ⬆️
#py35 92.36% <100%> (+0.15%) ⬆️
#py36 94% <100%> (+0.17%) ⬆️
#py37 92.37% <100%> (+0.02%) ⬆️
#trial 93.02% <100%> (+51.4%) ⬆️
#windows 94.13% <100%> (?)
#xdist 93.9% <100%> (+0.16%) ⬆️
Impacted Files Coverage Δ
src/_pytest/python.py 95.58% <ø> (-0.27%) ⬇️
src/_pytest/main.py 96.28% <100%> (+0.03%) ⬆️
testing/test_session.py 96.5% <100%> (ø) ⬆️
testing/test_collection.py 99.77% <100%> (ø) ⬆️
src/_pytest/fixtures.py 97.38% <0%> (+0.26%) ⬆️
testing/test_capture.py 99.24% <0%> (+0.3%) ⬆️
src/_pytest/pytester.py 87.39% <0%> (+0.42%) ⬆️
src/_pytest/nodes.py 94.75% <0%> (+0.8%) ⬆️
testing/acceptance_test.py 98.27% <0%> (+1.07%) ⬆️
... and 5 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 56e6bb0...5197354. Read the comment docs.

@blueyed
Copy link
Contributor

blueyed commented Nov 1, 2018

Great work, thanks!
Waiting for somebody else to merge it.

@nicoddemus
Copy link
Member

Great work guys, thanks!

@nicoddemus nicoddemus merged commit 21725e9 into pytest-dev:master Nov 2, 2018
@kchmck
Copy link
Contributor Author

kchmck commented Nov 2, 2018

Awesome, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: collection related to the collection phase type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants