Allow ovewriting a parametrized fixture while reusing the parent fixture's value

[nicoddemus]

Fix #1953

[bluetech]

The fix has a flaw in that if metafunc.parametrize happens, it still tries the super fixture, which leads to the parametrization possibly being attempted again. You can see it by changing the overriding fixture (in test_foo.py) to be parametrized itself, e.g. @pytest.fixture(params=[10, 20]). Then it crashes with ValueError: duplicate 'foo'.

The fix I think is just to stop when parametrizing. I would add the above case as an additional test either way.

BTW, when reviewing I refactored the function a bit to be more understandable (to me). You can apply it, if you think it's better of course. Based on master and contains the fix.

diff --git a/src/_pytest/fixtures.py b/src/_pytest/fixtures.py
index bf77d09f1..36cf15899 100644
--- a/src/_pytest/fixtures.py
+++ b/src/_pytest/fixtures.py
@@ -47,6 +47,7 @@ from _pytest.config import _PluggyPlugin
 from _pytest.config import Config
 from _pytest.config.argparsing import Parser
 from _pytest.deprecated import FILLFUNCARGS
+from _pytest.mark import Mark
 from _pytest.mark import ParameterSet
 from _pytest.outcomes import fail
 from _pytest.outcomes import TEST_OUTCOME
@@ -1529,34 +1530,53 @@ class FixtureManager:
         return initialnames, fixturenames_closure, arg2fixturedefs
 
     def pytest_generate_tests(self, metafunc: "Metafunc") -> None:
+        def get_parametrize_mark_argnames(mark: Mark) -> Sequence[str]:
+            if "argnames" in mark.kwargs:
+                argnames = mark.kwargs[
+                    "argnames"
+                ]  # type: Union[str, Tuple[str, ...], List[str]]
+            else:
+                argnames = mark.args[0]
+            if not isinstance(argnames, (tuple, list)):
+                argnames = [x.strip() for x in argnames.split(",") if x.strip()]
+            return argnames
+
         for argname in metafunc.fixturenames:
-            faclist = metafunc._arg2fixturedefs.get(argname)
-            if faclist:
-                fixturedef = faclist[-1]
+            # Get the FixtureDefs for the argname.
+            fixture_defs = metafunc._arg2fixturedefs.get(argname)
+            if not fixture_defs:
+                # Will raise FixtureLookupError at setup time.
+                continue
+
+            # The test itself parametrizes using this argname, give it
+            # precedence.
+            if any(
+                argname in get_parametrize_mark_argnames(mark)
+                for mark in metafunc.definition.iter_markers("parametrize")
+            ):
+                break
+
+            # In the common case we only look at the fixture def with the
+            # closest scope (last in the list). But if the fixture overrides
+            # another fixture, while requesting the super fixture, keep going
+            # in case the super fixture is parametrized (#1953).
+            for fixturedef in reversed(fixture_defs):
+                # Fixture is parametrized, apply it and stop.
                 if fixturedef.params is not None:
-                    markers = list(metafunc.definition.iter_markers("parametrize"))
-                    for parametrize_mark in markers:
-                        if "argnames" in parametrize_mark.kwargs:
-                            argnames = parametrize_mark.kwargs["argnames"]
-                        else:
-                            argnames = parametrize_mark.args[0]
+                    metafunc.parametrize(
+                        argname,
+                        fixturedef.params,
+                        indirect=True,
+                        scope=fixturedef.scope,
+                        ids=fixturedef.ids,
+                    )
+                    break
 
-                        if not isinstance(argnames, (tuple, list)):
-                            argnames = [
-                                x.strip() for x in argnames.split(",") if x.strip()
-                            ]
-                        if argname in argnames:
-                            break
-                    else:
-                        metafunc.parametrize(
-                            argname,
-                            fixturedef.params,
-                            indirect=True,
-                            scope=fixturedef.scope,
-                            ids=fixturedef.ids,
-                        )
-            else:
-                continue  # Will raise FixtureLookupError at setup time.
+                # Not requesting the overridden super fixture, stop.
+                if argname not in fixturedef.argnames:
+                    break
+
+                # Try super fixture, if any.
 
     def pytest_collection_modifyitems(self, items: "List[nodes.Item]") -> None:
         # Separate parametrized setups.

[bluetech]

Suggested change

	Fix error when overwriting a fixture, reusing the original fixture value, when the overwritten fixture is parameterized:
	Fix error when overwriting a fixture, reusing the original fixture value, when the overwritten fixture is parametrized:

😮 😮

[bluetech]

I think the changes would be too risky for a backport, so I prefer we don't backport it.

[nicoddemus]

The fix has a flaw in that if metafunc.parametrize happens, it still tries the super fixture, which leads to the parametrization possibly being attempted again. You can see it by changing the overriding fixture (in test_foo.py) to be parametrized itself, e.g. @pytest.fixture(params=[10, 20]). Then it crashes with ValueError: duplicate 'foo'.

Ahh good catch, thanks! I will apply your improvements and add a new test (it really amazes me that, besides the huge number of test cases and scenarios we have, we still sometimes have gaps on them).

BTW, when reviewing I refactored the function a bit to be more understandable (to me). You can apply it, if you think it's better of course. Based on master and contains the fix.

Much clearer, thanks, I will apply them. 👍

I think the changes would be too risky for a backport, so I prefer we don't backport it.

Fair enough. 👍

[nicoddemus]

Noticed this should be continue instead of break, otherwise we would skip evaluating the rest of the argnames for possible fixture parametrization (test_override_parametrize_fixture_and_indirect demonstrates this).

[nicoddemus]

Applied the changes and tests as requested, thanks again for the great review @bluetech!

[bluetech]

Great, LGTM!