From 5cbc06a4537461fd5e1f51df38c47733733a9b9f Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Thu, 30 May 2019 08:18:28 -0300 Subject: [PATCH 1/2] Show test module in the PytestCollectionWarning message Related to #5330 --- changelog/5330.bugfix.rst | 2 ++ src/_pytest/python.py | 6 ++++-- testing/python/collect.py | 19 ++++++++++++++++++- 3 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 changelog/5330.bugfix.rst diff --git a/changelog/5330.bugfix.rst b/changelog/5330.bugfix.rst new file mode 100644 index 000000000..61c715552 --- /dev/null +++ b/changelog/5330.bugfix.rst @@ -0,0 +1,2 @@ +Show the test module being collected when emitting ``PytestCollectionWarning`` messages for +test classes with ``__init__`` and ``__new__`` methods to make it easier to pin down the problem. diff --git a/src/_pytest/python.py b/src/_pytest/python.py index 035369a59..06d853f78 100644 --- a/src/_pytest/python.py +++ b/src/_pytest/python.py @@ -720,7 +720,8 @@ class Class(PyCollector): self.warn( PytestCollectionWarning( "cannot collect test class %r because it has a " - "__init__ constructor" % self.obj.__name__ + "__init__ constructor (from: %s)" + % (self.obj.__name__, self.parent.nodeid) ) ) return [] @@ -728,7 +729,8 @@ class Class(PyCollector): self.warn( PytestCollectionWarning( "cannot collect test class %r because it has a " - "__new__ constructor" % self.obj.__name__ + "__new__ constructor (from: %s)" + % (self.obj.__name__, self.parent.nodeid) ) ) return [] diff --git a/testing/python/collect.py b/testing/python/collect.py index bfd7e672e..501b30a49 100644 --- a/testing/python/collect.py +++ b/testing/python/collect.py @@ -146,7 +146,24 @@ class TestClass(object): result = testdir.runpytest("-rw") result.stdout.fnmatch_lines( [ - "*cannot collect test class 'TestClass1' because it has a __init__ constructor" + "*cannot collect test class 'TestClass1' because it has " + "a __init__ constructor (from: test_class_with_init_warning.py)" + ] + ) + + def test_class_with_new_warning(self, testdir): + testdir.makepyfile( + """ + class TestClass1(object): + def __new__(self): + pass + """ + ) + result = testdir.runpytest("-rw") + result.stdout.fnmatch_lines( + [ + "*cannot collect test class 'TestClass1' because it has " + "a __new__ constructor (from: test_class_with_new_warning.py)" ] ) From 65bd1b8a9316ee4ab514941eb883f89d20caa8b8 Mon Sep 17 00:00:00 2001 From: Victor Maryama Date: Mon, 13 May 2019 01:49:18 +0200 Subject: [PATCH 2/2] Avoiding looking upwards for parameter argnames when generating fixtureinfo. --- changelog/5036.bugfix.rst | 1 + src/_pytest/fixtures.py | 28 ++++++++++++++++++++-- src/_pytest/mark/structures.py | 12 ++++++++-- testing/python/fixtures.py | 43 ++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 4 deletions(-) create mode 100644 changelog/5036.bugfix.rst diff --git a/changelog/5036.bugfix.rst b/changelog/5036.bugfix.rst new file mode 100644 index 000000000..a9c266538 --- /dev/null +++ b/changelog/5036.bugfix.rst @@ -0,0 +1 @@ +Fix issue where fixtures dependent on other parametrized fixtures would be erroneously parametrized. diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py index 7761c4af8..fa2974f1b 100644 --- a/src/_pytest/fixtures.py +++ b/src/_pytest/fixtures.py @@ -1129,18 +1129,40 @@ class FixtureManager(object): self._nodeid_and_autousenames = [("", self.config.getini("usefixtures"))] session.config.pluginmanager.register(self, "funcmanage") + def _get_direct_parametrize_args(self, node): + """This function returns all the direct parametrization + arguments of a node, so we don't mistake them for fixtures + + Check https://github.com/pytest-dev/pytest/issues/5036 + + This things are done later as well when dealing with parametrization + so this could be improved + """ + from _pytest.mark import ParameterSet + + parametrize_argnames = [] + for marker in node.iter_markers(name="parametrize"): + if not marker.kwargs.get("indirect", False): + p_argnames, _ = ParameterSet._parse_parametrize_args( + *marker.args, **marker.kwargs + ) + parametrize_argnames.extend(p_argnames) + + return parametrize_argnames + def getfixtureinfo(self, node, func, cls, funcargs=True): if funcargs and not getattr(node, "nofuncargs", False): argnames = getfuncargnames(func, cls=cls) else: argnames = () + usefixtures = itertools.chain.from_iterable( mark.args for mark in node.iter_markers(name="usefixtures") ) initialnames = tuple(usefixtures) + argnames fm = node.session._fixturemanager initialnames, names_closure, arg2fixturedefs = fm.getfixtureclosure( - initialnames, node + initialnames, node, ignore_args=self._get_direct_parametrize_args(node) ) return FuncFixtureInfo(argnames, initialnames, names_closure, arg2fixturedefs) @@ -1174,7 +1196,7 @@ class FixtureManager(object): autousenames.extend(basenames) return autousenames - def getfixtureclosure(self, fixturenames, parentnode): + def getfixtureclosure(self, fixturenames, parentnode, ignore_args=()): # collect the closure of all fixtures , starting with the given # fixturenames as the initial set. As we have to visit all # factory definitions anyway, we also return an arg2fixturedefs @@ -1202,6 +1224,8 @@ class FixtureManager(object): while lastlen != len(fixturenames_closure): lastlen = len(fixturenames_closure) for argname in fixturenames_closure: + if argname in ignore_args: + continue if argname in arg2fixturedefs: continue fixturedefs = self.getfixturedefs(argname, parentid) diff --git a/src/_pytest/mark/structures.py b/src/_pytest/mark/structures.py index b133a4e26..561ccc3f4 100644 --- a/src/_pytest/mark/structures.py +++ b/src/_pytest/mark/structures.py @@ -103,8 +103,11 @@ class ParameterSet(namedtuple("ParameterSet", "values, marks, id")): else: return cls(parameterset, marks=[], id=None) - @classmethod - def _for_parametrize(cls, argnames, argvalues, func, config, function_definition): + @staticmethod + def _parse_parametrize_args(argnames, argvalues, **_): + """It receives an ignored _ (kwargs) argument so this function can + take also calls from parametrize ignoring scope, indirect, and other + arguments...""" if not isinstance(argnames, (tuple, list)): argnames = [x.strip() for x in argnames.split(",") if x.strip()] force_tuple = len(argnames) == 1 @@ -113,6 +116,11 @@ class ParameterSet(namedtuple("ParameterSet", "values, marks, id")): parameters = [ ParameterSet.extract_from(x, force_tuple=force_tuple) for x in argvalues ] + return argnames, parameters + + @classmethod + def _for_parametrize(cls, argnames, argvalues, func, config, function_definition): + argnames, parameters = cls._parse_parametrize_args(argnames, argvalues) del argvalues if parameters: diff --git a/testing/python/fixtures.py b/testing/python/fixtures.py index 18ede4006..bd1b1d975 100644 --- a/testing/python/fixtures.py +++ b/testing/python/fixtures.py @@ -3950,3 +3950,46 @@ def test_call_fixture_function_error(): with pytest.raises(pytest.fail.Exception): assert fix() == 1 + + +def test_fixture_param_shadowing(testdir): + """Parametrized arguments would be shadowed if a fixture with the same name also exists (#5036)""" + testdir.makepyfile( + """ + import pytest + + @pytest.fixture(params=['a', 'b']) + def argroot(request): + return request.param + + @pytest.fixture + def arg(argroot): + return argroot + + # This should only be parametrized directly + @pytest.mark.parametrize("arg", [1]) + def test_direct(arg): + assert arg == 1 + + # This should be parametrized based on the fixtures + def test_normal_fixture(arg): + assert isinstance(arg, str) + + # Indirect should still work: + + @pytest.fixture + def arg2(request): + return 2*request.param + + @pytest.mark.parametrize("arg2", [1], indirect=True) + def test_indirect(arg2): + assert arg2 == 2 + """ + ) + # Only one test should have run + result = testdir.runpytest("-v") + result.assert_outcomes(passed=4) + result.stdout.fnmatch_lines(["*::test_direct[[]1[]]*"]) + result.stdout.fnmatch_lines(["*::test_normal_fixture[[]a[]]*"]) + result.stdout.fnmatch_lines(["*::test_normal_fixture[[]b[]]*"]) + result.stdout.fnmatch_lines(["*::test_indirect[[]1[]]*"])