fixtures: match fixtures based on actual node hierarchy, not textual nodeids

Refs #11662.

--- Problem

Each fixture definition has a "visibility", the `FixtureDef.baseid`
attribute. This is nodeid-like string. When a certain `node` requests a
certain fixture name, we match node's nodeid against the fixture
definitions with this name.

The matching currently happens on the *textual* representation of the
nodeid - we split `node.nodeid` to its "parent nodeids" and then check
if the fixture's `baseid` is in there.

While this has worked so far, we really should try to avoid textual
manipulation of nodeids as much as possible. It has also caused problem
in an odd case of a `Package` in the root directory: the `Package` gets
nodeid `.`, while a `Module` in it gets nodeid `test_module.py`. And
textually, `.` is not a parent of `test_module.py`.

--- Solution

Avoid this entirely by just checking the node hierarchy itself. This is
made possible by the fact that we now have proper `Directory` nodes
(`Dir` or `Package`) for the entire hierarchy.

Also do the same for `_getautousenames` which is a similar deal.

The `iterparentnodeids` function is no longer used and is removed.

Author: bluetech

changelog/11785.trivial.rst

@@ -0,0 +1,7 @@
+Some changes were made to private functions which may affect plugins which access them:
+
+- ``FixtureManager._getautousenames()`` now takes a ``Node`` itself instead of the nodeid.
+- ``FixtureManager.getfixturedefs()`` now takes the ``Node`` itself instead of the nodeid.
+- The ``_pytest.nodes.iterparentnodeids()`` function is removed without replacement.
+  Prefer to traverse the node hierarchy itself instead.
+  If you really need to, copy the function from the previous pytest release.

src/_pytest/fixtures.py

@@ -424,9 +424,9 @@ def _getnextfixturedef(self, argname: str) -> "FixtureDef[Any]":
             # We arrive here because of a dynamic call to
             # getfixturevalue(argname) usage which was naturally
             # not known at parsing/collection time.
-            assert self._pyfuncitem.parent is not None
-            parentid = self._pyfuncitem.parent.nodeid
-            fixturedefs = self._fixturemanager.getfixturedefs(argname, parentid)
+            parent = self._pyfuncitem.parent
+            assert parent is not None
+            fixturedefs = self._fixturemanager.getfixturedefs(argname, parent)
             if fixturedefs is not None:
                 self._arg2fixturedefs[argname] = fixturedefs
         # No fixtures defined with this name.
@@ -846,9 +846,8 @@ def formatrepr(self) -> "FixtureLookupErrorRepr":
             available = set()
             parent = self.request._pyfuncitem.parent
             assert parent is not None
-            parentid = parent.nodeid
             for name, fixturedefs in fm._arg2fixturedefs.items():
-                faclist = list(fm._matchfactories(fixturedefs, parentid))
+                faclist = list(fm._matchfactories(fixturedefs, parent))
                 if faclist:
                     available.add(name)
             if self.argname in available:
@@ -989,9 +988,8 @@ def __init__(
         # The "base" node ID for the fixture.
         #
         # This is a node ID prefix. A fixture is only available to a node (e.g.
-        # a `Function` item) if the fixture's baseid is a parent of the node's
-        # nodeid (see the `iterparentnodeids` function for what constitutes a
-        # "parent" and a "prefix" in this context).
+        # a `Function` item) if the fixture's baseid is a nodeid of a parent of
+        # node.
         #
         # For a fixture found in a Collector's object (e.g. a `Module`s module,
         # a `Class`'s class), the baseid is the Collector's nodeid.
@@ -1482,7 +1480,7 @@ def getfixtureinfo(
         else:
             argnames = ()
         usefixturesnames = self._getusefixturesnames(node)
-        autousenames = self._getautousenames(node.nodeid)
+        autousenames = self._getautousenames(node)
         initialnames = deduplicate_names(autousenames, usefixturesnames, argnames)
 
         direct_parametrize_args = _get_direct_parametrize_args(node)
@@ -1517,10 +1515,10 @@ def pytest_plugin_registered(self, plugin: _PluggyPlugin) -> None:
 
         self.parsefactories(plugin, nodeid)
 
-    def _getautousenames(self, nodeid: str) -> Iterator[str]:
-        """Return the names of autouse fixtures applicable to nodeid."""
-        for parentnodeid in nodes.iterparentnodeids(nodeid):
-            basenames = self._nodeid_autousenames.get(parentnodeid)
+    def _getautousenames(self, node: nodes.Node) -> Iterator[str]:
+        """Return the names of autouse fixtures applicable to node."""
+        for parentnode in reversed(list(nodes.iterparentnodes(node))):
+            basenames = self._nodeid_autousenames.get(parentnode.nodeid)
             if basenames:
                 yield from basenames
 
@@ -1542,7 +1540,6 @@ def getfixtureclosure(
         # to re-discover fixturedefs again for each fixturename
         # (discovering matching fixtures for a given name/node is expensive).
 
-        parentid = parentnode.nodeid
         fixturenames_closure = list(initialnames)
 
         arg2fixturedefs: Dict[str, Sequence[FixtureDef[Any]]] = {}
@@ -1554,7 +1551,7 @@ def getfixtureclosure(
                     continue
                 if argname in arg2fixturedefs:
                     continue
-                fixturedefs = self.getfixturedefs(argname, parentid)
+                fixturedefs = self.getfixturedefs(argname, parentnode)
                 if fixturedefs:
                     arg2fixturedefs[argname] = fixturedefs
                     for arg in fixturedefs[-1].argnames:
@@ -1726,7 +1723,7 @@ def parsefactories(  # noqa: F811
             self._nodeid_autousenames.setdefault(nodeid or "", []).extend(autousenames)
 
     def getfixturedefs(
-        self, argname: str, nodeid: str
+        self, argname: str, node: nodes.Node
     ) -> Optional[Sequence[FixtureDef[Any]]]:
         """Get FixtureDefs for a fixture name which are applicable
         to a given node.
@@ -1737,18 +1734,18 @@ def getfixturedefs(
         an empty result is returned).
 
         :param argname: Name of the fixture to search for.
-        :param nodeid: Full node id of the requesting test.
+        :param node: The requesting Node.
         """
         try:
             fixturedefs = self._arg2fixturedefs[argname]
         except KeyError:
             return None
-        return tuple(self._matchfactories(fixturedefs, nodeid))
+        return tuple(self._matchfactories(fixturedefs, node))
 
     def _matchfactories(
-        self, fixturedefs: Iterable[FixtureDef[Any]], nodeid: str
+        self, fixturedefs: Iterable[FixtureDef[Any]], node: nodes.Node
     ) -> Iterator[FixtureDef[Any]]:
-        parentnodeids = set(nodes.iterparentnodeids(nodeid))
+        parentnodeids = {n.nodeid for n in nodes.iterparentnodes(node)}
         for fixturedef in fixturedefs:
             if fixturedef.baseid in parentnodeids:
                 yield fixturedef

src/_pytest/nodes.py

@@ -49,49 +49,13 @@
 tracebackcutdir = Path(_pytest.__file__).parent
 
 
-def iterparentnodeids(nodeid: str) -> Iterator[str]:
-    """Return the parent node IDs of a given node ID, inclusive.
-
-    For the node ID
-
-        "testing/code/test_excinfo.py::TestFormattedExcinfo::test_repr_source"
-
-    the result would be
-
-        ""
-        "testing"
-        "testing/code"
-        "testing/code/test_excinfo.py"
-        "testing/code/test_excinfo.py::TestFormattedExcinfo"
-        "testing/code/test_excinfo.py::TestFormattedExcinfo::test_repr_source"
-
-    Note that / components are only considered until the first ::.
-    """
-    pos = 0
-    first_colons: Optional[int] = nodeid.find("::")
-    if first_colons == -1:
-        first_colons = None
-    # The root Session node - always present.
-    yield ""
-    # Eagerly consume SEP parts until first colons.
-    while True:
-        at = nodeid.find(SEP, pos, first_colons)
-        if at == -1:
-            break
-        if at > 0:
-            yield nodeid[:at]
-        pos = at + len(SEP)
-    # Eagerly consume :: parts.
-    while True:
-        at = nodeid.find("::", pos)
-        if at == -1:
-            break
-        if at > 0:
-            yield nodeid[:at]
-        pos = at + len("::")
-    # The node ID itself.
-    if nodeid:
-        yield nodeid
+def iterparentnodes(node: "Node") -> Iterator["Node"]:
+    """Return the parent nodes, including the node itself, from the node
+    upwards."""
+    parent: Optional[Node] = node
+    while parent is not None:
+        yield parent
+        parent = parent.parent
 
 
 _NodeType = TypeVar("_NodeType", bound="Node")

testing/plugins_integration/requirements.txt

@@ -1,7 +1,9 @@
 anyio[curio,trio]==4.2.0
 django==5.0
 pytest-asyncio==0.23.2
-pytest-bdd==7.0.1
+# Temporarily not installed until pytest-bdd is fixed:
+# https://github.com/pytest-dev/pytest/pull/11785
+# pytest-bdd==7.0.1
 pytest-cov==4.1.0
 pytest-django==4.7.0
 pytest-flakes==4.0.5

testing/python/fixtures.py

@@ -1574,7 +1574,7 @@ def test_parsefactories_conftest(self, pytester: Pytester) -> None:
             """
             def test_hello(item, fm):
                 for name in ("fm", "hello", "item"):
-                    faclist = fm.getfixturedefs(name, item.nodeid)
+                    faclist = fm.getfixturedefs(name, item)
                     assert len(faclist) == 1
                     fac = faclist[0]
                     assert fac.func.__name__ == name
@@ -1598,7 +1598,7 @@ class TestClass(object):
                 def hello(self, request):
                     return "class"
                 def test_hello(self, item, fm):
-                    faclist = fm.getfixturedefs("hello", item.nodeid)
+                    faclist = fm.getfixturedefs("hello", item)
                     print(faclist)
                     assert len(faclist) == 3
 
@@ -1804,7 +1804,7 @@ def test_parsefactories_conftest(self, pytester: Pytester) -> None:
             """
             from _pytest.pytester import get_public_names
             def test_check_setup(item, fm):
-                autousenames = list(fm._getautousenames(item.nodeid))
+                autousenames = list(fm._getautousenames(item))
                 assert len(get_public_names(autousenames)) == 2
                 assert "perfunction2" in autousenames
                 assert "perfunction" in autousenames

testing/test_nodes.py

@@ -2,7 +2,6 @@
 import warnings
 from pathlib import Path
 from typing import cast
-from typing import List
 from typing import Type
 
 import pytest
@@ -12,29 +11,6 @@
 from _pytest.warning_types import PytestWarning
 
 
-@pytest.mark.parametrize(
-    ("nodeid", "expected"),
-    (
-        ("", [""]),
-        ("a", ["", "a"]),
-        ("aa/b", ["", "aa", "aa/b"]),
-        ("a/b/c", ["", "a", "a/b", "a/b/c"]),
-        ("a/bbb/c::D", ["", "a", "a/bbb", "a/bbb/c", "a/bbb/c::D"]),
-        ("a/b/c::D::eee", ["", "a", "a/b", "a/b/c", "a/b/c::D", "a/b/c::D::eee"]),
-        ("::xx", ["", "::xx"]),
-        # / only considered until first ::
-        ("a/b/c::D/d::e", ["", "a", "a/b", "a/b/c", "a/b/c::D/d", "a/b/c::D/d::e"]),
-        # : alone is not a separator.
-        ("a/b::D:e:f::g", ["", "a", "a/b", "a/b::D:e:f", "a/b::D:e:f::g"]),
-        # / not considered if a part of a test name
-        ("a/b::c/d::e[/test]", ["", "a", "a/b", "a/b::c/d", "a/b::c/d::e[/test]"]),
-    ),
-)
-def test_iterparentnodeids(nodeid: str, expected: List[str]) -> None:
-    result = list(nodes.iterparentnodeids(nodeid))
-    assert result == expected
-
-
 def test_node_from_parent_disallowed_arguments() -> None:
     with pytest.raises(TypeError, match="session is"):
         nodes.Node.from_parent(None, session=None)  # type: ignore[arg-type]

tox.ini

@@ -134,9 +134,11 @@ changedir = testing/plugins_integration
 deps = -rtesting/plugins_integration/requirements.txt
 setenv =
     PYTHONPATH=.
+# Command temporarily removed until pytest-bdd is fixed:
+# https://github.com/pytest-dev/pytest/pull/11785
+# pytest bdd_wallet.py
 commands =
     pip check
-    pytest bdd_wallet.py
     pytest --cov=. simple_integration.py
     pytest --ds=django_settings simple_integration.py
     pytest --html=simple.html simple_integration.py