test(migrations): prove 0002 purges tombstones and keeps live rows - #140
Merged
Conversation
migrations/embed_test.go says it plainly in its own header - those tests are "about COVERAGE, not about the SQL". So 0002 shipped, was pinned by grandturks (#1004/#1014) and was applied to a deployed cluster without a single assertion that it removes a tombstone. The gap surfaced running FootprintAI/grandturks#977's criterion 4 - "after #970's migration, count(*) WHERE deleted_at IS NOT NULL returns 0" - against ubm01. It returned 0. It had also returned 0 BEFORE the migration ran, because that database had never held a tombstone: the apply reported DELETE 0. The criterion passed while demonstrating nothing, which is sprint #827's failure one level down. This is the missing half, and it cannot be satisfied vacuously: it CREATES a tombstone, asserts it exists, then asserts the migration removed it. A require.NotZero on the fixture makes the vacuous case a hard failure rather than a silent pass. THE FIXTURE IS BUILT THE WAY THE OLD CODE BUILT IT. DocumentCURD.Delete is Unscoped now (#136/#137) and hard-deletes, so it cannot produce the rows this migration exists to purge. A scoped gorm delete is what the pre-#137 code did and what wrote every tombstone in every deployed database. Reproducing that rather than INSERTing a hand-made row keeps the fixture honest - gorm sets deleted_at exactly as production did, and if that ever changes the test changes with it. TWO ASSERTIONS, NOT ONE. "Tombstones are gone" is half the contract; "live rows are untouched" is the half that makes the migration safe to run, and a DELETE with a broken predicate would still satisfy the first. The keeper is read back through the normal path rather than counted, so a row that survived AS A TOMBSTONE would not pass. It also asserts the tombstoned row still holds its `data` payload before the purge - the state #936 objected to, and the reason this is a retention question rather than a tidiness one. A second test covers idempotence, because #970's runbook tells operators "re-running is harmless" and the deployed sites are applied by hand, so a second run is likely rather than hypothetical. Applied from the EMBEDDED copy, not from disk: that is what a consumer pinning this module actually runs. Gated on testing.Short() like every other postgres-backed test here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first CI run - the first time these tests executed against a real postgres - failed here with 'converting NULL to string is unsupported'. ModelDocumentData marshals the caller's map under a json_value key (documents.go:86), so data->>'imageBytes' reads a path that does not exist. The fixture guard above it passed, so the tombstone was created correctly; only this assertion was wrong. Worth noting what NOT to do: weakening it to 'data IS NOT NULL' would have gone green while no longer showing that the PAYLOAD survives, which is the whole reason #936 treated these rows as a retention problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Nothing asserted that
0002does anythingmigrations/embed_test.gosays so in its own header:So
0002_purge_soft_deleted_documentsshipped, was pinned by grandturks (#1004/#1014), and was applied to a deployed cluster — without a single assertion that it removes a tombstone.How the gap surfaced
Running FootprintAI/grandturks#977's criterion 4 against ubm01 — "after #970's migration,
count(*) WHERE deleted_at IS NOT NULLreturns 0".It returned 0. It had also returned 0 before the migration ran, because that database had never held a tombstone; the apply reported
DELETE 0. The criterion passed while demonstrating nothing — sprint #827's failure one level down.What this adds
A test that cannot be satisfied vacuously: it creates a tombstone, asserts it exists, then asserts the migration removed it. A
require.NotZeroon the fixture makes the vacuous case a hard failure rather than a silent pass — that specific failure mode is what prompted this.The fixture is built the way the old code built it.
DocumentCURD.DeleteisUnscopednow (#136/#137), so it hard-deletes and cannot produce the rows this migration exists to purge. A scoped gorm delete is what the pre-#137 code did, and what wrote every tombstone in every deployed database. Reproducing that rather thanINSERTing a hand-made row keeps the fixture honest: gorm setsdeleted_atexactly as production did, and if that behaviour ever changes the test changes with it.Two assertions, not one. "Tombstones are gone" is half the contract; "live rows are untouched" is the half that makes the migration safe to run — a
DELETEwith a broken predicate would still satisfy the first. The keeper is read back through the normal path rather than counted, so a row that survived as a tombstone would not pass.It also asserts the tombstoned row still holds its
datapayload before the purge — the state #936 objected to, and the reason this is a retention question rather than a tidiness one.A second test covers idempotence, because #970's runbook tells operators "re-running is harmless" and the deployed sites are applied by hand, so a second run is likely rather than hypothetical.
Applied from the embedded copy, not from disk — that is what a consumer pinning this module actually runs.
Verification, stated honestly
go vetpasses and the tests skip cleanly under-short. They have not been executed against a real Postgres — I have no Docker locally, so CI'spostgres:16-alpineservice is the first thing that will actually run them.If CI is green, that is the first time anyone has seen
0002remove a row. Worth watching that run rather than assuming it.Gated on
testing.Short()like every other postgres-backed test here.🤖 Generated with Claude Code