Skip to content

Preserve NodeId identity across string serialization - #15046

Open
host452b wants to merge 1 commit into
pytest-dev:mainfrom
host452b:fix/nodeid-cache-identity
Open

host452b wants to merge 1 commit into
pytest-dev:mainfrom
host452b:fix/nodeid-cache-identity

Conversation

@host452b

Copy link
Copy Markdown

Fixes #15045.

Custom item names containing [ or :: can produce different structured fields when their nodeid strings are read back from a cache. This makes --lf omit a still-failing item when another cached failure matches, and makes --sw lose the resume point.

Compare and hash NodeId instances by their public string form while retaining the structured fields for callers. This preserves item identity across serialization without changing the cache format. Regression tests cover string round trips, hashing, distinct literal IDs, and fresh-process --lf/--sw runs with custom items.

Validation:

  • Before the fix, 16 new regression cases failed; the focused NodeId/cacheprovider/stepwise suites now pass (128 passed, 2 skipped).
  • The issue's standalone reproducer changes from exit sequence [1, 0, 1] to [1, 1, 1].
  • A failure cache written by the unpatched version is read correctly by the fixed version.
  • Project pre-commit checks pass, including mypy.
  • Full suite on Python 3.12.3: 4622 passed, 50 skipped, 13 xfailed, 7 xpassed (exit 0), using PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest -q -p _hypothesis_pytestplugin --tb=short.

With autoload disabled and Hypothesis not explicitly loaded up front, the full suite hits a HypothesisSideeffectWarning in test_version_verbose. The same failure reproduces on unmodified main, including a two-test sequence; the command above loads the plugin before running tests. No warning filters or test exclusions were added.

AI assistance: OpenAI Codex helped investigate, implement, and test this change and is credited in the commit trailer.

Custom item names can contain delimiters that parse() interprets as
structured fields. Compare and hash by the public nodeid string so
lastfailed and stepwise can recognize cached custom items.

Fixes pytest-dev#15045.

Co-authored-by: OpenAI Codex <noreply@openai.com>
@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Sep 17, 2026
@host452b

Copy link
Copy Markdown
Author

@RonnyPfannschmidt could you help to review this ?

@RonnyPfannschmidt

Copy link
Copy Markdown
Member

@nicoddemus i think this just demonstrates the practical version of my initial dislike of having no distinction between a opaque/reparsed id and a real id - back at the sprint i couldn't quite put my finger on it beacuse it indeed only occurs when someone moves out of the normal bounds

the fix here seems like a stopgap but the full resolution seems at best daunting so we might need to stick with it for a bit

@host452b

Copy link
Copy Markdown
Author

The job is ubuntu-py313-pexpect, running tox -e py313-pexpect-coverage from .github/workflows/test.yml (lines 206–211 and 303–308).

The visible log excerpt does not contain the failing test or traceback; it only shows the final summary:

1 failed, 4622 passed, 49 skipped, 15 xfailed, 5 xpassed

The five XPASS results are debugger tests in testing/test_debugging.py, including:

  • TestPDB::test_pdb_with_injected_do_debug
  • TestDebuggingBreakpoints::test_pdb_not_altered
  • test_pdb_suspends_fixture_capturing[capfd]
  • test_pdb_suspends_fixture_capturing[capsys]

These are already marked non-strict:

@pytest.mark.xfail(reason="#10042", strict=False)

Therefore, the XPASS results are not causing the job failure. The fix must target the separate test whose traceback is missing from the supplied log.

The most likely corrective change for this PR is to update the remaining debugger regression test expectations consistently with the new capture behavior. The affected tests currently use exact pexpect output matching, for example:

child.expect(r"PDB set_trace \(IO-capturing turned off\)")
child.expect(r"PDB continue \(IO-capturing resumed\)")

If the implementation now includes fixture-specific status text, update the expectations to match the new output, as already done in test_pdb_suspends_fixture_capturing:

child.expect(
    rf"PDB set_trace \(IO-capturing turned off for fixture {fixture}\)"
)
child.expect(
    rf"PDB continue \(IO-capturing resumed for fixture {fixture}\)"
)

Also retain the Python 3.15 REPL workaround in the autouse fixture:

pytester._monkeypatch.setenv("PYTHON_BASIC_REPL", "1")

A correct patch should therefore:

  1. Leave the non-strict xfail markers in place; XPASS is allowed.
  2. Adjust only the failing pexpect assertion to accept the new debugger status message.
  3. Avoid weakening the job or marking the actual failure as xfail.
  4. Run the focused test set with:
tox run -e py313-pexpect-coverage -- \
  testing/test_debugging.py -k pdb

The exact failing assertion cannot be identified from the excerpt because the failure traceback is absent, but the workflow and test source confirm that the failure is in the Python 3.13 pexpect debugger coverage job, not in the reported XPASS results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--last-failed can silently skip failing custom items with brackets in their names

2 participants