-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: [#540] Add test cases #845
Conversation
WalkthroughThis pull request integrates MySQL support across the project’s testing framework and schema management. It updates module dependencies in Changes
Sequence Diagram(s)sequenceDiagram
participant TS as TestSuite
participant MTQ as mysqlTestQuery
participant TQ as TestQuery Instance
participant NT as newTestTables
participant BP as Blueprint
participant G as Grammar
TS->>MTQ: Call mysqlTestQuery(prefix, singular)
MTQ->>TS: Return TestQuery instance
TS->>TQ: Invoke CreateTable(testTables...)
TQ->>NT: Call newTestTables(driver, r.Driver().Grammar())
NT->>BP: Initialize blueprint for table creation
BP->>G: Use grammar to generate SQL
G-->>BP: Return SQL command
BP-->>NT: Return SQL command
NT-->>TQ: Return configured test tables
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #845 +/- ##
=======================================
Coverage 67.16% 67.16%
=======================================
Files 150 150
Lines 10442 10442
=======================================
Hits 7013 7013
Misses 3055 3055
Partials 374 374 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/table.go (1)
54-62
: Consider refactoring repeated blueprint logic.
All table creation methods follow a similar pattern (ID, timestamps, soft deletes). Extracting a shared helper to apply these repeated fields can improve maintainability.func (r *testTables) peoples() string { blueprint := schema.NewBlueprint(nil, "", "peoples") blueprint.Create() - blueprint.BigIncrements("id") - blueprint.Timestamps() - blueprint.SoftDeletes() + r.addDefaultFields(blueprint) blueprint.String("body") return blueprint.ToSql(r.grammar)[0] } +func (r *testTables) addDefaultFields(blueprint *schema.Blueprint) { + blueprint.BigIncrements("id") + blueprint.Timestamps() + blueprint.SoftDeletes() +}tests/query_test.go (1)
13-13
: Consider using a consistent import alias for postgres.The postgres package is imported without an alias. For consistency with other database drivers and to make the code more maintainable, consider using an alias.
-import "github.com/goravel/postgres" +import postgres "github.com/goravel/postgres"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/go.sum
is excluded by!**/*.sum
📒 Files selected for processing (9)
tests/go.mod
(3 hunks)tests/migrator_test.go
(1 hunks)tests/orm_test.go
(1 hunks)tests/query.go
(1 hunks)tests/query_test.go
(6 hunks)tests/repository_test.go
(1 hunks)tests/schema_test.go
(9 hunks)tests/table.go
(3 hunks)tests/utils.go
(5 hunks)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: test / windows (1.23)
🔇 Additional comments (26)
tests/table.go (15)
3-6
: No issues with import statements.
The newly introduced imports are consistent with the usage for blueprint-based table creation.
27-28
: No issues with the new 'grammar' field.
This field is properly integrated and sets up the blueprint functionality for driver-specific SQL generation.
31-32
: Constructor changes look good.
Accepting the grammar as a parameter cleanly injects driver-specific behavior.
65-73
: Same suggestion applies here regarding duplicated blueprint logic.
76-84
: Same suggestion applies here regarding duplicated blueprint logic.
87-97
: Same suggestion applies here regarding duplicated blueprint logic.
100-110
: Same suggestion applies here regarding duplicated blueprint logic.
113-123
: Same suggestion applies here regarding duplicated blueprint logic.
126-135
: Same suggestion applies here regarding duplicated blueprint logic.
138-147
: Same suggestion applies here regarding duplicated blueprint logic.
150-159
: Same suggestion applies here regarding duplicated blueprint logic.
162-172
: Same suggestion applies here regarding duplicated blueprint logic.
175-185
: Same suggestion applies here regarding duplicated blueprint logic.
188-197
: Same suggestion applies here regarding duplicated blueprint logic.
200-207
: Same suggestion applies here regarding duplicated blueprint logic.tests/query.go (1)
40-40
: Passing the driver's grammar aligns table creation with the correct SQL dialect.
This change ensures that generated statements remain consistent across different database drivers.tests/repository_test.go (1)
23-27
: Well-structured addition of 'mysqlTestQuery'.
Extending the driverToTestQuery map with MySQL ensures MySQL test coverage remains on par with PostgreSQL.tests/orm_test.go (1)
34-38
: LGTM! Good test setup for MySQL.The MySQL test setup follows the same pattern as PostgreSQL, ensuring consistent test environment and proper isolation between database drivers.
tests/migrator_test.go (1)
25-30
: LGTM! Good test coverage for MySQL migrations.The MySQL migrator test setup properly mirrors the PostgreSQL configuration, enabling comprehensive testing of migrations across both database drivers.
tests/utils.go (1)
79-128
: LGTM! Well-structured MySQL test query setup.The MySQL test query setup is well implemented with:
- Proper configuration of MySQL-specific settings
- Consistent error handling
- Good use of mocks for configuration
tests/schema_test.go (2)
227-232
: LGTM! Good handling of MySQL timestamp behavior differences.The test correctly handles the difference in timestamp behavior between MySQL and other databases, where MySQL updates the timestamp on update while others don't.
1631-1664
: LGTM! Comprehensive MySQL enum testing.The test suite thoroughly validates MySQL enum behavior:
- Tests constraint enforcement
- Verifies error handling for invalid values
- Confirms successful insertion of valid values
tests/query_test.go (2)
34-36
: LGTM! MySQL test query setup added.The MySQL test query setup follows the same pattern as PostgreSQL, maintaining consistency in the test suite.
2090-2118
: Verify SQLite exclusion for LockForUpdate test.The test correctly excludes SQLite from LockForUpdate tests since SQLite doesn't support row-level locking. However, let's verify if there are any other database-specific features that need similar treatment.
✅ Verification successful
SQLite exclusion and additional driver-specific logic verified.
The test intests/query_test.go
correctly excludes SQLite for the LockForUpdate operations. Our search also revealed that any additional database-specific features (e.g., in schema and index tests withintests/schema_test.go
) already include the appropriate guards, so no further exclusions for SQLite are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for other database-specific features that might need driver checks. # Look for potential database-specific features rg -A 5 'if driver [!=]=' # Look for SQLite-specific exclusions rg -A 5 'sqlite'Length of output: 16604
tests/go.mod (2)
28-28
: Verify MySQL driver versions.The indirect dependencies for MySQL support have been added:
- github.com/go-sql-driver/mysql v1.8.1
- gorm.io/driver/mysql v1.5.7
Let's verify these are the latest stable versions and check for any security advisories.
Also applies to: 58-58
✅ Verification successful
MySQL driver versions verified and no security vulnerabilities found.
- The MySQL driver for
github.com/go-sql-driver/mysql
is set to v1.8.1, which is the latest version available.- The GORM MySQL driver
gorm.io/driver/mysql
is at v1.5.7, matching the most recent stable release.- No security advisories were reported for
github.com/go-sql-driver/mysql
in the reviewed query.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check MySQL driver versions and security advisories. # Check for latest versions curl -s https://proxy.golang.org/github.com/go-sql-driver/mysql/@v/list curl -s https://proxy.golang.org/gorm.io/driver/mysql/@v/list # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/go-sql-driver/mysql") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 969
9-11
: Verify compatibility of new framework and MySQL dependencies.The addition of MySQL support introduces new dependencies. Let's verify their compatibility:
- Framework update to v1.15.2-0.20250202111117-cbb806d1e8de
- New MySQL dependency v0.0.0-20250202150522-4e41e0908557
✅ Verification successful
Compatibility Verified: Framework and MySQL Dependencies
The GitHub API queries returnednull
for both the framework and MySQL pseudo-versions, which is expected since pseudo-versions (with commit SHA and timestamps) do not have associated release records on GitHub. There are no indications from the queried metadata of any compatibility issues between the new dependencies. For additional certainty, please ensure the integration tests and CI pipelines are passing.
- Framework version
v1.15.2-0.20250202111117-cbb806d1e8de
is a pseudo-version without a formal release.- MySQL dependency
v0.0.0-20250202150522-4e41e0908557
is also a pseudo-version, showing no release metadata issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for compatibility issues between dependencies. # Check if there are any reported issues with these versions gh api graphql -f query=' { repository(owner: "goravel", name: "framework") { release(tagName: "v1.15.2-0.20250202111117-cbb806d1e8de") { description isPrerelease } } }' # Check MySQL driver compatibility gh api graphql -f query=' { repository(owner: "goravel", name: "mysql") { release(tagName: "v0.0.0-20250202150522-4e41e0908557") { description isPrerelease } } }'Length of output: 465
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
database/schema/schema.go (1)
35-38
: LGTM! Consider adding a comment explaining the fallback behavior.The fallback mechanism for schema initialization is a good addition that improves robustness, particularly for MySQL support. The implementation is clean and backwards compatible.
Consider adding a comment explaining why we fall back to
Database
whenSchema
is empty:schema := driver.Config().Schema + // Fall back to Database when Schema is empty, which is typical for MySQL if schema == "" { schema = driver.Config().Database }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sum
is excluded by!**/*.sum
📒 Files selected for processing (1)
database/schema/schema.go
(1 hunks)
⏰ Context from checks skipped due to timeout of 300000ms (2)
- GitHub Check: test / windows (1.23)
- GitHub Check: test / windows (1.22)
📑 Description
goravel/goravel#540
Summary by CodeRabbit
New Features
Tests
Chores
✅ Checks