chore: update tests mod #846
Conversation
|
Warning Rate limit exceeded@hwbrzzl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughThis pull request adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Suite
participant M as Migrator
participant D as SQL Server Driver
participant DB as Database
T ->> M: Initialize SQL Server test query
M ->> D: Set up connection & migration schema
D -->> M: Acknowledge setup
T ->> M: Call TestMigrationWithSqlserverSchema.Up()
M ->> DB: Execute "Up" migration (create table)
DB -->> M: Return success
T ->> M: Validate migration results
T ->> M: Call TestMigrationWithSqlserverSchema.Down()
M ->> DB: Execute "Down" migration (drop table)
DB -->> M: Return success
T ->> M: Teardown test resources
Possibly related PRs
Suggested reviewers
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 #846 +/- ##
=======================================
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/schema_test.go (1)
2596-2621: Uncomment and update SQL Server schema test.The commented-out SQL Server schema test should be updated and enabled to ensure proper testing of the SQL Server implementation.
Apply this diff to update and enable the test:
-// func TestSqlserverSchema(t *testing.T) { -// if env.IsWindows() { -// t.Skip("Skip test that using Docker") -// } +func TestSqlserverSchema(t *testing.T) { + if env.IsWindows() { + t.Skip("Skip test that using Docker") + } -// schema := "goravel" -// table := "table" -// sqlserverDocker := docker.Sqlserver() -// require.NoError(t, sqlserverDocker.Ready()) + schema := "goravel" + table := "table" + sqlserverTestQuery := sqlserverTestQuery("", false) + sqlserverTestQuery.WithSchema(schema) + newSchema := newSchema(sqlserverTestQuery, map[string]*TestQuery{ + sqlserverTestQuery.Driver().Config().Connection: sqlserverTestQuery, + }) -// sqlserverQuery := gorm.NewTestQueryWithSchema(sqlserverDocker, schema) -// newSchema := newSchema(sqlserverQuery, map[database.Driver]*gorm.TestQuery{ -// database.DriverSqlserver: sqlserverQuery, -// }) -// assert.NoError(t, newSchema.Create(fmt.Sprintf("%s.%s", schema, table), func(table contractsschema.Blueprint) { -// table.String("name") -// })) -// tables, err := newSchema.GetTables() + assert.NoError(t, newSchema.Create(fmt.Sprintf("%s.%s", schema, table), func(table contractsschema.Blueprint) { + table.String("name") + })) + tables, err := newSchema.GetTables() -// assert.NoError(t, err) -// assert.Len(t, tables, 1) -// assert.Equal(t, "table", tables[0].Name) -// assert.Equal(t, schema, tables[0].Schema) -// assert.True(t, newSchema.HasTable(fmt.Sprintf("%s.%s", schema, table))) -// } + assert.NoError(t, err) + assert.Len(t, tables, 1) + assert.Equal(t, "table", tables[0].Name) + assert.Equal(t, schema, tables[0].Schema) + assert.True(t, newSchema.HasTable(fmt.Sprintf("%s.%s", schema, table))) +}
🧹 Nitpick comments (11)
tests/query_test.go (7)
10-10: Use a more descriptive import alias.Currently, the alias
databasedbmight be unclear. Consider renaming it to something more consistent such asdbdriverordbcoreto clarify its usage throughout the code.
24-25: Additional field inQueryTestSuite.The addition of
additionalQuerysuggests expanded test coverage or specialized queries. Ensure that it’s covered by relevant tests to avoid unused overhead.
52-53: Holding an additional Postgres query instance.Confirm that using a second Postgres query (
s.additionalQuery) does not conflict with the main suite’s Postgres query variables. Each test environment may need explicit teardown steps.
701-731: Adding a refresh connection test scenario in Delete flow.Similar to the create scenario, you’re verifying that toggling to “dummy” does not inadvertently reflect new data or preserve old data. The test approach is sound.
You might consider adding clarifying docstrings to highlight why the “dummy” switch is expected to skip fresh queries.
1797-1818: TestingFirst()with a refreshed connection.This segment ensures that switching the mock config to “dummy” excludes newly created records. The logic is correct, but consider grouping fresh-connection logic in a helper to avoid duplication across multiple tests.
2108-2108: Conditional skip for SQLite in LockForUpdate.SQLite does not support row-level locking in the same manner. This condition is reasonable. Ensure you’re skipping or providing an alternative test scenario for SQLite to maintain coverage consistency.
3718-3731: Postgres with custom schema.Creation of tables within a non-default schema is tested here. Looks good for broad coverage. Remember to tear down schema objects if context extends beyond ephemeral test DBs.
contracts/testing/testing.go (1)
49-50: Clarify the usage of theDriverfield inDatabaseConfig.The added
Driverfield is labeled with a TODO. Ensure you document how it’s intended to be used (e.g., specifying database driver type) and update relevant references so it’s consistently respected across the testing suite.Would you like help drafting a short usage comment or example that clarifies its role for other collaborators?
tests/repository_test.go (1)
35-41: Consider extending TearDownTest for other database drivers.Currently, only SQLite resources are cleaned up. Consider implementing cleanup for other database drivers that might need resource management.
func (s *RepositoryTestSuite) TearDownTest() { + // Clean up SQLite resources if s.driverToTestQuery[sqlite.Name] != nil { docker, err := s.driverToTestQuery[sqlite.Name].Driver().Docker() s.NoError(err) s.NoError(docker.Shutdown()) } + // Clean up SQL Server resources if needed + if s.driverToTestQuery["sqlserver"] != nil { + docker, err := s.driverToTestQuery["sqlserver"].Driver().Docker() + if err == nil && docker != nil { + s.NoError(docker.Shutdown()) + } + } }tests/migrator_test.go (1)
194-214: Consider consolidating migration implementations.The
TestMigrationWithSqlserverSchemais very similar toTestMigration. Consider using a single implementation with schema-aware table naming.type TestMigration struct { schema contractsschema.Schema + useSchemaPrefix bool } -func NewTestMigration(schema contractsschema.Schema) *TestMigration { +func NewTestMigration(schema contractsschema.Schema, useSchemaPrefix bool) *TestMigration { - return &TestMigration{schema: schema} + return &TestMigration{schema: schema, useSchemaPrefix: useSchemaPrefix} } func (r *TestMigration) Up() error { + tableName := "users" + if r.useSchemaPrefix { + tableName = "goravel." + tableName + } - return r.schema.Create("users", func(table contractsschema.Blueprint) { + return r.schema.Create(tableName, func(table contractsschema.Blueprint) { table.String("name") }) } func (r *TestMigration) Down() error { + tableName := "users" + if r.useSchemaPrefix { + tableName = "goravel." + tableName + } - return r.schema.DropIfExists("users") + return r.schema.DropIfExists(tableName) }tests/query.go (1)
96-131: Consider extracting common test query setup logic.The test query functions for different databases share similar setup patterns. Consider extracting the common logic to reduce code duplication.
Here's a suggested refactor:
+type testQueryConfig struct { + connection string + prefix string + singular bool + driver driver.Driver + newDocker func() docker.Docker +} + +func newTestQuery(config testQueryConfig) *TestQuery { + mockConfig := &mocksconfig.Config{} + docker := config.newDocker() + containerInstance, err := docker.NewContainer().Build() + if err != nil { + panic(err) + } + + mockConfig.EXPECT().Get(fmt.Sprintf("database.connections.%s.write", config.connection)).Return(nil) + mockConfig.EXPECT().Add(fmt.Sprintf("database.connections.%s.port", config.connection), containerInstance.Config().Port) + + if err := containerInstance.Ready(); err != nil { + panic(err) + } + + mockDatabaseConfig(mockConfig, database.Config{ + Driver: config.connection, + Host: containerInstance.Config().Host, + Port: containerInstance.Config().Port, + Username: containerInstance.Config().Username, + Password: containerInstance.Config().Password, + Database: containerInstance.Config().Database, + }, config.connection, config.prefix, config.singular) + + ctx := context.WithValue(context.Background(), testContextKey, "goravel") + testQuery, err := NewTestQuery(ctx, config.driver(mockConfig, utils.NewTestLog(), config.connection), mockConfig) + if err != nil { + panic(err) + } + + return testQuery +}Then use it in the driver-specific functions:
func postgresTestQuery(prefix string, singular bool) *TestQuery { - connection := postgres.Name - mockConfig := &mocksconfig.Config{} - image := postgres.NewDocker(postgres.NewConfig(mockConfig, connection), testDatabase, testUsername, testPassword) - container := docker.NewContainer(image) - containerInstance, err := container.Build() - if err != nil { - panic(err) - } - - mockConfig.EXPECT().Get(fmt.Sprintf("database.connections.%s.write", connection)).Return(nil) - mockConfig.EXPECT().Add(fmt.Sprintf("database.connections.%s.port", connection), containerInstance.Config().Port) - - if err := containerInstance.Ready(); err != nil { - panic(err) - } - - mockDatabaseConfig(mockConfig, database.Config{ - Driver: postgres.Name, - Host: containerInstance.Config().Host, - Port: containerInstance.Config().Port, - Username: containerInstance.Config().Username, - Password: containerInstance.Config().Password, - Database: containerInstance.Config().Database, - }, connection, prefix, singular) - - ctx := context.WithValue(context.Background(), testContextKey, "goravel") - driver := postgres.NewPostgres(mockConfig, utils.NewTestLog(), connection) - testQuery, err := NewTestQuery(ctx, driver, mockConfig) - if err != nil { - panic(err) - } - - return testQuery + return newTestQuery(testQueryConfig{ + connection: postgres.Name, + prefix: prefix, + singular: singular, + driver: postgres.NewPostgres, + newDocker: func() docker.Docker { + return postgres.NewDocker(postgres.NewConfig(mockConfig, connection), testDatabase, testUsername, testPassword) + }, + }) }Also applies to: 133-168, 170-205, 207-229
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
contracts/testing/testing.go(1 hunks)tests/go.mod(1 hunks)tests/migrator_test.go(4 hunks)tests/mock_config.go(1 hunks)tests/orm_test.go(3 hunks)tests/query.go(2 hunks)tests/query_test.go(13 hunks)tests/repository_test.go(2 hunks)tests/schema_test.go(20 hunks)tests/table.go(4 hunks)tests/utils.go(0 hunks)
💤 Files with no reviewable changes (1)
- tests/utils.go
✅ Files skipped from review due to trivial changes (1)
- tests/go.mod
⏰ Context from checks skipped due to timeout of 300000ms (2)
- GitHub Check: test / windows (1.23)
- GitHub Check: test / windows (1.22)
🔇 Additional comments (25)
tests/query_test.go (11)
14-14: Review driver references.Ensure proper usage and removal of leftover references to
mysqlif this driver is conditionally included or no longer needed in some tests.
16-18: Keep driver imports consistent.All driver imports (sqlite, sqlserver, and testify) appear to be newly added. Verify that these additions align with the test coverage expansion for multiple databases. Remove any extraneous imports if not used.
36-38: Validate creation of new Postgres test query.The creation and usage of
postgresTestQueryOnelook correct. Confirm that no side effects occur with multiple test queries referencing the same driver configuration concurrently.
43-46: Ensure consistent naming for SQL Server test query.The lines create and store a
sqlserverTestQuery. Confirm that the name aligns with other driver queries (e.g.,mysqlTestQuery) and that the test table creation is consistent across all drivers.
48-51: SQLite test queries added.Great approach to expand coverage. Double-check that the SQLite environment is properly cleaned up in the
TearDownSuiteand that concurrency issues do not arise with leftover data.
60-64: Conditional SQLite cleanup.The teardown logic is correct, but repeated calls to
Driver().Docker()might be fragile if multiple SQLite queries exist. Ensure concurrency is managed or that only one container is used for SQLite in these tests.
435-463: Adding a refresh connection test scenario in Create flow.This block verifies that switching to a “dummy” connection does not retrieve newly created rows in the same query. Ensure test data remains consistent across mock configurations when toggling connections. The test logic looks good overall.
2052-2079: TestingGet()with a refreshed connection.Same pattern of validating an alternate connection. The test reliably checks that newly created records aren’t found under the “dummy” config. Code appears correct.
3049-3049: Case block for SQL Server raw SQL generation.Verifying raw SQL output for SQL Server is helpful. Confirm that any parameter placeholders match the actual syntax for your environment.
3684-3716: Table prefix and singular form tests.You’re verifying that the prefix and singular configurations are respected. Great approach. Confirm no collisions occur with these custom table names, especially if you run all tests in parallel or have lengthy table names that might conflict.
3733-3745: SQL Server with custom schema.Similarly verifying a non-default schema for SQL Server is excellent. Confirm no leftover schema objects remain, especially if reusing containers. Otherwise, tests may break on reruns.
tests/mock_config.go (1)
1-63: Comprehensive mocking for multiple database drivers.This new file centralizes mock configurations for PostgreSQL, MySQL, SQL Server, and SQLite. Overall this is well-organized. A few considerations:
- Safety: Confirm that
mockConfig.EXPECT()usage safely handles concurrent test runs, especially in parallel.- Maintainability: If you add new drivers or new config parameters, keep this file in sync.
Otherwise, your approach for returning dedicated driver instances via
viacallbacks is solid.tests/repository_test.go (1)
25-33: LGTM! Test setup properly initializes all database drivers.The setup includes all necessary database drivers (Postgres, MySQL, SQL Server, SQLite) with consistent naming patterns.
tests/orm_test.go (2)
41-49: LGTM! Comprehensive test setup for all database drivers.The setup properly initializes SQL Server and SQLite test queries with appropriate table creation.
63-68: Consider extending TearDownSuite for other database drivers.Similar to the repository tests, consider implementing cleanup for other database drivers.
tests/table.go (2)
199-201: Verify schema name change impact.The schema name has been changed to "goravel.schemas". Ensure this is consistent with the database configuration.
✅ Verification successful
Schema name is applied consistently across the codebase.
- The schema name "goravel.schemas" is used in both
tests/table.go(while creating the blueprint) and intests/models.go(via theTableName()method).- This indicates the change is consistently reflected, ensuring compatibility with the intended database configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other schema references to ensure consistency rg "goravel\.schemas" -A 2 -B 2Length of output: 395
22-22: Verify the impact of renaming TestTableGoravelUser to TestTableUser.The table constant has been renamed. Ensure all references are updated.
✅ Verification successful
TestTableGoravelUser has been successfully renamed to TestTableUser
- In
tests/table.go, no references toTestTableGoravelUserremain.- The file correctly displays the updated identifier
TestTableUser(line 22) with no remnants of the previous name.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to TestTableGoravelUser rg "TestTableGoravelUser" -lLength of output: 28
Script:
#!/bin/bash # Verify that tests/table.go contains the new TestTableUser and does not contain TestTableGoravelUser echo "Searching for TestTableUser in tests/table.go:" rg "TestTableUser" -n tests/table.go echo "" echo "Searching for TestTableGoravelUser in tests/table.go:" rg "TestTableGoravelUser" -n tests/table.goLength of output: 415
tests/migrator_test.go (1)
153-170: LGTM! Well-structured SQL Server schema test.The test properly validates schema creation, table existence, and cleanup.
tests/query.go (3)
9-20: LGTM! Import statements are well organized.The imports are logically grouped and include all necessary database drivers.
64-66: LGTM! MockConfig method provides clean access to mocked configuration.The method provides a type-safe way to access the mocked configuration.
72-94: Consider adding error handling for SQL Server schema creation.The WithSchema method skips schema configuration for SQL Server but doesn't verify if the schema creation was successful.
Run this script to check if schema creation errors are handled in other parts of the codebase:
tests/schema_test.go (4)
14-15: LGTM! Import statements are well organized.The imports are logically grouped and include all necessary database drivers.
38-43: LGTM! Test setup properly initializes all database drivers.The setup correctly initializes test queries for all supported databases.
47-51: LGTM! Proper cleanup of SQLite resources.The teardown properly handles SQLite cleanup by shutting down the Docker container.
1746-1783: Consider consistent foreign key behavior across databases.The test expectations for foreign keys differ between SQLite and other databases. Consider documenting these differences or implementing a more consistent behavior.
Run this script to check if these differences are documented:
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: b621728 | Previous: d0de53e | Ratio |
|---|---|---|---|
BenchmarkFile_ReadWrite |
244394 ns/op 2073 B/op 27 allocs/op |
162358 ns/op 2072 B/op 27 allocs/op |
1.51 |
BenchmarkFile_ReadWrite - ns/op |
244394 ns/op |
162358 ns/op |
1.51 |
This comment was automatically generated by workflow using github-action-benchmark.
📑 Description
goravel/goravel#540
Summary by CodeRabbit