test: fill regression-coverage gaps for in-scope features#58
Conversation
PR #58's spec audit found three shapes that look like they should round-trip end-to-end but don't (probed and confirmed). Fixture additions were withheld from PR #58 since pinning broken behaviour without a fix or documented limitation violates the no-spec-change scope of that branch. Land them as TODO entries so they're not lost. All three live under "Medium — silent diffs / fidelity gaps" alongside the existing ENUM/SET and FK-to-other-DB items (same flavour: catalog has the data, but the model / diff layer drops or mishandles it): - CHECK constraint `NOT ENFORCED` — drift loop after first apply. - TIMESTAMP NULL DEFAULT NULL — drift loop emitting `MODIFY COLUMN ts timestamp DEFAULT null` repeatedly. - Table-level `COMMENT='…'` — silent (no diff at all). Each entry documents what the diff layer would need to learn (or, alternatively, where to document the limitation in CAVEATS.md) so a future contributor can pick one up without re-doing the probe.
There was a problem hiding this comment.
Pull request overview
Adds regression fixtures to pin current myschema output for several documented “in-scope” schema shapes (indexes/columns/FKs/views/CLI filters), ensuring make test fails on future accidental drift without introducing behavior changes.
Changes:
- Adds apply fixtures (with
verify_no_drift) for additional supported DDL shapes: index variants, timestamp defaults, generated columns, FK actions, PK changes, and view modifications. - Adds plan fixtures to cover
--includeglob matching and--excludetable filtering behavior. - Expands fixture coverage for ordering-sensitive operations (e.g., DROP+CREATE index, ADD COLUMN before dependent CREATE INDEX).
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testdata/plan/include_filter_glob.yml | Pins --include=tmp_* glob behavior and disallowed drops output. |
| testdata/plan/exclude_filter.yml | Pins --exclude=audit_log behavior (excluded tables invisible to diff/plan). |
| testdata/apply/modify_view_body.yml | Pins view body changes emitting CREATE OR REPLACE VIEW + no-drift. |
| testdata/apply/modify_index_columns.yml | Pins index column-list change as DROP INDEX then CREATE INDEX + no-drift. |
| testdata/apply/cross_table_fk_ripple_on_column_rename.yml | Pins parent-side rename ripple to cross-table FK refcols + no-drift. |
| testdata/apply/create_view_with_column_aliases.yml | Pins views with explicit column-alias list + no-drift. |
| testdata/apply/change_primary_key.yml | Pins PK change ordering (DROP PK then ADD PK) + no-drift. |
| testdata/apply/add_self_referential_fk.yml | Pins same-table FK creation + no-drift. |
| testdata/apply/add_multi_column_unique_index.yml | Pins multi-column UNIQUE index column ordering round-trip + no-drift. |
| testdata/apply/add_multi_column_fk.yml | Pins composite FK column ordering round-trip + no-drift. |
| testdata/apply/add_json_column.yml | Pins JSON column add round-trip + no-drift. |
| testdata/apply/add_index_with_prefix_length.yml | Pins prefix-length index on VARCHAR + no-drift. |
| testdata/apply/add_index_on_blob_with_prefix_length.yml | Pins required prefix-length index on BLOB + no-drift. |
| testdata/apply/add_index_invisible.yml | Pins INVISIBLE secondary index round-trip + no-drift. |
| testdata/apply/add_index_desc.yml | Pins DESC index column modifier round-trip + no-drift. |
| testdata/apply/add_generated_column_stored.yml | Pins generated STORED column round-trip + no-drift. |
| testdata/apply/add_fulltext_index.yml | Pins FULLTEXT index creation path + no-drift. |
| testdata/apply/add_fk_on_update_cascade.yml | Pins FK with ON UPDATE CASCADE round-trip + no-drift. |
| testdata/apply/add_fk_on_delete_cascade.yml | Pins FK with ON DELETE CASCADE round-trip + no-drift. |
| testdata/apply/add_enum_column.yml | Pins basic ENUM column shape round-trip + no-drift. |
| testdata/apply/add_decimal_column.yml | Pins DECIMAL precision/scale formatting round-trip + no-drift. |
| testdata/apply/add_column_with_comment.yml | Pins column COMMENT clause round-trip + no-drift. |
| testdata/apply/add_column_with_alter_hints_instant.yml | Pins alter-hint splice for column-level ADD COLUMN (INSTANT) + no-drift. |
| testdata/apply/add_column_on_update_current_timestamp.yml | Pins ON UPDATE CURRENT_TIMESTAMP column round-trip + no-drift. |
| testdata/apply/add_column_default_current_timestamp_only.yml | Pins DEFAULT CURRENT_TIMESTAMP (no ON UPDATE) round-trip + no-drift. |
| testdata/apply/add_column_and_dependent_index.yml | Pins statement ordering: ADD COLUMN before dependent CREATE INDEX + no-drift. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #58 +/- ##
=======================================
Coverage 71.16% 71.16%
=======================================
Files 28 28
Lines 3062 3062
=======================================
Hits 2179 2179
Misses 647 647
Partials 236 236 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixture header for `add_column_with_alter_hints_instant.yml` claimed coverage of `--alter-algorithm=INSTANT` AND `--alter-lock=NONE`, but only `alter_algorithm: INSTANT` is set on the fixture (and the expected `applied` DDL only emits `ALGORITHM=INSTANT`). Updated the header to drop the `--alter-lock=NONE` claim and added a note about why the combination isn't pinned: MySQL rejects `ALGORITHM=INSTANT` with any explicit `LOCK=…` (Error 1221 "Incorrect usage of ALGORITHM=INSTANT and LOCK=…"), so combining them in this fixture would be untestable. Pointed at the existing plan fixture `testdata/plan/alter_hints.yml` and the partition-op apply fixtures as the lock-splice coverage instead. No behaviour / test change — comment-only.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Fix the IS column reference in the CHECK NOT ENFORCED TODO entry — information_schema.CHECK_CONSTRAINTS does not carry ENFORCED; the flag lives on TABLE_CONSTRAINTS, which catalog/tables.go already joins to CHECK_CONSTRAINTS for exactly this reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spec audit pass — survey AGENTS.md "In scope" against the
existing fixture set, identify shapes that are documented as
supported but not pinned end-to-end, and add minimal apply +
verify_no_drift fixtures for each. All new fixtures pass on
the current main with no behaviour change.
24 apply fixtures (all `verify_no_drift: true` — strongest
form: pins generated SQL string, MySQL accepting it, AND
catalog round-tripping back without spurious diff):
Index shapes:
- add_index_desc.yml — `KEY (col DESC)` round-trip.
- add_index_invisible.yml — `KEY ... INVISIBLE` round-trip.
- add_index_with_prefix_length.yml — `KEY (col(N))` on
VARCHAR.
- add_index_on_blob_with_prefix_length.yml — same on BLOB
(where MySQL *requires* the prefix length).
- add_multi_column_unique_index.yml — column ordering
preserved through `STATISTICS.SEQ_IN_INDEX`.
- add_fulltext_index.yml — `CREATE FULLTEXT INDEX` shape
(one of the 4 CREATE INDEX variants `appendAlterHints`
handles).
- modify_index_columns.yml — index column-list change must
DROP+CREATE in that order.
Column shapes:
- add_column_on_update_current_timestamp.yml — `ON UPDATE
CURRENT_TIMESTAMP`.
- add_column_default_current_timestamp_only.yml — `DEFAULT
CURRENT_TIMESTAMP` without ON UPDATE.
- add_column_with_comment.yml — column-level COMMENT.
- add_generated_column_stored.yml — `GENERATED ALWAYS AS
(...) STORED`.
- add_enum_column.yml — basic ENUM column round-trip
(element-list diff is a known TODO; this only pins the
CREATE shape).
- add_json_column.yml — JSON column round-trip.
- add_decimal_column.yml — `DECIMAL(M, D)` precision/scale.
Foreign key shapes:
- add_fk_on_delete_cascade.yml — explicit ON DELETE
CASCADE.
- add_fk_on_update_cascade.yml — explicit ON UPDATE
CASCADE.
- add_self_referential_fk.yml — same-table FK
(employee.manager_id → employee.id).
- add_multi_column_fk.yml — composite FK column list
ordering through `KEY_COLUMN_USAGE.ORDINAL_POSITION`.
- cross_table_fk_ripple_on_column_rename.yml — parent-side
column rename re-writes child-side FK RefCols
(`rewriteCrossTableFKRefCols`) so the child FK doesn't
diff as DROP+ADD.
Constraint shapes:
- change_primary_key.yml — single-column → composite PK
(DROP PRIMARY KEY then ADD PRIMARY KEY ordering).
View shapes:
- modify_view_body.yml — view body change emits
`CREATE OR REPLACE VIEW` and converges.
- create_view_with_column_aliases.yml — view's explicit
column-alias list `(uid, uname)`.
Combined operations:
- add_column_and_dependent_index.yml — ADD COLUMN + CREATE
INDEX in same plan; CREATE INDEX must come after ADD
COLUMN.
- add_column_with_alter_hints_instant.yml — `--alter-
algorithm=INSTANT` on a column-level ALTER (trailing-
comma splice path, complement to the partition-op
leading-position splice path).
2 plan fixtures (filter coverage):
- exclude_filter.yml — `--exclude=<table>` companion to
the existing `include_filter.yml` (only-exact-name case
was pinned, the exclude direction wasn't).
- include_filter_glob.yml — `--include=tmp_*` glob
pattern (existing `include_filter.yml` only used an
exact name).
Two probes during the audit surfaced known untracked spec
gaps where the round-trip doesn't actually converge today
(CHECK constraint `NOT ENFORCED` and `TIMESTAMP NULL
DEFAULT NULL`). Per the "no spec changes" scope of this
branch I removed those fixtures; they're worth a separate
follow-up to either fix the diff layer or document the
limitation.
Likewise table-level `COMMENT='…'` changes aren't tracked
by the diff today — fixture removed, would need a separate
spec discussion.
`make lint` clean, full `go test -p 1 ./...` green, all 26
new fixtures pass.
PR #58's spec audit found three shapes that look like they should round-trip end-to-end but don't (probed and confirmed). Fixture additions were withheld from PR #58 since pinning broken behaviour without a fix or documented limitation violates the no-spec-change scope of that branch. Land them as TODO entries so they're not lost. All three live under "Medium — silent diffs / fidelity gaps" alongside the existing ENUM/SET and FK-to-other-DB items (same flavour: catalog has the data, but the model / diff layer drops or mishandles it): - CHECK constraint `NOT ENFORCED` — drift loop after first apply. - TIMESTAMP NULL DEFAULT NULL — drift loop emitting `MODIFY COLUMN ts timestamp DEFAULT null` repeatedly. - Table-level `COMMENT='…'` — silent (no diff at all). Each entry documents what the diff layer would need to learn (or, alternatively, where to document the limitation in CAVEATS.md) so a future contributor can pick one up without re-doing the probe.
Fixture header for `add_column_with_alter_hints_instant.yml` claimed coverage of `--alter-algorithm=INSTANT` AND `--alter-lock=NONE`, but only `alter_algorithm: INSTANT` is set on the fixture (and the expected `applied` DDL only emits `ALGORITHM=INSTANT`). Updated the header to drop the `--alter-lock=NONE` claim and added a note about why the combination isn't pinned: MySQL rejects `ALGORITHM=INSTANT` with any explicit `LOCK=…` (Error 1221 "Incorrect usage of ALGORITHM=INSTANT and LOCK=…"), so combining them in this fixture would be untestable. Pointed at the existing plan fixture `testdata/plan/alter_hints.yml` and the partition-op apply fixtures as the lock-splice coverage instead. No behaviour / test change — comment-only.
Fix the IS column reference in the CHECK NOT ENFORCED TODO entry — information_schema.CHECK_CONSTRAINTS does not carry ENFORCED; the flag lives on TABLE_CONSTRAINTS, which catalog/tables.go already joins to CHECK_CONSTRAINTS for exactly this reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9576788 to
37bc702
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Grammar: "apply once" → "applies once". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- TODO: rewrite the CHECK NOT ENFORCED entry to point at the real gap (catalog/tables.go hard-codes Enforced=true) — model and diff layers already plumb the flag. - TODO: rewrite the table-level COMMENT entry to point at the diff layer (Engine/Comment intentionally not diffed) — model and catalog already plumb the field. - testdata/apply/add_json_column.yml: COLUMN_TYPE, not DATA_TYPE, matches what catalog/tables.go loadColumns actually selects. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Spec audit pass — survey AGENTS.md "In scope" against the existing fixture set, identify shapes that are documented as supported but not pinned end-to-end, and add minimal apply +
verify_no_driftfixtures for each.No behaviour change. All new fixtures pin the current output of
make testso they will fail if a future commit accidentally regresses any of these shapes — that's the point.26 fixtures total (24 apply + 2 plan), +733 lines.
Coverage delta
Index shapes (7)
add_index_desc.yml—KEY (col DESC)round-trip.add_index_invisible.yml—KEY ... INVISIBLEround-trip.add_index_with_prefix_length.yml—KEY (col(N))on VARCHAR.add_index_on_blob_with_prefix_length.yml— same on BLOB (where MySQL requires the prefix length).add_multi_column_unique_index.yml— column ordering preserved throughSTATISTICS.SEQ_IN_INDEX.add_fulltext_index.yml—CREATE FULLTEXT INDEXshape (one of the 4 CREATE INDEX variantsappendAlterHintshandles).modify_index_columns.yml— index column-list change must DROP+CREATE in that order.Column shapes (7)
add_column_on_update_current_timestamp.yml—ON UPDATE CURRENT_TIMESTAMP.add_column_default_current_timestamp_only.yml—DEFAULT CURRENT_TIMESTAMPwithout ON UPDATE.add_column_with_comment.yml— column-level COMMENT.add_generated_column_stored.yml—GENERATED ALWAYS AS (...) STORED.add_enum_column.yml— basic ENUM column round-trip (element-list diff is a known TODO; this only pins the CREATE shape).add_json_column.yml— JSON column round-trip.add_decimal_column.yml—DECIMAL(M, D)precision/scale.Foreign key shapes (5)
add_fk_on_delete_cascade.yml— explicitON DELETE CASCADE.add_fk_on_update_cascade.yml— explicitON UPDATE CASCADE.add_self_referential_fk.yml— same-table FK (employees.manager_id → employees.id).add_multi_column_fk.yml— composite FK column list ordering throughKEY_COLUMN_USAGE.ORDINAL_POSITION.cross_table_fk_ripple_on_column_rename.yml— parent-side column rename rewrites child-side FK RefCols (rewriteCrossTableFKRefCols) so the child FK doesn't diff as DROP+ADD.Constraint / view shapes (4)
change_primary_key.yml— single-column → composite PK (DROP PRIMARY KEY then ADD PRIMARY KEY ordering).modify_view_body.yml— view body change emitsCREATE OR REPLACE VIEWand converges.create_view_with_column_aliases.yml— view's explicit column-alias list(uid, uname).add_column_and_dependent_index.yml— ADD COLUMN + CREATE INDEX in same plan; CREATE INDEX must come after ADD COLUMN.CLI / filter (3)
add_column_with_alter_hints_instant.yml—--alter-algorithm=INSTANTon a column-level ALTER (trailing-comma splice path; complement to the existing partition-op leading-position splice fixtures).exclude_filter.yml—--exclude=<table>companion to the existinginclude_filter.yml(only-exact-name include was pinned, the exclude direction wasn't).include_filter_glob.yml—--include=tmp_*glob pattern (existinginclude_filter.ymlonly used an exact name).Untracked spec gaps surfaced during the audit
Three probes during the audit found shapes where the round-trip doesn't actually converge today. Per the no-spec-change scope of this branch I removed those fixtures rather than pinning broken behaviour. Listing them here so we can decide separately whether to fix the diff layer or document the limitation:
NOT ENFORCED— myschema doesn't preserve theNOT ENFORCEDkeyword.verify_no_driftfails because re-plan emitsDROP CHECK chk + ADD CONSTRAINT chk CHECK (...)repeatedly.TIMESTAMP NULL DEFAULT NULL— same drift loop; re-plan repeatedly emitsMODIFY COLUMN ts timestamp DEFAULT null.COMMENT='…'changes — not detected as a diff at all (silent; the table-level COMMENT difference is invisible to the diff layer).Test plan
make lintmake test(8.0)make test-scenario