Skip to content

test(purchase): hold back later migrations when staging 000127 - #1060

Merged
cristim merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1059
Oct 17, 2026
Merged

cristim merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1059

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 17, 2026

Copy link
Copy Markdown
Collaborator

Problem

TestMigration_NeverStartedLegacyRampsBecomeNew (from #905) stages every migration except 000127_never_started_legacy_ramps_to_new, runs the migrator, then adds 000127 and runs it again. When a migration newer than 000127 exists, the first run already takes the schema past 127. golang-migrate never applies a version below the current one, so 000127 is skipped and the test fails:

expected: "new"
actual  : "legacy"
Messages: never started, no executions

This is the Integration Tests failure on #1022 (000129). #1020 (000128) and every later migration would hit it too.

Fix

stagedMigrations now copies only the migrations that sort before 000127 (zero-padded prefixes sort in version order). addTarget then copies 000127 and every later migration. The test still checks an upgrade from just before 000127 with the real runner, however many migrations follow. This is a test-only change.

Closes #1059

Verification

Integration tests on testcontainers Postgres, run with AWS_CONFIG_FILE=/dev/null:

  • Fails before the fix, with a later migration present: I added a throwaway 000999_tmp_probe (up/down SELECT 1;) and ran the test on the old helper. It fails with the same expected "new", actual "legacy" seen on fix(scheduler): record partial multi-subscription sweeps in collection state #1022.
  • Passes with the fix: the same tree with the probe passes.
  • Real tree: with the probe removed, the test passes with the fix.
  • Checks: go vet -tags integration ./internal/purchase/ and golangci-lint (--build-tags integration, 0 issues) are clean.

Once this merges, #1020 and #1022 need main merged in to pick it up.

Please mirror the issue's labels: priority/p1, severity/medium, urgency/now, impact/few, effort/xs, type/bug.

🤖 Generated with Claude Code

stagedMigrations ran every migration but 000127 first, so any later
migration (LeanerCloud#1020's 000128, LeanerCloud#1022's 000129) moved the schema past 127 and
golang-migrate never applied it on the second run. Stage only migrations
that sort before 000127 and add it together with every later one.

Closes LeanerCloud#1059

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p1 Next up; this sprint severity/medium Moderate harm urgency/now Drop other things impact/few Limited audience effort/xs Trivial / one-liner type/bug Defect labels Oct 17, 2026
@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 52682743-a9bc-4c52-976d-9991e95f67c8
📥 Commits

Reviewing files that changed from the base of the PR and between ca2120e and 501f8d7.

📒 Files selected for processing: 1
  • internal/purchase/plan_step_ledger_migration_integration_test.go
+------------------------------------------------------------+
| To infinity and beyond! Scouring your code for pesky bugs. |
+------------------------------------------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@cristim

cristim commented Oct 17, 2026

Copy link
Copy Markdown
Member

Independent review verdict: CLEAN at 501f8d7

Reviewer: fresh Claude agent (Sonnet 5.5, high effort), not the author, cold brief. The PR body and #1059 were treated as claims. Coordinator: claude-cc-reviewer.

Blocking findings: none.

Verified

  • Hold-back logic (plan_step_ledger_migration_integration_test.go:37-52):
    • Every .sql file that sorts at or after 000127_never_started_legacy_ramps_to_new is held back, and both of its up/down pairs stay together.
    • *_test.go files in the migrations dir are skipped by the .sql extension filter.
    • The first run applies only versions below 127, so the seed lands at schema 126 exactly. The test still proves that 000127 flips only the never-started plan.

Fail before / pass after. Run with Docker and GOWORK=off: go test -tags integration -run TestMigration_NeverStartedLegacyRampsBecomeNew ./internal/purchase/ -count=1. "Later migration" means #1020's 000128 copied in.

Test file Later migration Result
fix none PASS
fix 000128 PASS
origin/main 000128 FAIL (expected new, actual legacy)
origin/main none PASS

Other tests: no other test uses this staging pattern. azure_pricing_integration_test.go pins with MigrateToVersion, which is safe when later migrations exist. go vet -tags integration is clean.

Non-blocking: the e.Name() < neverStartedMigration comparison relies on 6-digit zero-padded versions. All 244+ files are padded that way, and the code comment says so.

Gate

Merge: the owner authorized the merge drivers to ready and merge drafts that meet the gate. This PR is p1, and it unblocks #1020 and #1022, so I am marking it ready and merging.

@cristim
cristim marked this pull request as ready for review October 17, 2026 16:50
@cristim
cristim merged commit 22384a1 into LeanerCloud:main Oct 17, 2026
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/xs Trivial / one-liner impact/few Limited audience priority/p1 Next up; this sprint severity/medium Moderate harm type/bug Defect urgency/now Drop other things

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test(purchase): NeverStartedLegacyRampsBecomeNew skips 000127 once a later migration exists

2 participants