Move config/validation tests to migration_test.go, delete runner_test.go (#716 subtasks 7+9)#731
Merged
Merged
Conversation
….go (block#716 subtask 7) Move 5 remaining tests from runner_test.go into migration_test.go: - TestBadOptions - TestBadAlter - TestDefaultPort - TestPasswordMasking - TestDSN These all test Migration struct validation, DSN construction, and error handling — they belong with the other Migration config tests. Conversions: - TestBadAlter: table setup converted to NewTestTable, runner calls to NewTestRunner - assert converted to require (fail-fast) - Table-driven tests use 'tc' instead of 'tt' (avoids shadowing testutils.TestTable) runner_test.go: DELETED (323 lines -> 0) migration_test.go: 1109 -> 1269 lines (+160) Fixes block#724 Part of block#716 (test reorganization)
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the migration test suite reorganization by consolidating configuration/validation tests under migration_test.go and removing the now-redundant runner_test.go, while also relocating several explanatory comments back to the tests they describe.
Changes:
- Moved 5 config/validation tests from
runner_test.gointomigration_test.go(with helper-based setup andrequireassertions). - Deleted
pkg/migration/runner_test.goafter the move. - Restored/moved stray explanatory comments to
e2e_test.goanddatatype_test.go.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/migration/runner_test.go | Deleted after migrating remaining tests out. |
| pkg/migration/migration_test.go | Added the migrated config/validation tests and necessary import. |
| pkg/migration/e2e_test.go | Moved Issue #277 context comment to the relevant test. |
| pkg/migration/datatype_test.go | Restored expanded explanatory comments for chunker/ENUM reorder tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aparajon
approved these changes
Apr 30, 2026
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
Move the 5 remaining tests from
runner_test.gointomigration_test.goand deleterunner_test.go.These tests all validate the
Migrationstruct's configuration, DSN construction, and error handling — they belong with the other Migration config tests inmigration_test.go.Tests moved:
TestBadOptions— validates required fieldsTestBadAlter— invalid ALTER statement scenarios (parse errors, MySQL rejections)TestDefaultPort— default port appended when not specifiedTestPasswordMasking— password masking in DSN stringsTestDSN— DSN round-trip with special charactersAlso restored stray comments to correct files:
TestDataFromBadSqlModecomment →e2e_test.goTestChangeDatatypeLossyNoAutoInccomment →datatype_test.goTestEnumReorderfull explanation →datatype_test.goConversions:
TestBadAlter: table setup →NewTestTable, runner calls →NewTestRunnerassert→require(fail-fast)tcinstead oftt(avoids shadowingtestutils.TestTable)Impact:
runner_test.go: DELETED (323 lines → 0) 🎉migration_test.go: 1,109 → 1,269 lines (+160)Fixes #724
Fixes #726
Part of #716 (test reorganization)