Replace string nodeids with a structured NodeId internally by nicoddemus · Pull Request #14758 · pytest-dev/pytest

nicoddemus · 2026-07-22T09:10:52Z

pytest identified every collection-tree node with a plain "::"-joined nodeid string, repeatedly re-parsed (split/partition) at dozens of call sites for cache persistence, terminal/JUnit reporting, and stepwise/ subtests bookkeeping. Introduce a NodeId dataclass (src/_pytest/_nodeid.py) and use it internally as the dict/set key and equality type everywhere a nodeid was previously compared or looked up as a string, while keeping the existing nodeid: str property as a full backward-compatible surface for external plugins.

Function's NodeId also carries structured per-parametrize-call data (ParamId: id/argnames/scope) when built from live collection data, laying groundwork for future scope-aware scheduling (e.g. in pytest-xdist) without committing any consumer to it yet.

RonnyPfannschmidt

this is shaping up nicely

we'll have to investigate how to deal with the loadscope groups xdist tucks into node ids at the moment as thats a mess that messes with the strings

nicoddemus · 2026-07-23T08:54:15Z

Should we expose the new node id types as public? I wonder if people will need at least for type annotations...

The-Compiler

Really nice cleanup!

pytest identified every collection-tree node with a plain "::"-joined nodeid string, repeatedly re-parsed (split/partition) at dozens of call sites for cache persistence, terminal/JUnit reporting, and stepwise/ subtests bookkeeping. Introduce a NodeId dataclass (src/_pytest/_nodeid.py) and use it internally as the dict/set key and equality type everywhere a nodeid was previously compared or looked up as a string, while keeping the existing nodeid: str property as a full backward-compatible surface for external plugins. Function's NodeId also carries structured per-parametrize-call data (ParamId: id/argnames/scope) when built from live collection data, laying groundwork for future scope-aware scheduling (e.g. in pytest-xdist) without committing any consumer to it yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Replace the module-level parse_nodeid_path_and_names() and reports.py's private _coerce_node_id() with NodeId.parse()/NodeId.coerce() classmethods, and reword the Node.id/report.id docstrings to reference nodeid directly instead of the ad hoc "collection tree address" phrasing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Compute and cache str(self) on first use inside __str__ itself instead of eagerly in __post_init__, and drop it as a declared dataclass field -- it's just an ad hoc attribute set via object.__setattr__ once computed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

… strings) NodeId.parse()/coerce() let any external string be turned into a NodeId that looked structurally trustworthy (had .names/.params fields) while actually lying about them -- a parametrized test's bracket would get glued into .names[-1] with .params falsely staying empty, since that structure can't be reliably recovered once flattened into a string. Split the concept in two: NodeId is now only ever built from live collection data (direct construction, or .child() chaining off an already-live parent), so its .params can always be trusted. OpaqueNodeId is the honest counterpart for nodeids reconstructed from external sources (the on-disk lastfailed/nodeids/stepwise caches, xdist's JSON wire format, a duck-typed report-like object's .nodeid) -- it only knows path and an unparsed rest, with no claim to structured names or params. Both remain hashable and compare equal by canonical string form across types, so a live item.id still matches a cache-loaded OpaqueNodeId for the same node. Containers that only ever hold one kind stay narrowly typed; the few that legitimately mix live and boundary-sourced ids in the same run (e.g. NFPlugin.cached_nodeids, seeded from cache then updated with live items) are typed as a NodeId | OpaqueNodeId union. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

nitpick_ignore the intentionally-undocumented internal NodeId, OpaqueNodeId and _WithNodeId classes (same pattern already used for BaseReport etc.), and reword _WithNodeId.id's docstring to avoid an unresolvable :attr: cross-reference to `nodeid` in the TestReport/CollectReport doc context. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…m boundary PR review feedback (RonnyPfannschmidt) noted that NodeId.child() had no protection against being called on an id that already carries params -- i.e. building further tree structure on top of a parametrized leaf. Rather than a runtime assert, encode the distinction as a type: pytest's own Node hierarchy already splits cleanly into Collector (can have children) and Item (leaves), so map NodeId onto that exact boundary. - CollectionNodeId (path, names): has .child()/.leaf(), for Collector ids. - ItemNodeId (path, names, params): no .child()/.leaf() at all -- building further structure on a leaf is now a mypy error, not a runtime mistake. - NodeId = CollectionNodeId | ItemNodeId, a type alias for genuinely-mixed cases. Every container previously typed NodeId | OpaqueNodeId was re-derived from scratch by tracing what hook data actually flows into it (not carried over blindly): most narrow to ItemNodeId-only (NFPlugin.cached_nodeids, StepwiseCacheInfo.last_failed, terminal's per-item progress tracking, subtests' failed-count map), CollectReport/TestReport now carry CollectionNodeId/ItemNodeId respectively (with covariant Collector.id/ Item.id overrides to match), while LFPlugin.lastfailed, junitxml's node_reporters and terminal's timing tracker stay genuinely mixed since pytest_collectreport feeds collector-level failures into the same structures pytest_runtest_logreport feeds item-level ones into. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Addresses PR review feedback: since .parse() already has the full nodeid string in hand, cache it directly as _str instead of splitting into path/rest and letting __str__ reconstruct it later. Direct construction (bypassing .parse()) still reconstructs via _build_str() as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…le lookup type Addresses PR review feedback: cacheprovider.py's lastfailed/cached_nodeids don't care about the rich structure CollectionNodeId/ItemNodeId carry, only about identity -- normalizing everything to OpaqueNodeId avoids mixing lookup types in the same container. - CollectionNodeId/ItemNodeId get a trivial .to_opaque() -> OpaqueNodeId method. - New to_opaque_node_id() free function normalizes a NodeId | OpaqueNodeId value (e.g. a report's .id, which may be live or reconstructed from JSON) down to OpaqueNodeId unconditionally. - LFPlugin.lastfailed: dict[OpaqueNodeId, bool], NFPlugin.cached_nodeids: set[OpaqueNodeId] -- both narrowed from the NodeId | OpaqueNodeId / ItemNodeId | OpaqueNodeId unions, with .to_opaque()/to_opaque_node_id() conversions at every insertion/lookup site. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…t.py typing.override was added in Python 3.12; pytest supports >=3.10, so add a version-gated compat shim (mirroring the existing `deprecated` pattern in _pytest.compat) rather than duplicating it in _nodeid.py, which stays a leaf module since compat.py has no _pytest.* imports of its own either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

OpaqueNodeId.as_opaque() returns self, letting callers holding a NodeId | OpaqueNodeId value call .as_opaque() unconditionally without checking which concrete type they have. This replaces the standalone to_opaque_node_id() free function, which is no longer needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Adds an assert_never arm for exhaustiveness, matching the pattern already used elsewhere in the codebase. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

The module is already under the _pytest private package, so an extra leading underscore on the module name itself is redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Upstream renamed CallSpec2 to CallSpec (pytest-dev#14742). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

bluetech

Interesting work!

pytest identified every collection-tree node with a plain "::"-joined nodeid string, repeatedly re-parsed (split/partition) at dozens of call sites for cache persistence, terminal/JUnit reporting, and stepwise/ subtests bookkeeping.

Is the rationale here performance? If so, is there a way to show it is indeed faster?

Function's NodeId also carries structured per-parametrize-call data (ParamId: id/argnames/scope) when built from live collection data, laying groundwork for future scope-aware scheduling (e.g. in pytest-xdist) without committing any consumer to it yet.

I don't understand what this means, can you expand a bit? I think this explains why ParamId contains argnames and scope, which is otherwise unexpected since they're not present in the string nodeid and are conceptually redundant, so I'd like to understand it better.

bluetech · 2026-07-23T21:00:52Z

+  building further collection-tree structure on top of one is a static
+  type error, not just a runtime mistake.
+- :class:`OpaqueNodeId` -- A nodeid reconstructed from an external string
+  source, rather than from live collection. It this no claim to structured


Grammar issue here ("It this no claim")

bluetech · 2026-07-23T21:04:20Z

+    from typing_extensions import Self
+
+
+@dataclasses.dataclass(frozen=True)


I recommend also slots=True, kw_only=True on all dataclasses.

bluetech · 2026-07-23T21:06:00Z

+from typing import TYPE_CHECKING
+from typing import TypeVar
+
+# This module must stay a dependency-free leaf: it is imported from


I would remove this comment, I mean it is only pertinent for someone who tries to add imports to this file, and that someone would soon find out the problem if they try anyway...

bluetech · 2026-07-23T21:13:03Z

+    scope: Scope | None = None
+
+
+class _CachedStrEqHashMixin:


Why not make it a BaseNodeId ABC? I think it would be nicer/clearer than a mixin class, because mixin classes are the devil's handiwork.

bluetech · 2026-07-23T21:26:30Z

+    def __eq__(self, other: object) -> bool:
+        if not isinstance(other, _CachedStrEqHashMixin):
+            return NotImplemented
+        return str(self) == str(other)
+
+    def __hash__(self) -> int:
+        return hash(str(self))


What's the rationale for the string-based eq/hash implementation -- performance or semantic?

By semantic I mean: let N1 and N2 be two different structured nodeids that map to the same string nodeid (e.g. due to the - ambiguity described in the module docstring), do we intentionally want N1 == N2?

bluetech · 2026-07-23T21:45:09Z

+        """
+        return self._id

    def __hash__(self) -> int:


Preexisting: it's unclear to me why Node hashes via the nodeid. Node is an object with an identity, its hash should just be based on its id(self) (=> the default hash implementation). I.e. I think we should just remove the __hash__.

bluetech · 2026-07-23T21:47:03Z

        fspath: None = None,
        path: Path | None = None,
-        nodeid: str | None = None,
+        nodeid: str | NodeId | None = None,


Keeping support for str here for backward compat?

bluetech · 2026-07-23T21:56:27Z

+    from an external string (e.g. JSON-deserialized via ``_from_json``, or
+    assigned through the ``nodeid`` setter below).
+
+    Deliberately not added to :class:`BaseReport` itself: several tests


How hard would it be to adjust the tests?

bluetech · 2026-07-23T21:58:22Z

+    def nodeid(self) -> str:
+        return str(self._id)
+
+    @nodeid.setter


Hmm is there code that mutates report.nodeid?

bluetech · 2026-07-23T21:59:52Z

+    def nodeid(self, value: str) -> None:
+        self._id = OpaqueNodeId.parse(value)
+
+    @property


Both subclasses override this so maybe remove from the mixin?

psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jul 22, 2026

RonnyPfannschmidt requested changes Jul 22, 2026

View reviewed changes

Comment thread src/_pytest/_nodeid.py Outdated

Comment thread src/_pytest/_nodeid.py Outdated

RonnyPfannschmidt reviewed Jul 22, 2026

View reviewed changes

Comment thread src/_pytest/_nodeid.py Outdated

nicoddemus requested a review from RonnyPfannschmidt July 22, 2026 12:36

RonnyPfannschmidt reviewed Jul 22, 2026

View reviewed changes

Comment thread src/_pytest/nodeid.py

RonnyPfannschmidt reviewed Jul 22, 2026

View reviewed changes

Comment thread src/_pytest/cacheprovider.py Outdated

RonnyPfannschmidt reviewed Jul 22, 2026

View reviewed changes

Comment thread src/_pytest/cacheprovider.py Outdated

RonnyPfannschmidt mentioned this pull request Jul 22, 2026

introduce fixture scope and dependency based sort key for sorting tests #1269

Closed

nicoddemus marked this pull request as ready for review July 22, 2026 15:20

nicoddemus force-pushed the refactor-node-ids branch from 877b612 to 957b2d3 Compare July 22, 2026 15:20

nicoddemus requested a review from RonnyPfannschmidt July 22, 2026 15:21

nicoddemus force-pushed the refactor-node-ids branch from 0339c53 to da6d73c Compare July 23, 2026 07:30

RonnyPfannschmidt requested changes Jul 23, 2026

View reviewed changes

Comment thread src/_pytest/_nodeid.py Outdated

Comment thread src/_pytest/_nodeid.py Outdated

Comment thread src/_pytest/_nodeid.py Outdated

Comment thread src/_pytest/_nodeid.py Outdated

Comment thread src/_pytest/nodes.py

nicoddemus requested a review from RonnyPfannschmidt July 23, 2026 08:52

The-Compiler requested changes Jul 23, 2026

View reviewed changes

nicoddemus requested a review from The-Compiler July 23, 2026 09:32

nicoddemus and others added 13 commits July 23, 2026 12:14

Add changelog entry for the NodeId refactor

5247545

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Improve docs

7716077

Remove NodeId.fspath, redundant with NodeId.path

7f98bb0

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Assert the actual nodeid value in test_to_json_nodeid_wire_shape

6cac855

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

nicoddemus and others added 5 commits July 23, 2026 12:14

Use an explicit match statement in coerce_node_id

dfdc44c

Adds an assert_never arm for exhaustiveness, matching the pattern already used elsewhere in the codebase. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Review changes

8699336

Move comment

475cd64

Rename _pytest._nodeid to _pytest.nodeid

9ed3457

The module is already under the _pytest private package, so an extra leading underscore on the module name itself is redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

nicoddemus force-pushed the refactor-node-ids branch from 33f8694 to 9ed3457 Compare July 23, 2026 10:18

Fix stale CallSpec2 reference in ParamId docstring after rebase

07039ee

Upstream renamed CallSpec2 to CallSpec (pytest-dev#14742). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

bluetech reviewed Jul 23, 2026

View reviewed changes

		from typing_extensions import Self


		@dataclasses.dataclass(frozen=True)

Uh oh!

Uh oh!

Conversation

nicoddemus commented Jul 22, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicoddemus commented Jul 23, 2026

Uh oh!

The-Compiler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluetech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants