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

[parametrize] enforce explicit argnames declaration #6330

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
Expand Up @@ -274,6 +274,7 @@ Vidar T. Fauske
Virgil Dupras
Vitaly Lashmanov
Vlad Dragos
Vladyslav Rachek
Volodymyr Piskun
Wei Lin
Wil Cooley
Expand Down
2 changes: 2 additions & 0 deletions changelog/5712.feature.rst
@@ -0,0 +1,2 @@
Now all arguments to ``@pytest.mark.parametrize`` need to be explicitly declared in the function signature or via ``indirect``.
Previously it was possible to omit an argument if a fixture with the same name existed, which was just an accident of implementation and was not meant to be a part of the API.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicoddemus actually I'm wondering a bit if that shouldn't be something like "Previously it was possible to omit an argument if a fixture with same name was declared in parametrize and requested in some fixture above". Example from #5712 suggests it, as far as I can understand. In code posted by @Harius fixture argument doesn't really exist, does it?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Actually when I read it again, I think the existing text is better: the fixture doesn't need to be declared above, it just needs to exist.

3 changes: 3 additions & 0 deletions doc/en/example/parametrize.rst
Expand Up @@ -398,6 +398,9 @@ The result of this test will be successful:

.. regendoc:wipe

Note, that each argument in `parametrize` list should be explicitly declared in corresponding
python test function or via `indirect`.

Parametrizing test methods through per-class configuration
--------------------------------------------------------------

Expand Down
16 changes: 11 additions & 5 deletions src/_pytest/fixtures.py
@@ -1,6 +1,5 @@
import functools
import inspect
import itertools
import sys
import warnings
from collections import defaultdict
Expand Down Expand Up @@ -1280,10 +1279,8 @@ def getfixtureinfo(self, node, func, cls, funcargs=True):
else:
argnames = ()

usefixtures = itertools.chain.from_iterable(
mark.args for mark in node.iter_markers(name="usefixtures")
)
initialnames = tuple(usefixtures) + argnames
usefixtures = get_use_fixtures_for_node(node)
initialnames = usefixtures + argnames
fm = node.session._fixturemanager
initialnames, names_closure, arg2fixturedefs = fm.getfixtureclosure(
initialnames, node, ignore_args=self._get_direct_parametrize_args(node)
Expand Down Expand Up @@ -1480,3 +1477,12 @@ def _matchfactories(self, fixturedefs, nodeid):
for fixturedef in fixturedefs:
if nodes.ischildnode(fixturedef.baseid, nodeid):
yield fixturedef


def get_use_fixtures_for_node(node) -> Tuple[str, ...]:
"""Returns the names of all the usefixtures() marks on the given node"""
return tuple(
str(name)
for mark in node.iter_markers(name="usefixtures")
for name in mark.args
)
33 changes: 33 additions & 0 deletions src/_pytest/python.py
Expand Up @@ -1012,6 +1012,8 @@ def parametrize(

arg_values_types = self._resolve_arg_value_types(argnames, indirect)

self._validate_explicit_parameters(argnames, indirect)

# Use any already (possibly) generated ids with parametrize Marks.
if _param_mark and _param_mark._param_ids_from:
generated_ids = _param_mark._param_ids_from._param_ids_generated
Expand Down Expand Up @@ -1162,6 +1164,37 @@ def _validate_if_using_arg_names(self, argnames, indirect):
pytrace=False,
)

def _validate_explicit_parameters(self, argnames, indirect):
"""
The argnames in *parametrize* should either be declared explicitly via
indirect list or in the function signature

:param List[str] argnames: list of argument names passed to ``parametrize()``.
:param indirect: same ``indirect`` parameter of ``parametrize()``.
:raise ValueError: if validation fails
"""
if isinstance(indirect, bool) and indirect is True:
return
parametrized_argnames = list()
funcargnames = _pytest.compat.getfuncargnames(self.function)
if isinstance(indirect, Sequence):
for arg in argnames:
if arg not in indirect:
parametrized_argnames.append(arg)
elif indirect is False:
parametrized_argnames = argnames

usefixtures = fixtures.get_use_fixtures_for_node(self.definition)

for arg in parametrized_argnames:
if arg not in funcargnames and arg not in usefixtures:
func_name = self.function.__name__
msg = (
'In function "{func_name}":\n'
'Parameter "{arg}" should be declared explicitly via indirect or in function itself'
).format(func_name=func_name, arg=arg)
fail(msg, pytrace=False)


def _find_parametrized_scope(argnames, arg2fixturedefs, indirect):
"""Find the most appropriate scope for a parametrized call based on its arguments.
Expand Down
2 changes: 1 addition & 1 deletion testing/python/collect.py
Expand Up @@ -463,7 +463,7 @@ def fix3():
return '3'

@pytest.mark.parametrize('fix2', ['2'])
def test_it(fix1):
def test_it(fix1, fix2):
assert fix1 == '21'
assert not fix3_instantiated
"""
Expand Down
51 changes: 51 additions & 0 deletions testing/python/metafunc.py
Expand Up @@ -28,6 +28,9 @@ def __init__(self, names):
class DefinitionMock(python.FunctionDefinition):
obj = attr.ib()

def listchain(self):
return []

names = fixtures.getfuncargnames(func)
fixtureinfo = FixtureInfo(names)
definition = DefinitionMock._create(func)
Expand Down Expand Up @@ -1877,3 +1880,51 @@ def test_converted_to_str(a, b):
"*= 6 passed in *",
]
)

def test_parametrize_explicit_parameters_func(self, testdir):
testdir.makepyfile(
"""
import pytest


@pytest.fixture
def fixture(arg):
return arg

@pytest.mark.parametrize("arg", ["baz"])
def test_without_arg(fixture):
assert "baz" == fixture
"""
)
result = testdir.runpytest()
result.assert_outcomes(error=1)
Copy link
Member

Choose a reason for hiding this comment

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

We should also check the actual message shown to the user. For that you can use result.stdout.fnmatch_lines:

result.stdout.fnmatch_lines([
    '*In function "test_without_arg"*',    
    '*Parameter "arg" should be declared explicitly via indirect*',
    '*or in function itself*',
])

(same for the other test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! The interesting thing is that it seems like proposed functionality in general causes other test to fail, e.g. test_usefixtures_seen_in_generate_tests in the same file (testing/python/metafunc.py). Any ideas how do deal with that? Would be really grateful

Copy link
Member

Choose a reason for hiding this comment

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

Oh that test should be declaring fix2, which is exactly what the functionality now tells about.

Please just update the test:

diff --git a/testing/python/collect.py b/testing/python/collect.py
index 30f9841b5..cf530bf0f 100644
--- a/testing/python/collect.py
+++ b/testing/python/collect.py
@@ -463,7 +463,7 @@ class TestFunction:
                return '3'

             @pytest.mark.parametrize('fix2', ['2'])
-            def test_it(fix1):
+            def test_it(fix1, fix2):
                assert fix1 == '21'
                assert not fix3_instantiated
         """

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I've updated that one in the last commit

result.stdout.fnmatch_lines(
[
'*In function "test_without_arg"*',
'*Parameter "arg" should be declared explicitly via indirect or in function itself*',
]
)

def test_parametrize_explicit_parameters_method(self, testdir):
testdir.makepyfile(
"""
import pytest

class Test:
@pytest.fixture
def test_fixture(self, argument):
return argument

@pytest.mark.parametrize("argument", ["foobar"])
def test_without_argument(self, test_fixture):
assert "foobar" == test_fixture
"""
)
result = testdir.runpytest()
result.assert_outcomes(error=1)
result.stdout.fnmatch_lines(
[
'*In function "test_without_argument"*',
'*Parameter "argument" should be declared explicitly via indirect or in function itself*',
]
)