Merge pull request #5349 from asottile/mm

Merge master into features

Author: blueyed

changelog/5036.bugfix.rst

@@ -0,0 +1 @@
+Fix issue where fixtures dependent on other parametrized fixtures would be erroneously parametrized.

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.

src/_pytest/fixtures.py

@@ -1127,18 +1127,40 @@ def __init__(self, session):
         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)
 
@@ -1172,7 +1194,7 @@ def _getautousenames(self, nodeid):
                 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
@@ -1200,6 +1222,8 @@ def merge(otherlist):
         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)

src/_pytest/mark/structures.py

@@ -103,8 +103,11 @@ def extract_from(cls, parameterset, force_tuple=False):
         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 @@ def _for_parametrize(cls, argnames, argvalues, func, config, function_definition
         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:

src/_pytest/python.py

@@ -719,15 +719,17 @@ def collect(self):
             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 []
         elif hasnew(self.obj):
             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 []

testing/python/collect.py

@@ -146,7 +146,24 @@ def __init__(self):
         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)"
             ]
         )
 

testing/python/fixtures.py

@@ -3950,3 +3950,46 @@ def fix():
 
     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[]]*"])