Skip to content

Add YAML-driven plan / apply / dump test harness#7

Merged
winebarrel merged 1 commit into
mainfrom
yaml-test-harness
May 2, 2026
Merged

Add YAML-driven plan / apply / dump test harness#7
winebarrel merged 1 commit into
mainfrom
yaml-test-harness

Conversation

@winebarrel

Copy link
Copy Markdown
Owner

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-runs Plan after Apply and asserts no diff.
  • plan_test.go (TestPlanYAML) — fields: init, desired, plan, error, allow_drop, include, exclude. error substring-matches the returned error and skips the plan 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.

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.0
  • make test (all suites)
  • make test-scenario
  • golangci-lint clean
  • CI green on this PR

🤖 Generated with Claude Code

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>
@winebarrel winebarrel enabled auto-merge May 2, 2026 02:38
@winebarrel winebarrel merged commit 802d61e into main May 2, 2026
2 checks passed
@winebarrel winebarrel deleted the yaml-test-harness branch May 2, 2026 02:40
@codecov

codecov Bot commented May 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.14%. Comparing base (a8d676c) to head (3fa05dd).
⚠️ Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant