Skip to content

refactor(testing): deduplicate the fixture output-path computation#853

Merged
tcoratger merged 2 commits into
leanEthereum:mainfrom
tcoratger:refactor/dedupe-fixture-output-path
Jun 6, 2026
Merged

refactor(testing): deduplicate the fixture output-path computation#853
tcoratger merged 2 commits into
leanEthereum:mainfrom
tcoratger:refactor/dedupe-fixture-output-path

Conversation

@tcoratger
Copy link
Copy Markdown
Collaborator

Motivation

Item 10 of the packages/testing/ audit (follow-up to #848#852). The fixture output-path computation in framework/pytest_plugins/filler.py existed twice — once in add_fixture (report metadata, during the test) and once in write_fixtures (actual write paths, at session end) — as independently maintained near-copies that had to stay in lockstep by discipline alone. Both carried a silent except ValueError: relative_path = test_file fallback that would write fixtures under a wrong location rather than failing.

Changes

  • One source of truth: a _split_test_nodeid helper + a FixtureCollector.fixture_output_file method replace both copies; the path reported into report.user_properties and the path written at session end are now provably the same
  • Fail loudly: the silent fallback becomes a raised ValueError naming the offending nodeid and the expected tests/consensus root
  • Grouping by output path: the session-end write groups fixtures by their computed output file instead of a parallel (file, function, format) tuple key
  • Exact suffix strip: fixture_format.replace("_test", "")removesuffix("_test") — the old global replace would silently rename a future format containing _test mid-name
  • Dead plumbing removed: the test_name parameter and tuple slot were stored but never read

Review panel (py-architect + code-tester)

Both specialists reviewed the diff independently:

  • code-tester (verdict: ship as-is): traced every consumer of the touched surface (add_fixture/write_fixtures each have exactly one caller; report.user_properties has no downstream consumer — no JUnit/JSON reporter configured, so dropping test_name is safe). Empirically probed the fallback-removal risk: the fill CLI pins --rootdir to the workspace root, so pytest nodeids are rootdir-relative regardless of cwd or absolute path args — verified with four invocation styles that the fallback was unreachable dead code. Filled tests/consensus/lstar/ssz on branch and main: trees and contents byte-identical.
  • py-architect (verdict: ship with one fix, applied): requested the removesuffix hardening (the only old-vs-new behavioral edge: two formats differing by a _test suffix would previously have silently last-write-won the same file with mismatched metadata; now they'd merge consistently — strictly more correct, and unreachable today since all 13 registered format names strip uniquely). Also suggested the docstring note linking the raise to the collection guard — applied.

Verification

  • just check passes (ruff, format, ty, codespell, mdformat)
  • Smoke fill before/after the removesuffix fix: 118/118, output trees identical
  • Independent reviewer fill comparison vs main: byte-identical

🤖 Generated with Claude Code

tcoratger and others added 2 commits June 6, 2026 00:37
The output-path logic existed twice, once computing report metadata
during the test and once computing write paths at session end, as
independently maintained near-copies. Both carried a silent fallback
that, for a test file outside the consensus spec tree, wrote fixtures
under a wrong location instead of failing.

Changes:

- one node-id splitting helper and one output-file method replace the
  two copies; the reported path and the written path can no longer
  drift apart
- the silent fallback becomes a raised error naming the offending
  node id and the expected consensus test root; review confirmed the
  fallback was unreachable under the real CLI, which pins the pytest
  root directory to the workspace root
- the session-end write groups fixtures by their computed output file
  instead of a parallel tuple key
- the format directory derivation strips the test suffix exactly,
  instead of replacing the substring anywhere in the format name
- the test-name parameter and tuple slot were stored but never read;
  they are removed end-to-end

Emitted fixture trees are byte-identical to main.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tcoratger tcoratger merged commit 0bfa23b into leanEthereum:main Jun 6, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant