Skip to content

fix: refuse golden regeneration with xdist workers - #751

Closed
ANO3457 wants to merge 1 commit into
pyrite-wiki:devfrom
ANO3457:fix/golden-regenerate-xdist-712
Closed

ANO3457 wants to merge 1 commit into
pyrite-wiki:devfrom
ANO3457:fix/golden-regenerate-xdist-712

Conversation

@ANO3457

@ANO3457 ANO3457 commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #712.

Refuse PYRITE_CHARACTERIZATION_REGENERATE=1 with effective xdist workers at root pytest startup, before collection or writes, with a clear run serially message. Keep serial regeneration (-n 0, including parallel-default overrides) and ordinary parallel comparison unchanged. Correct the README, golden helper explanation, and all four regeneration command docstrings.

Verification

  • Real pytest subprocess regressions: 12 passed, 1 skipped locally (Windows/Python 3.13).
  • The final same tests against baseline root conftest: 6 behavioral failures / 6 controls passed / 1 skipped; refusals previously let test bodies write.
  • Ruff check passed and format check passed for all seven touched Python files.
  • A POSIX-only control performs actual golden save/load/comparison in a temporary directory and confirms parallel comparison leaves the bytes unchanged. It is skipped on Windows because golden_io uses fcntl.

Test infrastructure caveat

Generic verify-red copies conftest.py into its baseline as test-side code, so it also copies this startup-hook fix and may report unexpected passes. Direct reversal to the original root conftest provides behavioral RED evidence instead; the verifier has not been modified. Linux CI must validate integration and the POSIX control. The whole characterization suite was not run locally; no tracked goldens were changed.

Prepared with Codex.

Submission checks

Reject unsafe regeneration before collection and writes, retain serial regeneration and parallel comparison, and correct every existing regeneration command. Includes subprocess regression tests and changelog.
@ANO3457
ANO3457 marked this pull request as ready for review October 4, 2026 06:33

markramm commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

Automated review. I'm an AI agent that triages pull requests for this project's maintainer. A person reads these and makes the final call. If I've got something wrong, say so here.

Thank you for taking #712. I read the head (e1b9855); its checks pass.

Matches the issue

  • Characterization goldens regenerate only partly under pytest -n (7 of 14), silently #712's wanted property is that regeneration either rewrites every golden or refuses to run under xdist with a message to run serially. pytest_configure in the root conftest.py raises pytest.UsageError ("run serially with -n 0") when PYRITE_CHARACTERIZATION_REGENERATE=1 and xdist workers are in effect, before collection.
  • Property coverage: I grepped the checkout for the variable. It appears in golden_io.py and the four characterization test modules; all four docstrings and golden_io.py are updated in the diff, and the README gains a section.

Tests

  • tests/test_characterization_regeneration.py starts real pytest subprocesses, so it goes through the actual entry point. The body notes that verify-red copies conftest.py into its baseline, so it may report unexpected passes; the author supplied a separate reversal check.

Overlaps another open pull request

Housekeeping

  • The changelog fragment is present.

This is a first pass for the maintainer, who makes the review decision.


Generated by Claude Code

@markramm

Copy link
Copy Markdown
Collaborator

Automated review (Pyrite conductor) of head e1b9855. I'm an AI agent reviewing for the maintainer; comment only, and the maintainer makes the final call. If I've got something wrong, say so here.

Thank you, @ANO3457, and sorry this one waited a week. The narrow bug in #712 is fixed. With the regeneration variable set, every ordinary way of turning xdist on is refused before collection, with a message that names -n 0: -n 4, -n auto, -n logical, --dist ... --tx ..., PYTEST_ADDOPTS, -o addopts=. A normal -n 4 run without the variable is unchanged. The new tests start real pytest subprocesses, and with the hook removed the six refusal tests fail while the seven controls pass. A trial merge onto today's dev is clean.

Two changes before merge

  1. "Rewrites every golden completely" is not yet true on the serial path, and the docstring this PR edits says it is. Every caller passes save the dict it got from load(), so a key the live run no longer produces is kept (tests/characterization/golden_io.py:136). Checked by adding a stale key to goldens/error_bodies.json and regenerating with -n 0: it survives, and the comparison run passes. Either make regeneration drop keys the run did not reproduce (test: stale key in, regenerate serially, key gone), or remove the claim "a key nothing produced this run is deleted" from the docstring and open an issue for it.
  2. Put the refusal where every writer passes. The check lives in the root conftest.py, so --confcutdir tests or --noconftest runs a worker with regeneration on. All six golden writers go through golden_io.save; raising there when PYTEST_XDIST_WORKER is set covers those two cases and any future writer.

Optional

  • The README says regeneration needs a POSIX platform; golden_io.py imports fcntl at module top, so comparison runs need it too. It is also worth saying that every extension must be installed before regenerating, since the matrix goldens depend on the registered tools.
  • tests/test_characterization_regeneration.py:38 uses a fixed timeout=30 on about fourteen subprocesses; under a loaded -n 4 run that is the kind of fixed wall-clock bound this repository treats as a test bug.
  • test_access_deciding_routes.py has no "Regenerate:" line, unlike the four modules updated here.

Recommendation: merge after changes 1 and 2.

@markramm markramm added the state: needs-author Changes requested on the current head: the next move is the author's label Oct 14, 2026
@markramm

Copy link
Copy Markdown
Collaborator

@ANO3457, thank you for this. We haven't heard from you on it for over a week, so we're closing it to keep the queue honest. Nothing is lost: the branch and the review stay here. Reopen it or ping us anytime, and if you'd rather not finish it, say so and we'll carry it with your authorship kept.

@markramm markramm closed this Oct 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state: needs-author Changes requested on the current head: the next move is the author's

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Characterization goldens regenerate only partly under pytest -n (7 of 14), silently

2 participants