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!

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:58:22Z

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


Hmm is there code that mutates report.nodeid?

Not internally, but there might plugins out there that do it, so I decided to just implement a wrapper and convert to a OpaqueNodeId internally.

Alternatively, we can use the setter to raise an error instead.

nicoddemus · 2026-07-24T07:28:53Z

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

No, AFAIU we always found node ids to be a bit hacky, because while the original idea was to produce an opaque unique id for test items, in the end we always end up parsing it to extract information (such as the path to the file relative to the root), so it makes sense architecturally to use a data structure for it, with the information plainly available.

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.

@RonnyPfannschmidt might elaborate a bit given he asked to add this information, but I assume is that given we can now have a richer data structure, we might as well store more information on it.

RonnyPfannschmidt · 2026-07-24T07:40:24Z

there are about 3 key details that always created a headache

a) we split node-ids all over the place in absolutely hackish ways - this has been a recurring source of grave errors
b) in mutliple venues of communications we ended up with nodeids that are broken in some ways (one of the msot daunting examples being loadscope groups tacking strings on front of nodeids which in turn breaks consistent reporting
c) parameters in nodeids where always completely lossy - now we have a bare minimum of metadata to make sense of/order nodes

and then there's a number of other issues relating to that - so this really is about removing a mess and pain

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>

Replaces the _CachedStrEqHashMixin with an ABC (_CachedStrEqHash), and turns the object.__setattr__-based _str caching trick into a proper init=False dataclass field (renamed to _str_cache) on each concrete class, so slots=True gives it a real slot. The ABC declares its own __slots__ = () explicitly, since abc.ABC's __slots__ = () does not propagate to subclasses that don't redeclare it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…NodeId These three classes no longer share a base class and no longer compare equal to each other just because their str() forms coincide -- an OpaqueNodeId reconstructed from an untrusted external string (an on-disk cache file, xdist JSON, ...) should never be silently treated as "the same node" as a live, trustworthy CollectionNodeId/ItemNodeId. Same-type equality/hashing is now handled by each dataclass's own generated __eq__/__hash__ instead of a shared, string-based implementation; str() caching is reimplemented independently on each class. Traced every place that could rely on the old cross-type behavior at runtime and fixed each real dependency by normalizing explicitly via .as_opaque() before comparing/storing, mirroring the pattern already used by cacheprovider.py's LFPlugin/NFPlugin: - stepwise.py: StepwiseCacheInfo.last_failed is compared against a live item.id/report.id after being loaded from the on-disk cache as an OpaqueNodeId -- this is the entire mechanism --stepwise uses to find where to resume. - terminal.py: _progress_nodeids_reported/_timing_nodeids_reported and the teardown-section lookup helpers process every report in a run, including SubtestReport, whose .id is always OpaqueNodeId (built via a JSON round-trip even in-process), so they'd otherwise double-count a subtest and its enclosing test as different nodes. - junitxml.py: LogXML.node_reporters has the same SubtestReport-vs- enclosing-test issue -- without normalizing, a failed subtest and its enclosing test's failure ended up as two separate <testcase> elements in the JUnit XML instead of one (verified by hand before/after). testing/test_reports.py's JSON round-trip test is updated to compare the reconstructed report's _id by string instead of by equality, since a deserialized report's _id is now correctly a different (less trusted) type than the original live report's. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Session and FSCollector, the only two real callers, both build a nodeid that is always effectively a bare CollectionNodeId (Session's literal "", FSCollector's filesystem-relative path -- neither ever contains "::"), so both can build CollectionNodeId directly instead of going through Node.__init__'s string-splitting fallback. This also closes a latent type-safety gap: Item.__init__ already narrows its own signature to ItemNodeId | None to prevent an Item ending up with a non-ItemNodeId _id, but that was only a static restriction -- the shared Node.__init__ body didn't itself guard against a caller ignoring the type hint and passing a raw str through an Item subclass, which would have silently produced a CollectionNodeId in an Item's _id slot. Removing str support from Node.__init__ entirely closes this structurally rather than by convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Guards against external plugins that may still be passing a raw nodeid string (now unsupported), raising a clear error pointing at Node.from_parent() instead of silently misbehaving. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Node hashed by nodeid but never overrode __eq__ (stays identity-based), so the custom hash provided no actual behavioral difference for any dict/set correctness -- collisions are always disambiguated via ==, which is identity-based here regardless of hash value. Every real dict/set of Node/Item/Collector objects in the codebase already relies on identity semantics (e.g. main.py's _collect_one_node explicitly notes fixture registration is "keyed by node identity"). Removing it lets Node fall back to the default identity-based object.__hash__, which is the behaviorally-correct pairing for its default identity-based __eq__. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

_WithNodeId was only ever used as BaseReport's sole base and had no other consumers left (isinstance(x, _WithNodeId) checks were already removed), so there was no reason to keep it as a separate mixin class -- its nodeid/id properties now live directly on BaseReport. Also rewrites junitxml.LogXML.node_reporter's initial if/else as an explicit match statement with a final assert_never arm for exhaustiveness, and removes the now-stale _WithNodeId nitpick_ignore entry from doc/en/conf.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

nicoddemus · 2026-07-24T13:50:19Z

@RonnyPfannschmidt @bluetech @The-Compiler please take another look. 👍

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 force-pushed the refactor-node-ids branch from 33f8694 to 9ed3457 Compare July 23, 2026 10:18

bluetech reviewed Jul 23, 2026

View reviewed changes

The-Compiler force-pushed the main branch from cb2ee5b to ced9022 Compare July 24, 2026 09:22

nicoddemus and others added 8 commits July 24, 2026 15:34

Add changelog entry for the NodeId refactor

0de5e2d

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

Improve docs

36ed5fe

Remove NodeId.fspath, redundant with NodeId.path

628ce3a

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

Assert the actual nodeid value in test_to_json_nodeid_wire_shape

24423d0

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

nicoddemus and others added 19 commits July 24, 2026 15:34

Use an explicit match statement in coerce_node_id

f6ef95e

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

f7029fa

Move comment

354e153

Rename _pytest._nodeid to _pytest.nodeid

deae8b0

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>

Fix stale CallSpec2 reference in ParamId docstring after rebase

b8552b6

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

Fix grammar

83b9109

Doc changes

0a13d23

nicoddemus force-pushed the refactor-node-ids branch from 07039ee to 67c9d54 Compare July 24, 2026 13:38

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bluetech Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

nicoddemus Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nicoddemus commented Jul 24, 2026

Uh oh!

RonnyPfannschmidt commented Jul 24, 2026

Uh oh!

nicoddemus commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants