From 72cb25111fcdcef634284cf0222c9289c8d12cfb Mon Sep 17 00:00:00 2001 From: Mathias Date: Thu, 2 Jul 2026 14:49:02 +0200 Subject: [PATCH] test(store): address migrations by version, not step count (#8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The up/down migration tests stepped a hard-coded number of Steps(-N)/Steps(+N) down from HEAD and back. The counts assumed a specific latest migration, so adding one shifted every count by one and unrelated tests (010/011/014) went red with confusing off-by-one symptoms — a papercut on every new migration. Drive the schema to an exact version with m.Migrate(version) via two helpers (headVersion, migrateTo). Each test now steps to just below its target by version, asserts the down effect, steps up to the target, asserts the up effect, then restores to the captured HEAD. A migration added on top changes HEAD but shifts no count, so no test needs editing. Verified by adding a throwaway migration 017 on top: all four tests stayed green with zero edits. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_015QbdxXWxLefS5AwLN5eyze --- internal/adapters/store/migrate_test.go | 85 ++++++++++++++----------- 1 file changed, 48 insertions(+), 37 deletions(-) diff --git a/internal/adapters/store/migrate_test.go b/internal/adapters/store/migrate_test.go index de48273..9133a30 100644 --- a/internal/adapters/store/migrate_test.go +++ b/internal/adapters/store/migrate_test.go @@ -3,6 +3,7 @@ package store_test import ( "context" "database/sql" + "errors" "os" "testing" @@ -34,6 +35,30 @@ func fileMigrator(t *testing.T) *migrate.Migrate { return m } +// headVersion reports the current (HEAD) schema version so a test can restore +// to it after stepping down, without hard-coding what HEAD is. Adding a +// migration on top changes HEAD but no test that uses this needs editing. +func headVersion(t *testing.T, m *migrate.Migrate) uint { + t.Helper() + v, dirty, err := m.Version() + require.NoError(t, err) + require.False(t, dirty, "schema must not be dirty") + return v +} + +// migrateTo drives the schema to an exact version *by version number*, not by +// step count. This is the whole point of the migrate-test design: a migration +// added above the target does not shift any count here, so unrelated tests stay +// green (see issue #8). ErrNoChange (already at that version) is not a failure. +func migrateTo(t *testing.T, m *migrate.Migrate, version uint) { + t.Helper() + err := m.Migrate(version) + if errors.Is(err, migrate.ErrNoChange) { + return + } + require.NoError(t, err) +} + // loginEventsExists reports whether the login_events relation is present. func loginEventsExists(t *testing.T) bool { t.Helper() @@ -53,25 +78,15 @@ func TestMigration010LoginEventsUpDown(t *testing.T) { require.True(t, loginEventsExists(t), "login_events must exist at latest migration") m := fileMigrator(t) - // 011..016 sit above 010; step them down first so 010 is exercised in isolation. - require.NoError(t, m.Steps(-1), "down 016 drops channel_caption_state, login_events intact") - require.True(t, loginEventsExists(t), "016 down leaves login_events intact") - require.NoError(t, m.Steps(-1), "down 015 reshapes transcripts, login_events intact") - require.True(t, loginEventsExists(t), "015 down leaves login_events intact") - require.NoError(t, m.Steps(-1), "down 014 drops channel_title, login_events intact") - require.True(t, loginEventsExists(t), "014 down leaves login_events intact") - require.NoError(t, m.Steps(-1), "down 013 drops channel_errors, login_events intact") - require.True(t, loginEventsExists(t), "013 down leaves login_events intact") - require.NoError(t, m.Steps(-1), "down 012 is a no-op, login_events intact") - require.True(t, loginEventsExists(t), "012 down leaves login_events intact") - require.NoError(t, m.Steps(-1), "down 011 must not touch login_events") - require.True(t, loginEventsExists(t), "011 down leaves login_events intact") + head := headVersion(t, m) - require.NoError(t, m.Steps(-1), "down 010 must drop login_events") + migrateTo(t, m, 9) // just below 010 — everything above steps down require.False(t, loginEventsExists(t), "login_events must be gone after the down migration") - require.NoError(t, m.Steps(7), "up must recreate 010 then re-apply 011..016") + migrateTo(t, m, 10) // up 010 require.True(t, loginEventsExists(t), "login_events must be restored after the up migration") + + migrateTo(t, m, head) // restore to HEAD for sibling tests } // autoSummarizeDefault reads the users.auto_summarize column default as text @@ -93,21 +108,15 @@ func TestMigration011AutoSummarizeDefaultUpDown(t *testing.T) { require.Equal(t, "true", autoSummarizeDefault(t), "011 sets the default to TRUE") m := fileMigrator(t) - require.NoError(t, m.Steps(-1), "down 016 drops channel_caption_state") - require.NoError(t, m.Steps(-1), "down 015 reshapes transcripts") - require.NoError(t, m.Steps(-1), "down 014 drops channel_title") - require.NoError(t, m.Steps(-1), "down 013 drops channel_errors") - require.NoError(t, m.Steps(-1), "down 012 is a no-op") - require.NoError(t, m.Steps(-1), "down 011 reverts the column default") + head := headVersion(t, m) + + migrateTo(t, m, 10) // just below 011 — reverts the column default require.Equal(t, "false", autoSummarizeDefault(t), "default is FALSE after the down migration") - require.NoError(t, m.Steps(1), "up 011 re-applies the TRUE default") + migrateTo(t, m, 11) // up 011 re-applies the TRUE default require.Equal(t, "true", autoSummarizeDefault(t)) - require.NoError(t, m.Steps(1), "up 012 runs clean (no FORCE RLS on fresh schema)") - require.NoError(t, m.Steps(1), "up 013 creates channel_errors") - require.NoError(t, m.Steps(1), "up 014 recreates channel_title") - require.NoError(t, m.Steps(1), "up 015 reshapes transcripts to shared") - require.NoError(t, m.Steps(1), "up 016 recreates channel_caption_state (HEAD)") + + migrateTo(t, m, head) // restore to HEAD for sibling tests } // channelTitleExists reports whether videos.channel_title is present. @@ -127,17 +136,15 @@ func TestMigration014VideoChannelTitleUpDown(t *testing.T) { require.True(t, channelTitleExists(t), "channel_title exists at latest migration") m := fileMigrator(t) - require.NoError(t, m.Steps(-1), "down 016 drops channel_caption_state, channel_title intact") - require.True(t, channelTitleExists(t), "016 down leaves channel_title intact") - require.NoError(t, m.Steps(-1), "down 015 reshapes transcripts, channel_title intact") - require.True(t, channelTitleExists(t), "015 down leaves channel_title intact") - require.NoError(t, m.Steps(-1), "down 014 must drop channel_title") + head := headVersion(t, m) + + migrateTo(t, m, 13) // just below 014 — drops channel_title require.False(t, channelTitleExists(t), "channel_title must be gone after the down migration") - require.NoError(t, m.Steps(1), "up 014 must recreate channel_title") + migrateTo(t, m, 14) // up 014 recreates channel_title require.True(t, channelTitleExists(t), "channel_title must be restored after the up migration") - require.NoError(t, m.Steps(1), "up 015 restores the shared transcripts shape") - require.NoError(t, m.Steps(1), "up 016 recreates channel_caption_state (HEAD)") + + migrateTo(t, m, head) // restore to HEAD for sibling tests } // TestMigration012FixAutoSummarizeRLS proves 012 runs cleanly and flips any @@ -148,7 +155,11 @@ func TestMigration012FixAutoSummarizeRLS(t *testing.T) { // Round-trip: down 012, then up 012 — must be idempotent. m := fileMigrator(t) - require.NoError(t, m.Steps(-1), "down 012 must not error") - require.NoError(t, m.Steps(1), "up 012 must re-apply cleanly") + head := headVersion(t, m) + + migrateTo(t, m, 11) // down 012 must not error + migrateTo(t, m, 12) // up 012 must re-apply cleanly require.Equal(t, "true", autoSummarizeDefault(t), "default still TRUE after 012 re-applied") + + migrateTo(t, m, head) // restore to HEAD for sibling tests }