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

Issue 2836 fixture collection bug #2862

Merged
merged 13 commits into from
Oct 24, 2017
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ Stephan Obermann
Tareq Alayan
Ted Xiao
Thomas Grainger
Tom Dalton
Tom Viner
Trevor Bekolay
Tyler Goodlet
Expand Down
35 changes: 34 additions & 1 deletion _pytest/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -1130,7 +1130,40 @@ def getfixturedefs(self, argname, nodeid):
else:
return tuple(self._matchfactories(fixturedefs, nodeid))

@classmethod
def _splitnode(cls, nodeid):
Copy link

Choose a reason for hiding this comment

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

It appears that this can be a staticmethod instead. The class isn't referenced anywhere in this method.

Copy link
Contributor Author

@tom-dalton-fanduel tom-dalton-fanduel Oct 23, 2017

Choose a reason for hiding this comment

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

I'm not a fan of staticmethods, though mainly for inheritance related reasons and doubt anyone is subclassing fixtruemanager, let alone doing so and then wanting to change these. It feels like they should probably be plain functions in a 'node' module somewhere but hopefully someone more familiar with pytest can guide me on that... (see also my comment about _splitnode in the description).

Copy link
Member

Choose a reason for hiding this comment

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

since both helpers are completely unrelated to the class, how about just moving them out of the class instead

"""Split a nodeid into constituent 'parts'.

Node IDs are strings, and can be things like:
'',
'testing/code',
'testing/code/test_excinfo.py'
'testing/code/test_excinfo.py::TestFormattedExcinfo::()'

"""
if nodeid == '':
return []
sep = py.path.local.sep
parts = nodeid.split(sep)
last_part = parts[-1]
if '::' in last_part:
# Replace single last element 'test_foo.py::Bar::()' with multiple elements 'test_foo.py', 'Bar', '()'
parts[-1:] = last_part.split("::")
return parts
Copy link

Choose a reason for hiding this comment

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

Just a suggestion to simplify the code: remove the special case for the empty nodeid. The following would return functionally equivalent results (it would just return [''] instead of []) without special-casing anything:

sep = py.path.local.sep
parts = nodeid.split(sep)
parts[-1:] = parts[-1].split("::")
return parts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've shortened this down a bit (and made the docstring longer to compensate ;-))


@classmethod
def _ischildnode(cls, baseid, nodeid):
"""Return True if the nodeid is a child node of the baseid.

E.g. 'foo/bar::Baz::()' is a child of 'foo', 'foo/bar' and 'foo/bar::Baz', but not of 'foo/blorp'
"""
base_parts = cls._splitnode(baseid)
node_parts = cls._splitnode(nodeid)
if len(node_parts) < len(base_parts):
return False
return node_parts[:len(base_parts)] == base_parts

def _matchfactories(self, fixturedefs, nodeid):
for fixturedef in fixturedefs:
if nodeid.startswith(fixturedef.baseid):
if self._ischildnode(fixturedef.baseid, nodeid):
yield fixturedef
3 changes: 3 additions & 0 deletions changelog/2836.bug
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Attempted fix for https://github.com/pytest-dev/pytest/issues/2836
Copy link
Member

Choose a reason for hiding this comment

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

i propose match fixture paths against actual path segments in order to avoid matching folders which share a prefix

CC @nicoddemus


Improves the handling of "nodeid"s to be more aware of path and class separators.
26 changes: 26 additions & 0 deletions testing/test_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import pytest
import py

import _pytest._code
from _pytest.main import Session, EXIT_NOTESTSCOLLECTED, _in_venv


Expand Down Expand Up @@ -830,3 +831,28 @@ def test_continue_on_collection_errors_maxfail(testdir):
"*Interrupted: stopping after 3 failures*",
"*1 failed, 2 error*",
])


def test_fixture_scope_sibling_conftests(testdir):
"""Regression test case for https://github.com/pytest-dev/pytest/issues/2836"""
foo_path = testdir.mkpydir("foo")
foo_path.join("conftest.py").write(_pytest._code.Source("""
import pytest
@pytest.fixture
def fix():
return 1
"""))
foo_path.join("test_foo.py").write("def test_foo(fix): assert fix == 1")

# Tests in `food/` should not see the conftest fixture from `foo/`
food_path = testdir.mkpydir("food")
food_path.join("test_food.py").write("def test_food(fix): assert fix == 1")

res = testdir.runpytest()
assert res.ret == 1

res.stdout.fnmatch_lines([
"*ERROR at setup of test_food*",
"E*fixture 'fix' not found",
"*1 passed, 1 error*",
])
18 changes: 18 additions & 0 deletions testing/test_fixtures.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import pytest

from _pytest import fixtures


@pytest.mark.parametrize("baseid, nodeid, expected", (
('', '', True),
('', 'foo', True),
('', 'foo/bar', True),
('', 'foo/bar::TestBaz::()', True),
('foo', 'food', False),
('foo/bar::TestBaz::()', 'foo/bar', False),
('foo/bar::TestBaz::()', 'foo/bar::TestBop::()', False),
('foo/bar', 'foo/bar::TestBop::()', True),
))
def test_fixturemanager_ischildnode(baseid, nodeid, expected):
result = fixtures.FixtureManager._ischildnode(baseid, nodeid)
assert result is expected