test: remove non-enforcing snapshot drift CI step (Closes #415) - #468
Open
Nife-tanny wants to merge 1 commit into
Open
test: remove non-enforcing snapshot drift CI step (Closes #415)#468Nife-tanny wants to merge 1 commit into
Nife-tanny wants to merge 1 commit into
Conversation
The 'Snapshot drift detection' CI step was a no-op: it only printed advice when .snap fixtures existed, but never failed on drift, and it did not even fire because contracts/test_snapshots/ does not exist and is gitignored. No snapshot-golden mechanism exists in the Rust tests (SNAPSHOT_UPDATE is never read by any code), so there is no baseline to commit and no way to ever produce one that survives a commit. Remove the dead step and the orphaned tooling/docs that only existed to support it (scripts/update_snapshots.sh, the CONTRIBUTING/README/PR template snapshot sections). A maintainer who wants real snapshot testing can build a proper harness (e.g. insta) later, but as-is this step misled contributors into thinking drift was being caught. Closes TevaLabs#415
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.
Overview
Path B — Remove it.
Resolves GitHub issue #415 by removing the non-enforcing "Snapshot drift detection" step from CI rather than half-committing to a snapshot baseline that cannot exist in this repo today.
Why Path B (not Path A)
Investigation of the current state revealed that the snapshot step was pure fiction — it never printed its advice, could never fail, and there is no snapshot-golden mechanism in the codebase to enforce:
No snapshot mechanism exists in the code.
scripts/update_snapshots.shsetsSNAPSHOT_UPDATE=1, but no Rust test ever reads that env var. There is noinsta, no.snapreader/writer, and no golden-file harness anywhere incontracts/src/. Thecontracts/test_snapshots/directory it targets does not exist anywhere in the repo.The directory is gitignored and thus cannot be committed.
contracts/test_snapshots/is listed in.gitignore. Even if a generator existed, emitted fixtures would never be tracked, so there is no committable baseline — and committing a "verified current" one is impossible because no generator exists to produce it.The CI step is a total no-op. Its guard (
if [ -d contracts/test_snapshots ] && ls .../*.snap) is always false (the directory never exists), so the step always printed only"No snapshot golden files to check."It has never printed the drift advice it was written to print, and never failed.Nobody owns it. The single commit that introduced it (
e73bb77"Add snapshot policy and update script for snapshot management") added a CI message + docs + a script, but no implementation and no fixtures, and nothing was ever committed afterward (contracts/test_snapshots/remains empty/absent).Path A would have required building an entire new snapshot-testing framework from scratch (a real generation harness, deciding what to capture, unwiring the gitignore, and a maintained baseline) — far beyond "enforce this existing step," and the issue explicitly asks for the honest choice rather than defaulting to A because it "sounds more thorough." There is no current baseline to commit, so Path B (remove the misleading step) is the correct and honest resolution.
What was removed
.github/workflows/ci.yml— removed theSnapshot drift detectionstep from therust-testjob (lines 59–71). Standalone step; no other job/step greps its output, andci-successdoes not reference it. Job ordering, caching, and downstream steps are unaffected.scripts/update_snapshots.sh— removed. Orphaned tooling that only existed to support the dead step; grep confirmed no other active script references it.CONTRIBUTING.md— removed the "Snapshot Tests" section describing the non-existent golden-file workflow..github/PULL_REQUEST_TEMPLATE.md— removed the "Snapshot policy" checklist section.README.md— removed stale references tocontracts/test_snapshots/(directory tree entry, "NOT Committed" list, git-status-check note/comment)..gitignore'scontracts/test_snapshots/entry was left in place (inert but harmless; kept to minimize scope).Reinstate real snapshot testing later
A future maintainer who wants real drift detection should build an actual snapshot harness — e.g. integrate
instaintocontracts/src/tests/, capture storage/event output from the existing test-utils scenarios, remove thecontracts/test_snapshots/entry from.gitignoreso fixtures can be committed, add aSNAPSHOT_UPDATE-style regeneration path that the tests actually honor, and wire a hard-failing diff check into CI.Verification
contractsYAML (ci.yml) validated as valid YAML (js-yaml).git diff --name-only origin/main...HEAD(empty for*.rs/Cargo.*). This change is CI config, docs, the PR template, and the deletion of a dead bash script.Local verification of standard commands
cargo fmt --all -- --check: fails (pre-existing) with an identical 4772-line diff on bothorigin/mainand this branch. This is a pre-existing repository condition unrelated to this change — confirmed by running it in a clean worktree oforigin/main.cargo test --workspace --locked: could not complete locally due to a rustc compiler ICE (internal compiler error) while compiling dependency build scripts (proc-macro2,zmij) under the local Windows x86_64 + rustc 1.97.1 toolchain. This is a local environment/toolchain limitation that occurs before any of the repo's own code is compiled; it is unrelated to this change (which modifies no Rust/Cargo files) and would occur identically on the base branch. CI runs onubuntu-latestand is unaffected.No new failures were introduced by this change; the pre-existing
cargo fmtfailure and the local toolchain ICE are flagged above and left untouched.Closes #415