-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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([
'*In function "test_without_arg"*',
'*Parameter "arg" should be declared explicitly via indirect*',
'*or in function itself*',
]) (same for the other test) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh that test should be declaring 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
""" There was a problem hiding this comment. Choose a reason for hiding this commentThe 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*', | ||
] | ||
) |
There was a problem hiding this comment.
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 fixtureargument
doesn't really exist, does it?There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.