fix(tests): a fixture must not inherit a previous run - #89
Merged
Merged
Conversation
Closes #82. `tokenizer_config_wins_over_registry` failed once during a full `cargo test --tests` and never reproduced: assertion `left == right` failed: tier 3 does not fire for this fixture left: TokenizerConfig right: Registry⚠️ AND THE ISSUE I FILED RULED OUT THE ACTUAL MECHANISM, IN WRITING, WITH A CORRECT ARGUMENT AIMED AT THE WRONG QUESTION. It said: "Concurrent test binaries have distinct pids, so two fixtures cannot collide on a name by that route." That answers CAN TWO LIVE PROCESSES COLLIDE. I used it to answer CAN TWO FIXTURES COLLIDE, which is a larger question, and never considered SEQUENTIAL collision across runs. THE MECHANISM: - `CARGO_TARGET_TMPDIR` is NEVER cleaned between runs - `create_dir_all` SUCCEEDS on an existing directory and does NOT clear it - the nonce is pid + atomic counter: unique WITHIN a run, and it RECURS ACROSS runs, because pids are recycled and the counter restarts at 0 Measured on disk before the fix: 14183 stale fixture directories under target/tmp 393 distinct pids represented 6389 already holding a tokenizer_config.json 347 stale dirs for THIS test's own fixture name 343 of those 347 already poisoned The test writes `tokenizer_config.json` at its LAST step while asserting tier 3 fires at its FIRST. On a name+pid+counter recurrence it reads the PREVIOUS RUN's tier-1 file and fails exactly as recorded.⚠️ EVERY OBSERVATION IS EXPLAINED, INCLUDING THE ONE THAT MISLED ME. "Only under a full `cargo test --tests`" is real, but it is a CORRELATE rather than the mechanism: more binaries means more pids consumed, which raises the chance of drawing one a previous run used. A CONDITION THAT RAISES THE PROBABILITY OF A COLLISION LOOKS EXACTLY LIKE THE CAUSE OF IT. FIX: `fresh_dir` removes before it creates and asserts the directory is empty afterwards. Extracted as a named helper deliberately -- a helper that promises "the directory exists" is indistinguishable from one that promises "the directory is YOURS" until something asserts the difference. DETECTOR: `a_fixture_directory_does_not_inherit_a_previous_run` poisons a directory exactly as a previous run would, ASSERTS THE POISONING TOOK (a probe that failed to poison would make the check below pass for the wrong reason), then asserts the fixture comes back clean.⚠️ BORN-RED, AND THE FIRST ATTEMPT FIRED THE WRONG ARM: M1 remove deleted only -> fresh_dir's OWN emptiness guard fired "fixture ... is not empty at creation" M2 remove AND guard deleted -> the regression test's named assertion fired "the fixture inherited a previous run's tier-1 file -- this is issue #82" Two independent guards, each firing on its own arm. M1 alone would have let me claim the regression test works when it had not run. NOT DONE, deliberately: the 14183 stale directories are NOT deleted. The fix makes them harmless, and deleting is a destructive act on a shared checkout for a benefit the fix already provides. chat_template_render: 75 passed, 0 failed. Eleven-step chain green, exit 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JtTf3315ahKPPyBiYugnPh
There was a problem hiding this comment.
Sorry @ciresnave-bot, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 5 days and 2 hours by commenting @sourcery-ai review. Upgrade to get a review now.
Reviewer's GuideThe PR fixes cross-run fixture contamination by replacing File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #82.
tokenizer_config_wins_over_registryfailed once during a fullcargo test --testsand never reproduced:It said: "Concurrent test binaries have distinct pids, so two fixtures cannot collide on a name by that route."
That is a correct argument aimed at the wrong question. It answers can two live processes collide — and I used it to answer can two fixtures collide, which is larger. I never considered sequential collision across runs.
The mechanism
CARGO_TARGET_TMPDIRis never cleaned between runscreate_dir_allsucceeds on an existing directory and does not clear itMeasured on disk before the fix:
The test writes
tokenizer_config.jsonat its last step while asserting tier 3 fires at its first. On a name+pid+counter recurrence it reads the previous run's tier-1 file and fails exactly as recorded.cargo test --tests" is real, but it is a correlate, not the mechanism — more binaries means more pids consumed, which raises the chance of drawing one a previous run used. A condition that raises the probability of a collision looks exactly like the cause of it.Fix
fresh_dirremoves before it creates and asserts the directory is empty afterwards. Extracted as a named helper deliberately: a helper that promises "the directory exists" is indistinguishable from one that promises "the directory is yours" until something asserts the difference.Detector
a_fixture_directory_does_not_inherit_a_previous_runpoisons a directory exactly as a previous run would, asserts the poisoning took (a probe that failed to poison would make the check below pass for the wrong reason), then asserts the fixture comes back clean.Two independent guards, each firing on its own arm. M1 alone would have let me claim the regression test works when it had not run.
Not done, deliberately
The 14183 stale directories are not deleted. The fix makes them harmless, and deleting is a destructive act on a shared checkout for a benefit the fix already provides.
Verification
chat_template_render: 75 passed, 0 failed. Eleven-step chain green at exit 0 — fmt, lockfile, doc, test, featgate, clippy, spm, sweep, e2e, census, dtype.🤖 Generated with Claude Code
https://claude.ai/code/session_01JtTf3315ahKPPyBiYugnPh
Summary by Sourcery
Ensure temporary test fixtures are recreated cleanly so prior runs cannot affect current test results.
Bug Fixes:
Enhancements:
Tests: