Expand test coverage across catalog / diff / model#20
Merged
Conversation
Pin the v1 spec by exercising code paths that previously had thin or no
coverage. Each addition targets a real catalog / diff / model branch,
not "coverage for coverage's sake".
catalog/catalog_test.go (50% → 77%):
- TestForeignKeyRoundTrip: column list, ref db/table/cols, ON DELETE
CASCADE / ON UPDATE SET NULL round trip
- TestForeignKeyRestrictAndNoActionNormalize: confirms the catalog
folds RESTRICT and NO ACTION into "" so the diff side compares
cleanly with the parser (regression for the recent referenceAction
bug)
- TestCheckConstraintRoundTrip: pins the JOIN with TABLE_CONSTRAINTS
and the `CHECK (<expr>)` definition shape that constraintInlineSQL
relies on (regression for the duplicate-CHECK bug surfaced in
Copilot review round 2)
- TestGeneratedColumnRoundTrip: STORED vs VIRTUAL — covers the
Stored=true/false branches that previously masked an inverted-flag
bug
- TestIndexCatalogQuirks: prefix length, multi-column, FULLTEXT
KeyType mapping
- TestColumnDefaultNormalisation: bareword ENUM default wrapped,
numeric default left bare, CURRENT_TIMESTAMP expression left bare
- TestViewsRoundTrip: VIEW_DEFINITION + DEFINER + SECURITY surfaced
- TestTableMetadata: ENGINE / table COMMENT round trip
diff/tables_test.go (56% → 69%):
- Removed the dead `parseOne` helper (unused since the YAML harness
landed) and added `parsePair` / `allowList` helpers that cut every
test body to ~5 lines.
- Add: TestDiffDropColumnAllowed, TestDiffModifyColumnType,
TestDiffModifyColumnDefault, TestDiffDropTableAllowed,
TestDiffAddCheckConstraint, TestDiffDropCheckConstraint{Allowed,
Suppressed}, TestDiffAddIndex, TestDiffDropIndexSuppressed,
TestDiffAddForeignKey, TestDiffDropForeignKeySuppressed,
TestDiffDropTableWithFK (table drop emits FK drop first; both
policies), TestDropPolicyWildcard ("all" allows everything),
TestDropPolicyNilFallsBackToAllowAll (nil → AllowAll, not
no-allow), TestDiffNoChanges (sanity).
model/table_test.go (52% → 96%, +43 percentage points):
- TestTableSQLBasic, TestTableSQLWithCheckConstraint (regression for
CHECK CHECK), TestTableSQLWithEngineCharsetCollationComment,
TestTableIdxAndFkSQL, TestTableToSQLCombined,
TestTablesToSQLOrderingAndSeparators
- TestColumnDefSQLAttributes covers every attribute the renderer
knows about in one go; TestColumnDefSQLAutoIncrementAndQuotedComment
pins the AUTO_INCREMENT branch and embedded-quote escape;
TestColumnDefSQLVirtualGenerated covers the Stored=false branch.
All other tests / scenarios / sample-DB round-trips still pass.
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 #20 +/- ##
===========================================
+ Coverage 55.20% 69.79% +14.58%
===========================================
Files 25 25
Lines 1652 1652
===========================================
+ Hits 912 1153 +241
+ Misses 621 381 -240
+ Partials 119 118 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR expands automated test coverage for the catalog, diff, and model packages to better pin the v1 schema spec and exercise previously uncovered branches (including several past regression surfaces).
Changes:
- Added comprehensive renderer tests for
model.Tableandmodel.ColumnDefSQL(newmodel/table_test.go). - Refactored and significantly expanded
difftable-diff tests, including drop-policy behavior and FK/table drop ordering. - Expanded
catalogintegration tests to validate MySQL information_schema round-trips for FKs, CHECK constraints, generated columns, index quirks, defaults, views, and table metadata.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| model/table_test.go | New test suite covering CREATE TABLE rendering + index/FK emitters + column-definition rendering branches. |
| diff/tables_test.go | Adds helper utilities and many new tests for ALTER/CREATE/DROP output and allow-drop suppression semantics. |
| catalog/catalog_test.go | Adds MySQL-backed round-trip tests for constraints, FKs, generated columns, indexes, defaults, views, and metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Six places in catalog_test.go did `obj, _ := tables.GetOk(...)` and then dereferenced obj. If a key changed (e.g. from a catalog refactor) the tests would crash with a nil-pointer panic instead of a clear "X table should be loaded" failure. Switch every catalog GetOk to the `obj, ok := …; require.True(t, ok)` form, including the nested Columns/Indexes/Constraints/ForeignKeys lookups. No behaviour change — failures are just actionable now. Caught by Copilot review on PR #20. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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.
winebarrel
added a commit
that referenced
this pull request
May 2, 2026
Copilot review #20: backtickedNameAfterPrefix matched its multi-word prefixes (e.g. "UNIQUE KEY", "FULLTEXT INDEX") with strings.HasPrefix, which only succeeds on exactly one space between the keywords. SQL formatted with multiple spaces or a tab between the two keywords (e.g. `UNIQUE KEY \`also weird\` (id)` or `FULLTEXT\tINDEX …`) silently fell through to the strings.Fields-based path, which can't extract a backticked index name containing whitespace and surfaces as "target index not found". New consumeKeywordSequence helper walks the prefix keyword-by-keyword against the line, allowing any run of `[ \t]+` between successive keywords (and requiring at least one whitespace character after the final keyword so `KEYWORD` doesn't accidentally match `KEY`). stripUntilAfterBacktickedName goes through the same helper for consistency, so the CONSTRAINT path picks up the same flexibility. Tests - TestExtractInlineRenamesBacktickedIndexMultiWhitespacePrefix covers `UNIQUE KEY` (multi-space) and `FULLTEXT\tINDEX` (tab). 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
Pin the v1 spec by exercising code paths that previously had thin or no coverage. Each addition targets a real catalog / diff / model branch — not coverage for coverage's sake.
Coverage delta
myschema(root)catalogdiffmodelparsercatalog/catalog_test.go
TestForeignKeyRoundTrip— column list, ref db/table/cols, ON DELETE CASCADE / ON UPDATE SET NULLTestForeignKeyRestrictAndNoActionNormalize— confirms RESTRICT / NO ACTION fold to""(regression for the recentreferenceActionbug)TestCheckConstraintRoundTrip— pins the JOIN with TABLE_CONSTRAINTS and theCHECK (<expr>)definition shape (regression for the duplicate-CHECK bug from review round 2)TestGeneratedColumnRoundTrip— STORED vs VIRTUAL (covers the inverted-flag bug surface)TestIndexCatalogQuirks— prefix length, multi-column, FULLTEXT KeyTypeTestColumnDefaultNormalisation— bareword ENUM wrapped, numeric bare, CURRENT_TIMESTAMP bareTestViewsRoundTrip— VIEW_DEFINITION + DEFINER + SECURITYTestTableMetadata— ENGINE / table COMMENTdiff/tables_test.go
parseOnehelper; addedparsePair/allowListhelpers (every test body ~5 lines now).TestDiffDropTableWithFK(table drop emits FK drop first),TestDropPolicyWildcard("all" allows everything),TestDropPolicyNilFallsBackToAllowAll(nil → AllowAll),TestDiffNoChanges.model/table_test.go (new file)
TestTableSQLBasic,TestTableSQLWithCheckConstraint(regression for CHECK CHECK),TestTableSQLWithEngineCharsetCollationComment,TestTableIdxAndFkSQL,TestTableToSQLCombined,TestTablesToSQLOrderingAndSeparatorsTestColumnDefSQLAttributescovers every attribute in one go;TestColumnDefSQLAutoIncrementAndQuotedCommentpins AUTO_INCREMENT + embedded-quote escape;TestColumnDefSQLVirtualGeneratedcovers the Stored=false branch.Test plan
make test(parser + diff + catalog + model + YAML harness)make test-scenariogolangci-lint🤖 Generated with Claude Code