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.

Sign up for GitHub

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

Jump to bottom

Allow tests declared as @staticmethod to use fixtures #2700

Merged
merged 2 commits into from
Sep 6, 2017
Merged

Allow tests declared as @staticmethod to use fixtures #2700

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a993add
Allow tests declared as @staticmethod to use fixtures
nicoddemus Aug 17, 2017
1fc185b
Add comment about possible future refactoring in the fixture mechanism
nicoddemus Sep 5, 2017
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
5 changes: 4 additions & 1 deletion _pytest/compat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,10 @@ def num_mock_patch_args(function):
return len(patchings)


def getfuncargnames(function, startindex=None):
def getfuncargnames(function, startindex=None, cls=None):
if startindex is None and cls is not None:
Copy link
Member

Choose a reason for hiding this comment

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

this bit should be documented as a hack and factored out when revisiting fixtures

Copy link
Member Author

@nicoddemus nicoddemus Aug 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Why is it a hack? It is not entirely clear to me why it this is there and I would like to document it properly then. 😉

Copy link
Member

Choose a reason for hiding this comment

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

it moves a new responsibility into getfuncargnames, and fills the code with a lot of conditionals that are strictly not needed at that place and interaction

while reviewing i noticed that this also creates a dependence on how things are named, it will work incorrectly in some cases

Copy link
Member Author

Choose a reason for hiding this comment

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

it moves a new responsibility into getfuncargnames, and fills the code with a lot of conditionals that are strictly not needed at that place and interaction

Oh I see. I decided to move cls here so that callers didn't have to handle known that cls might affect the results of this call themselves; the previous code was splitting that responsibility between two places, getfixtureinfo in _pytest/fixtures.py and here (see the diff). IMHO if getfuncargnames also needs a cls argument to figure things out, it should receive it and not require callers to handle that special case themselves.

while reviewing i noticed that this also creates a dependence on how things are named, it will work incorrectly in some cases

Which things? The function name or the argument names?

Copy link
Member

Choose a reason for hiding this comment

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

the function name

Copy link
Member

Choose a reason for hiding this comment

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

@nicoddemus from my pov the fixture mechanism should ask the node for the requested fixtures, not extract it somewhere down the line from the function object, in fact it should never even see the function object simply to separate concerns -

Copy link
Member Author

Choose a reason for hiding this comment

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

I see thanks. But from my understanding this is quite a refactor on how fixture names are obtained and out of scope of this PR.

This PR attempts to fix fixture support for static methods and doesn't add complexity to the current system IMHO. If you disagree we should close this PR, which I'm OK with.

Copy link
Member

Choose a reason for hiding this comment

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

im fine with the pr, i just want to see a note there detailing what is made worse to get it done

is_staticmethod = isinstance(cls.__dict__.get(function.__name__, None), staticmethod)
startindex = 0 if is_staticmethod else 1
# XXX merge with main.py's varnames
# assert not isclass(function)
realfunction = function
Expand Down
6 changes: 1 addition & 5 deletions _pytest/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,11 +957,7 @@ def __init__(self, session):

def getfixtureinfo(self, node, func, cls, funcargs=True):
if funcargs and not hasattr(node, "nofuncargs"):
if cls is not None:
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's what I mean about the "split" responsibility.

Note as well that getfuncargnames is an internal utility function used in two places in the code base, it might as well make things simpler for callers to avoid mistakes and keep the calling code clean (as it does here, IMHO).

startindex = 1
else:
startindex = None
argnames = getfuncargnames(func, startindex)
argnames = getfuncargnames(func, cls=cls)
else:
argnames = ()
usefixtures = getattr(func, "usefixtures", None)
Expand Down
1 change: 1 addition & 0 deletions changelog/2699.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Allow tests declared as ``@staticmethod`` to use fixtures.
14 changes: 12 additions & 2 deletions testing/python/collect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,21 @@ class test(object):
])

def test_static_method(self, testdir):
"""Support for collecting staticmethod tests (#2528, #2699)"""
testdir.getmodulecol("""
import pytest
class Test(object):
@staticmethod
def test_something():
pass
@pytest.fixture
def fix(self):
return 1
@staticmethod
def test_fix(fix):
assert fix == 1
""")
result = testdir.runpytest()
if sys.version_info < (2, 7):
Expand All @@ -162,8 +172,8 @@ def test_something():
])
else:
result.stdout.fnmatch_lines([
"*collected 1 item*",
"*1 passed in*",
"*collected 2 items*",
"*2 passed in*",
])

def test_setup_teardown_class_as_classmethod(self, testdir):
Expand Down
6 changes: 6 additions & 0 deletions testing/python/fixture.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,16 @@ class A(object):
def f(self, arg1, arg2="hello"):
pass

@staticmethod
def static(arg1, arg2):
pass

assert fixtures.getfuncargnames(A().f) == ('arg1',)
if sys.version_info < (3, 0):
assert fixtures.getfuncargnames(A.f) == ('arg1',)

assert fixtures.getfuncargnames(A.static, cls=A) == ('arg1', 'arg2')


class TestFillFixtures(object):
def test_fillfuncargs_exposed(self):
Expand Down