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 #3751 #3798

Merged
merged 3 commits into from Aug 16, 2018

Conversation

@jonozzz
Copy link

commented Aug 10, 2018

The previous fix would basically bypass the if block because the return value would always be a list.

turturica turturica
@coveralls

This comment has been minimized.

Copy link

commented Aug 10, 2018

Coverage Status

Coverage increased (+0.08%) to 92.558% when pulling 27b5435 on jonozzz:fix-3751 into 4d8903f on pytest-dev:master.

@@ -647,7 +647,7 @@ def test_pkgfile(self, testdir):
col = testdir.getnode(config, x)
assert isinstance(col, pytest.Module)
assert col.name == "x.py"
assert col.parent.parent is None
assert col.parent.parent.parent is None

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 10, 2018

Member

lets please add add comment here to explain the nesting since it is a tad confusing as of now

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 10, 2018

Member

Agreed. Probably checking the name of each parent will also help clarifying things. 👍

This comment has been minimized.

Copy link
@jonozzz

jonozzz Aug 10, 2018

Author

It's pretty simple, it's basically testing to see that the Session's parent is None, which should always be and the nodes have been correctly nested. For a more visual example:

tmpdir
 \- subdir
     \- __init__.py
     \- x.py


Session<>
   ^
   | (parent)
Package<subdir>
   ^
   | (parent)
Module<x>

This comment has been minimized.

Copy link
@blueyed

blueyed Aug 13, 2018

Contributor

Maybe the ASCII diagram could be added to the test's docstring?

Copy link
Member

left a comment

Thanks @jonozzz!

Let's improve the test a bit before merging this

turturica and others added 2 commits Aug 10, 2018
turturica turturica
Copy link
Member

left a comment

Thanks for the comment @jonozzz.

I fixed the formatting (it was mixing tabs and spaces apparently) and also improved the test a bit.

assert isinstance(col, pytest.Module)
assert isinstance(col.parent, pytest.Package)
assert isinstance(col.parent.parent, pytest.Session)
# session is batman (has no parents)

This comment has been minimized.

Copy link
@jonozzz

jonozzz Aug 10, 2018

Author

Lol 😂

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

@RonnyPfannschmidt and @blueyed you want to take more time to review this or can we merge it?

@RonnyPfannschmidt RonnyPfannschmidt merged commit 939a792 into pytest-dev:master Aug 16, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.