Conversation
Add comprehensive test skip mechanism to handle non-consequential view formatting differences in PostgreSQL 14-15 vs 16+. Root cause: pg_get_viewdef() in PG 14-15 returns table-qualified column names (e.g., employees.id) while PG 16+ returns simplified names (e.g., id). This causes test comparison failures but does not impact correctness. Changes: - Add GetMajorVersion() helper in testutil/postgres.go - Create skip list in testutil/skip_list.go with 13 test patterns - Add version detection and skip logic to all test files: * cmd/migrate_integration_test.go (TestPlanAndApply) * cmd/dump/dump_integration_test.go (dump integration tests) * cmd/include_integration_test.go (TestIncludeIntegration) * internal/diff/diff_test.go (TestDiffFromFiles) Skipped tests on PG 14-15: - View tests (create_view/*) - Materialized view tests (create_materialized_view/*) - Online materialized view index tests - Comment tests involving views - Migration tests v4 and v5 - Dump integration tests (Employee, Issue82ViewLogicExpr) - Include integration test All tests continue to run normally on PG 16-17. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the approach to handling PostgreSQL version differences in view definitions by moving from normalization-based handling to test skipping. Instead of attempting to normalize view definition differences between PostgreSQL 14-15 and 16+, tests that are affected by pg_get_viewdef() formatting differences are now skipped on PostgreSQL 14-15.
Key Changes:
- Removed complex view definition normalization logic from IR processing
- Added a skip list mechanism to skip known incompatible tests on PostgreSQL 14-15
- Added version detection functionality to enable test skipping based on PostgreSQL major version
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| testutil/skip_list.go | New file implementing test skip list logic for version-specific test exclusions |
| testutil/postgres.go | Added GetMajorVersion function to detect PostgreSQL major version from database connection |
| ir/normalize.go | Removed PostgreSQL 14-15 specific view definition normalization logic |
| internal/diff/diff_test.go | Added version detection and test skipping logic to file-based diff tests |
| cmd/migrate_integration_test.go | Added version detection and test skipping to migration tests |
| cmd/include_integration_test.go | Added version detection and test skipping to include integration test |
| cmd/dump/dump_integration_test.go | Added version detection and test skipping to dump command tests, threaded testName through helper functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Improvements: - Remove boolean return value from ShouldSkipTest() - function now only calls t.Skipf() - Add missing test "create_view/add_view_join" to skip lists - Simplify matching logic to exact match only (no prefix matching) - Update all call sites to remove unnecessary if checks This eliminates potential false positives from prefix matching and makes the behavior more predictable and maintainable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
tianzhou
changed the title
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/diff/diff_test.go
Outdated
|
|
||
| // extractTestName extracts the test name from a file path | ||
| // Converts "../../testdata/diff/create_view/add_view/old.sql" to "create_view_add_view" | ||
| func extractTestName(filePath string) string { |
There was a problem hiding this comment.
The extractTestName function is redundant. Lines 118-119 in TestDiffFromFiles already extract the test name using the same logic: relPath, _ := filepath.Rel(testdataDir, path) and testName := strings.ReplaceAll(relPath, string(os.PathSeparator), \"_\"). The test name extracted there (line 119) is used as the subtest name (line 130) and should be passed to runFileBasedDiffTest instead of re-extracting it from oldFile.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changes: - Create single skipListPG14_15 variable shared by both versions - Remove duplicate entries between version 14 and 15 - Both versions now reference the same list via map This eliminates ~20 lines of duplication and makes maintenance easier. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add comprehensive test skip mechanism to handle non-consequential view
formatting differences in PostgreSQL 14-15 vs 16+.
Root cause: pg_get_viewdef() in PG 14-15 returns table-qualified column
names (e.g., employees.id) while PG 16+ returns simplified names (e.g., id).
This causes test comparison failures but does not impact correctness.
Changes:
Skipped tests on PG 14-15:
All tests continue to run normally on PG 16-17.
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com