Add YAML-driven plan / apply / dump test harness#7
Merged
Conversation
Pistachio-style integration tests that read SQL fixtures from YAML and assert on the corresponding library output. Each fixture file is one top-level YAML map; required fields are documented in the *TestCase struct at the top of each _test.go. - apply_test.go (TestApplyYAML) — fields: init, desired, applied, allow_drop, include, exclude, verify_no_drift. Drift check defaults to true: the harness reruns Plan after Apply and asserts no diff. - plan_test.go (TestPlanYAML) — fields: init, desired, plan, error, allow_drop, include, exclude. The error field substring-matches the returned error and skips the plan-output assertion. - dump_test.go (TestDumpYAML) — fields: init, dump, include, exclude. - test_helper_test.go centralises YAML discovery (runYAMLCases generic helper), Client construction, and desired-SQL temp-file plumbing so the three suites stay short. Eight initial fixtures cover round-trip dump (basic + include filter), plan (no diff / new table / drop with allow-drop), and apply (new table / add column / no diff). All require the same docker-compose MySQL the catalog test already uses. AGENTS.md and TODO.md updated to point contributors at the new harness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
+ Coverage 32.14% 39.14% +7.00%
==========================================
Files 20 20
Lines 1313 1313
==========================================
+ Hits 422 514 +92
+ Misses 806 698 -108
- Partials 85 101 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
winebarrel
added a commit
that referenced
this pull request
May 2, 2026
Seven inline comments, all valid; two are real bugs.
parser/directive.go
- End-of-loop guard in ExtractInlineRenames: a `pending` directive
whose target line never arrived (statement body ran out) is now
appended to Unsupported instead of being silently dropped.
(Copilot #1.)
- leadingBacktickedIdent comment rewritten: previous wording had
curly quotes ("…" instead of `…`) and didn't explain MySQL's
backtick-doubling escape. New comment is precise: doubled
backticks remain unsupported; the regex correctly rejects them
so the validator surfaces "malformed directive". (Copilot #2, #3.)
diff/rename.go
- applyTableRenames / applyColumnRenames / applyIndexRenames now
short-circuit when RenameFrom equals the desired name. This
avoids generating `ALTER TABLE x RENAME TO x` (rejected on some
MySQL versions, no-op on others) for what is almost always a
user typo. (Copilot #5, #6, #7.)
- rewriteConstraintColumnRefs: PRIMARY KEY column lists in
current.Constraints are rewritten old → new alongside the index
and FK rewrites, so a renamed-PK column doesn't surface as
DROP+ADD PRIMARY KEY in diffConstraints. CHECK constraints are
deliberately skipped — their Definition is a free-form
expression and rewriting it requires a full SQL parser.
(Copilot #4.)
diff/tables.go
- Calls rewriteConstraintColumnRefs after applyColumnRenames,
alongside the existing index / FK rewrites.
Tests
- parser/directive_test.go: TestExtractInlineRenamesTrailingPendingUnsupported
covers the end-of-loop pending path.
- diff/rename_test.go: TestDiffRenameSelfRenameIsNoOp covers the
table / column / index self-rename guards in one shot.
TestDiffRenameColumnAlsoRewritesPKConstraint covers the new PK
rewrite end-to-end (renames `users.old_id` → `users.id` and
asserts no DROP/ADD PRIMARY KEY appears).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel
added a commit
that referenced
this pull request
May 2, 2026
Two inline comments, both real bugs.
parser/directive.go
- ExtractStmtRenameFrom now also skips `#` line comments and
single-line `/* … */` block comments in the leading block.
Previously it stopped at the first non-`--` line, so a stray
`# header` or `/* generated */` between such a comment and
`-- myschema:renamed-from` defeated the scan: the validator
saw the directive as known, but the extractor never reached it,
and the rename silently degraded into DROP+CREATE. Mirrors the
same skip logic in ExtractInlineRenames. (Copilot #1.)
diff/rename.go + diff/tables.go
- rewriteFKColumnRefs() now takes (db, name) and, for any FK
whose (RefDB, RefTable) matches that table (i.e. self-
referential), rewrites RefCols too. The cross-table pass in
DiffTables explicitly skips the renamed table itself, so without
this self-ref handling a `users.parent_id REFERENCES users(id)`
FK would diff as DROP+ADD after renaming `users.id`. (Copilot #2.)
- diffTable threads `current.Database, current.Name` into the call.
Tests
- parser/directive_test.go: TestExtractStmtRenameFromSkipsLeadingHashAndBlockComments
covers `#` and `/* … */` lines preceding the directive in the
leading block.
- diff/rename_test.go: TestDiffRenameColumnAlsoRewritesSelfReferentialFK
covers the self-ref RefCols rewrite end-to-end (renames
users.id → users.user_id with a self-referential
fk_parent(parent_id) → users(id), asserts no DROP/ADD on
fk_parent).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
winebarrel
added a commit
that referenced
this pull request
May 3, 2026
#1 Pin the RANGE COLUMNS REORGANIZE end-to-end. The plan-only fixture (partition_value_change_reorganize_range_columns.yml) covered the generator side, but nothing confirmed MySQL accepts `REORGANIZE PARTITION ... INTO (...)` with tuple boundaries or that the rewritten boundaries round-trip cleanly. Add the matching apply / verify_no_drift fixture. #2 The case-only partition-name no-op already has a regression test (`partition_name_case_only_diff_no_op.yml`, commit 4c6998f). #3 Pin the per-partition *option*-only diff (here: COMMENT-only) end-to-end. partitionDefEqual delegates to `parser.FormatPartitionDefinition`, which preserves COMMENT / MAX_ROWS / TABLESPACE / etc., so any byte-different definition already routed through the REORGANIZE branch — but no fixture pinned that MySQL accepts the generated statement or that the rewritten options round-trip. New apply fixture `partition_per_partition_option_change_reorganize.yml` covers both. LIST avoids the RANGE upper-bound cascade so the REORGANIZE only touches the one slot whose option actually changed. #4 / #5 / #6 Widen the Coverage / Not-yet-implemented sections of AGENTS.md (in scope + v1 cuts) and the TODO.md "Already shipped" block from "pure value-change (same names, only `VALUES` differs)" to "per-partition definition rewrite ... when both sides have the same partition names in the same order — covers `VALUES …` boundary tweaks plus COMMENT / MAX_ROWS / TABLESPACE / other per-partition option changes that round-trip through vitess's PartitionDefinition formatter". Brings the docs in line with what diffPartitions actually does (and with what CAVEATS.md "Per-partition definition change" already describes). #7 Fix the misleading comment in partitionNameListEqual. `parser.NormalizePartitionOption` only lower-cases function / column-reference identifiers inside the partition expression — it does NOT lower-case partition names. The case-insensitive `EqualFold` here is needed precisely because the round-trip preserves whatever case the user wrote on the partition name.
winebarrel
added a commit
that referenced
this pull request
May 5, 2026
Round 1 review on PR #75 caught eight issues; this commit addresses all of them. 1. (review #1) loadPreSQL treated any non-empty PreSQL string as "set", including whitespace-only values. Whitespace-only env var (`MYSCHEMA_PRE_SQL=`) would trigger the mutually-exclusive error against a legitimate --pre-sql-file, and skip the no-op short-circuit. Fix: TrimSpace both fields before deciding "set". 2. (review #2) loadPreSQL read --pre-sql-file with os.ReadFile, which doesn't support the repo's existing `-` for stdin convention used by parser.ReadSQLFile (the desired-SQL file args already accept it). Fix: route through parser.ReadSQLFile so `--pre-sql-file=-` works. 3. (review #3 + #4) runPreSQL was invoked AFTER connect(), so flag-validation errors (both flags set, missing file) could be masked by a downstream connection failure, and the client opened a DB connection it then threw away. Split into: - loadPreSQL: validate / read, no DB contact (called BEFORE connect) - execPreSQL: run on conn (called AFTER connect) apply.go and plan.go updated accordingly. 4. CLI-level mutual exclusion: tag both PreSQL and PreSQLFile with kong's `xor:"pre-sql"` so kong rejects the both-set case at parse time before our code sees it. The runtime check in loadPreSQL stays in place for programmatic API callers (Apply / Plan invoked from Go without going through kong). 5. (review #5 / #6 / #7 / #8) Test reshuffle. The original suite over-relied on success-only smoke tests that would have silently passed even if pre-SQL were skipped: - TestApply_PreSQLString and TestApply_PreSQLMultiStatement only checked NoError. - TestApply_PreSQLAppliesToSession claimed to be the "strongest behavioural pin" but probed nothing — the connection it would have probed is closed by Apply's defer. - TestPlan_PreSQLString same issue. Fix: load-bearing pins now feed INVALID pre-SQL and assert the apply / plan aborts with a wrapped "pre-sql" error containing the exact failing piece. The multi-statement split test now uses a payload with an invalid SECOND piece and asserts the error references that piece exactly (proves the split happened — without splitting, the driver would reject the whole concatenated string with a different error). 6. YAML migration: the new harness field `pre_sql` lets inline-payload tests live as testdata/apply/pre_sql_*.yml and testdata/plan/pre_sql_*.yml fixtures (3 + 1 = 4 new fixtures). File-related and programmatic-API tests stay in Go because the YAML harness has no `pre_sql_file` field (would need fixture-relative path handling, out of scope). Coverage is unchanged at the package level; loadPreSQL stays at 100% and execPreSQL is exercised by all the real-execution paths. Co-Authored-By: Claude Opus 4.7 (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.
Summary
Pistachio-style YAML integration tests for the three top-level operations.
apply_test.go(TestApplyYAML) — fixture fields:init,desired,applied,allow_drop,include,exclude,verify_no_drift. Drift check defaults to true: the harness re-runsPlanafterApplyand asserts no diff.plan_test.go(TestPlanYAML) — fields:init,desired,plan,error,allow_drop,include,exclude.errorsubstring-matches the returned error and skips the plan assertion.dump_test.go(TestDumpYAML) — fields:init,dump,include,exclude.test_helper_test.gocentralises YAML discovery (runYAMLCasesgeneric helper), Client construction, and desired-SQL temp-file plumbing.8 initial fixtures cover dump (basic + include), plan (no diff / new table / drop with allow-drop), apply (new table / add column / no diff).
Test plan
go test -v -run YAML .— 8/8 pass against docker-compose MySQL 8.0make test(all suites)make test-scenariogolangci-lintclean🤖 Generated with Claude Code