Delete 261 orphaned test snapshots, guard against new ones - #442
Merged
Conversation
test_snapshots/ held 895 files; 262 had no test function behind them. They accumulated as tests were renamed or removed -- most of them left by 8716d10, which cut test.rs from 143 test functions to 86 while its message claimed to add 29 tests. Before deleting, checked that none of them pointed at coverage that is actually missing: every orphan group maps to an area with substantial live tests (escrow_interest_yield 81, admin_override 118, cancel_escrow 83, dispute 72, and so on). The genuinely lost ones were the 16 handled in #441; these are stale artifacts of renames and reorganisation. Deleted 261 -- the 262nd reconnected to a test restored in #441. After deletion a full test run regenerated none of them, which confirms no live test owns them. 633 snapshots remain, 0 orphaned. Added scripts/check-orphan-snapshots.py and a CI step ahead of the format check. An orphan on its own is harmless clutter; a pile of them is where a silently deleted test hides. Verified the script actually fails (exit 1) on a planted orphan rather than only passing on a clean tree. 708 tests pass, clippy clean under -D warnings, fmt clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new orphan-snapshot checker currently treats any Rust fn as a “test function,” which can miss true orphans and should be restricted to #[test] functions to match the script’s stated intent.
Pull request overview
This PR cleans up the Soroban snapshot corpus by removing orphaned test_snapshots/ JSON files (snapshots with no corresponding live test function), and adds an automated guard to prevent new orphans from accumulating unnoticed.
Changes:
- Add a Python script to detect committed snapshots whose test function name no longer exists in
contracts/milestone-escrow/src/**/*.rs. - Wire the orphan-snapshot check into CI ahead of formatting/linting.
- Delete a large set of orphaned snapshot JSON files under
contracts/milestone-escrow/test_snapshots/.
File summaries
| File | Description |
|---|---|
| scripts/check-orphan-snapshots.py | New checker script that fails CI if snapshot files are not backed by a live test function name. |
| .github/workflows/ci.yml | Adds a CI step to run the orphan snapshot checker. |
| contracts/milestone-escrow/test_snapshots/**/*.json | Removes orphaned committed snapshot artifacts to reduce noise and prevent masking silently deleted tests. |
Review details
- Files reviewed: 19/264 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
test_snapshots/held 895 files; 262 had no test function behind them. They accumulated as tests were renamed or removed — most left by8716d10, which cuttest.rsfrom 143 test functions to 86 while claiming to add 29.Checked before deleting
An orphan can mean two different things: a test was renamed (harmless), or a test was deleted (lost coverage). Every orphan group here maps to an area that still has substantial live tests:
escrow_interest_yieldtax_withholding_deductionsadmin_override_*cancel_escrow_eventdispute_arbitration_splitThe genuinely lost ones were the 16 handled in #441. These are stale artifacts.
Result
Deleted 261 — the 262nd reconnected to a test restored in #441. A full test run afterwards regenerated none of them, confirming no live test owns them. 633 snapshots remain, 0 orphaned.
Guard
Added
scripts/check-orphan-snapshots.pyand a CI step ahead of the format check. An orphan alone is clutter; a pile of them is where a silently deleted test hides.Verified it actually fails rather than only passing on a clean tree:
708 tests pass, clippy clean under
-D warnings, fmt clean.🤖 Generated with Claude Code