chore: adopt testcontainers-go for Postgres, MySQL, MSSQL and MongoDB #1515
Conversation
mdelapenya
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes involve significant modifications to multiple test files, primarily replacing the previous test store initialization with a new function, Changes
Possibly related issues
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (8)
mongodb/go.modis excluded by!**/*.modmongodb/go.sumis excluded by!**/*.sum,!**/*.summssql/go.modis excluded by!**/*.modmssql/go.sumis excluded by!**/*.sum,!**/*.summysql/go.modis excluded by!**/*.modmysql/go.sumis excluded by!**/*.sum,!**/*.sumpostgres/go.modis excluded by!**/*.modpostgres/go.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (9)
- .github/workflows/benchmark.yml (2 hunks)
- .github/workflows/test-mongodb.yml (2 hunks)
- .github/workflows/test-mssql.yml (2 hunks)
- .github/workflows/test-mysql.yml (2 hunks)
- .github/workflows/test-postgres.yml (2 hunks)
- mongodb/mongodb_test.go (11 hunks)
- mssql/mssql_test.go (12 hunks)
- mysql/mysql_test.go (13 hunks)
- postgres/postgres_test.go (12 hunks)
Additional comments not posted (32)
.github/workflows/test-mysql.yml (1)
30-31: LGTM!The introduction of the
TEST_MYSQL_IMAGEenvironment variable is a good practice to ensure consistency and reliability in the testing environment..github/workflows/test-mongodb.yml (2)
19-20: LGTM!Updating the Go version matrix to include newer versions is a good practice to ensure compatibility and performance with the latest Go versions.
30-30: LGTM!The introduction of the
TEST_MONGODB_IMAGEenvironment variable is a good practice to ensure consistency and reliability in the testing environment..github/workflows/test-postgres.yml (1)
30-31: LGTM!The introduction of the
TEST_POSTGRES_IMAGEenvironment variable is a good practice to ensure consistency and reliability in the testing environment..github/workflows/test-mssql.yml (1)
30-31: LGTM!The addition of the environment variable
TEST_MSSQL_IMAGEis correct and aligns with the PR objective..github/workflows/benchmark.yml (4)
118-118: LGTM!The addition of the environment variable
TEST_MONGODB_IMAGEis correct and aligns with the PR objective.
119-119: LGTM!The addition of the environment variable
TEST_MSSQL_IMAGEis correct and aligns with the PR objective.
120-120: LGTM!The addition of the environment variable
TEST_MYSQL_IMAGEis correct and aligns with the PR objective.
121-121: LGTM!The addition of the environment variable
TEST_POSTGRES_IMAGEis correct and aligns with the PR objective.mongodb/mongodb_test.go (10)
10-10: LGTM!The import statement for
testcontainers-go/modules/mongodbis correct and necessary for using the testcontainers-go library for MongoDB.
13-19: LGTM!The constant block for MongoDB image and credentials is correctly defined and aligns with the PR objective.
21-44: LGTM!The
newTestStorefunction is correctly implemented and improves the isolation of tests by ensuring that each test starts with a fresh instance of the MongoDB store.
53-55: LGTM!The modification of the
Test_MongoDB_Setfunction to usenewTestStoreis correct and aligns with the PR objective.
66-68: LGTM!The modification of the
Test_MongoDB_Set_Overridefunction to usenewTestStoreis correct and aligns with the PR objective.
82-84: LGTM!The modification of the
Test_MongoDB_Getfunction to usenewTestStoreis correct and aligns with the PR objective.
100-102: LGTM!The modification of the
Test_MongoDB_Set_Expirationfunction to usenewTestStoreis correct and aligns with the PR objective.
112-114: LGTM!The modification of the
Test_MongoDB_Get_Expiredfunction to usenewTestStoreis correct and aligns with the PR objective.
121-123: LGTM!The modification of the
Test_MongoDB_Get_NotExistfunction to usenewTestStoreis correct and aligns with the PR objective.
135-137: LGTM!The modification of the
Test_MongoDB_Deletefunction to usenewTestStoreis correct and aligns with the PR objective.mssql/mssql_test.go (4)
4-11: LGTM!The import statements are correct and necessary for the functionality.
14-21: LGTM!The constants improve configurability and readability.
23-51: LGTM!The function is correctly implemented and ensures each test runs in isolation with a fresh database connection.
Line range hint
59-264: LGTM!The updates ensure that each test retrieves its own instance of the storage, improving test isolation and reliability.
mysql/mysql_test.go (5)
4-11: LGTM!The import statements are correct and necessary for the functionality.
14-21: LGTM!The constants improve configurability and readability.
23-39: LGTM!The function is correctly implemented and ensures each test runs in isolation with a fresh database connection.
41-57: LGTM!The function is correctly implemented and ensures the MySQL container is properly configured and started.
Line range hint
59-294: LGTM!The updates ensure that each test retrieves its own instance of the storage, improving test isolation and reliability.
postgres/postgres_test.go (4)
Line range hint
4-13: LGTM!The import statements are correct and necessary for the functionality.
16-23: LGTM!The constants improve configurability and readability.
25-56: LGTM!The function is correctly implemented and ensures each test runs in isolation with a fresh database connection.
Line range hint
64-282: LGTM!The updates ensure that each test retrieves its own instance of the storage, improving test isolation and reliability.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/test-mongodb.yml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test-mongodb.yml
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
mongodb/go.modis excluded by!**/*.modpostgres/go.modis excluded by!**/*.mod
Files selected for processing (3)
- .github/workflows/test-mssql.yml (2 hunks)
- .github/workflows/test-mysql.yml (2 hunks)
- .github/workflows/test-postgres.yml (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test-mssql.yml
- .github/workflows/test-postgres.yml
Additional comments not posted (4)
.github/workflows/test-mysql.yml (4)
19-19: LGTM!The addition of Go version 1.22.x to the matrix is appropriate.
20-20: LGTM!The addition of Go version 1.23.x to the matrix is appropriate.
30-30: LGTM!Setting the
TEST_MYSQL_IMAGEenvironment variable todocker.io/mysql:9ensures consistency and reliability in the testing environment.
31-31: LGTM!Running the tests with the specified MySQL image is appropriate.
|
For some reason, the memcache service is failing in the benchmark job. Any guess? |
|
sorry overlooked, that's really strange, I'll have to take a look at it in the next few days |
|
@ReneWerner87 any update on the benchmark thing? 🙏 Once merge, I can send a new PR with more stores |
|
unfortunately haven't had time to look yet, will have a look on sunday and fix it then, but feel free to make pull requests for the other stores if you like in this case we only have to apply the fix to the other PR |
|
@mdelapenya now the reason you removed the other services 
now the options from postgres for the health check belongs to memcached 
but there is no postgres started 
other question |
* main: Update minio/README.md Set default MaxRetry value in configDefault Add MaxRetry for MinIO operations chore(deps): bump benchmark-action/github-action-benchmark chore(deps): bump tj-actions/changed-files from 44 to 45 chore(deps): bump dependabot/fetch-metadata from 2.1.0 to 2.2.0
|
@ReneWerner87 I've added 78da7fc in order to align the modules modified by this PR (couchbase, mongo, mssql, mysql and postgres) in regards of the Go version matrix: all using 1.21 and above, as in the existing minio module. I have one question regarding the clickhouse module, which runs the tests for 1.21 and 1.22: I guess we still want to do it in 1.23 too, right? I can add it in this PR. Last thing: the benchmark is failing, and I think it's because the default timeout per test (60s) it's not enough to pull the MSSQL Docker image (2.19GB). Would it be OK to add the |
|
Mmmm not the timeout but this: https://github.com/gofiber/storage/actions/runs/12277044952/job/34255559810?pr=1515#step:14:729 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
mssql/mssql_test.go (1)
Line range hint
227-239: Simplify panic handling inTest_SslRequiredModeusingrequire.PanicsThe current implementation manually handles panic recovery, which can be simplified for clarity and readability. Use
require.Panicsto assert that a panic occurs as expected.Apply this diff to simplify the test:
func Test_SslRequiredMode(t *testing.T) { - defer func() { - if recover() == nil { - require.Equalf(t, true, nil, "Connection was established with a `require`") - } - }() - _ = New(Config{ - Database: "fiber", - Username: "username", - Password: "password", - Reset: true, - SslMode: "require", - }) + require.Panics(t, func() { + _ = New(Config{ + Database: "fiber", + Username: "username", + Password: "password", + Reset: true, + SslMode: "require", + }) + }, "Expected panic when connecting with SslMode `require`") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (20)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-clickhouse.ymlis excluded by!**/*.yml.github/workflows/test-mongodb.ymlis excluded by!**/*.yml.github/workflows/test-mssql.ymlis excluded by!**/*.yml.github/workflows/test-mysql.ymlis excluded by!**/*.yml.github/workflows/test-postgres.ymlis excluded by!**/*.ymlclickhouse/go.modis excluded by!**/*.modclickhouse/go.sumis excluded by!**/*.sum,!**/*.sumcouchbase/go.modis excluded by!**/*.modcouchbase/go.sumis excluded by!**/*.sum,!**/*.summinio/go.modis excluded by!**/*.modminio/go.sumis excluded by!**/*.sum,!**/*.summongodb/go.modis excluded by!**/*.modmongodb/go.sumis excluded by!**/*.sum,!**/*.summssql/go.modis excluded by!**/*.modmssql/go.sumis excluded by!**/*.sum,!**/*.summysql/go.modis excluded by!**/*.modmysql/go.sumis excluded by!**/*.sum,!**/*.sumpostgres/go.modis excluded by!**/*.modpostgres/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
mssql/mssql_test.go(12 hunks)
🔇 Additional comments (15)
mssql/mssql_test.go (15)
64-67: Proper initialization and cleanup of testStore
The testStore is correctly initialized using newTestStore(t), and resources are properly released with defer testStore.Close(). This ensures isolation and prevents resource leaks in the test.
78-81: Proper initialization and cleanup of testStore
The testStore is correctly initialized and closed in Test_MSSQL_Set_Override. Resource management is handled appropriately.
95-98: Proper initialization and cleanup of testStore
The test setup in Test_MSSQL_Get is correctly handled.
114-117: Proper initialization and cleanup of testStore
The expiration test is set up correctly, with appropriate use of time.Sleep to allow the key to expire.
127-130: Proper initialization and cleanup of testStore
Test_MSSQL_Get_Expired correctly checks for expired keys.
137-140: Proper initialization and cleanup of testStore
Test_MSSQL_Get_NotExist correctly handles non-existent keys.
152-155: Proper initialization and cleanup of testStore
Test_MSSQL_Delete correctly tests the deletion of keys.
170-173: Proper initialization and cleanup of testStore
Test_MSSQL_Reset correctly tests the reset functionality.
195-198: Proper initialization and cleanup of testStore
Test_MSSQL_GC correctly tests garbage collection of expired keys.
221-224: Proper initialization and cleanup of testStore
Test_MSSQL_Non_UTF8 correctly handles non-UTF8 values.
249-251: Ensure testStore is closed in Test_MSSQL_Close
The testStore is initialized but not deferred for closure. Since this test specifically tests the Close method, it's acceptable. However, ensure that any resources are properly cleaned up.
256-259: Proper initialization and cleanup of testStore
Test_MSSQL_Conn correctly tests the connection retrieval.
264-267: Proper initialization and cleanup of testStore in benchmarks
Benchmark_MSSQL_Set is correctly set up with resource management.
279-282: Proper initialization and cleanup of testStore in benchmarks
Benchmark_MSSQL_Get is correctly set up.
297-300: Proper initialization and cleanup of testStore in benchmarks
Benchmark_MSSQL_SetAndDelete correctly initializes and cleans up testStore.
mssql/mssql_test.go
Outdated
| ctx := context.Background() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a cancellable context in newTestStore for better resource management
Currently, newTestStore uses context.Background(), which is not cancellable. To prevent potential context leaks and allow for better control over resource cleanup, consider using a cancellable context such as context.WithCancel or context.WithTimeout.
Apply this diff to implement a cancellable context with a timeout:
func newTestStore(t testing.TB) (*Storage, error) {
t.Helper()
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
img := mssqlImage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx := context.Background() | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() | |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
postgres/postgres_test.go (1)
28-34: Use a cancellable context for better resource managementCurrently,
newTestStoreusescontext.Background(), which is not cancellable. To allow better control over resource cleanup and prevent potential context leaks, consider using a cancellable context such ascontext.WithTimeoutorcontext.WithCancel.Apply this diff to implement a cancellable context:
func newTestStore(t testing.TB) (*Storage, error) { t.Helper() - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() img := postgresImage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mssql/go.modis excluded by!**/*.mod
📒 Files selected for processing (2)
mssql/mssql_test.go(12 hunks)postgres/postgres_test.go(12 hunks)
🔇 Additional comments (1)
mssql/mssql_test.go (1)
33-34: Use a cancellable context for better resource management
Consider using a cancellable context like context.WithTimeout to prevent potential context leaks and allow better control over resource cleanup.
Apply this diff:
func newTestStore(t testing.TB) (*Storage, error) {
t.Helper()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
- ctx := context.Background()| testStore, err := newTestStore(t) | ||
| require.NoError(t, err) | ||
| defer testStore.Close() | ||
|
|
||
| err = testStore.Set(key, val, exp) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertion to verify key expiration
The test currently sets a key with an expiration and sleeps for the duration of the expiration but does not verify that the key has expired. Consider adding an assertion to check that the key is no longer retrievable after expiration.
Apply this diff to add the assertion:
err = testStore.Set(key, val, exp)
require.NoError(t, err)
time.Sleep(1100 * time.Millisecond)
+result, err := testStore.Get(key)
+require.NoError(t, err)
+require.Zero(t, len(result))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testStore, err := newTestStore(t) | |
| require.NoError(t, err) | |
| defer testStore.Close() | |
| err = testStore.Set(key, val, exp) | |
| testStore, err := newTestStore(t) | |
| require.NoError(t, err) | |
| defer testStore.Close() | |
| err = testStore.Set(key, val, exp) | |
| require.NoError(t, err) | |
| time.Sleep(1100 * time.Millisecond) | |
| result, err := testStore.Get(key) | |
| require.NoError(t, err) | |
| require.Zero(t, len(result)) |
mssql/mssql_test.go
Outdated
| testStore, err := newTestStore(t) | ||
| require.NoError(t, err) | ||
| defer testStore.Close() | ||
|
|
||
| err = testStore.Set(key, val, exp) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertion to verify key expiration
The test currently sets a key with an expiration and sleeps for the duration of the expiration but does not verify that the key has expired. Consider adding an assertion to check that the key is no longer retrievable after expiration.
Apply this diff to add the assertion:
err = testStore.Set(key, val, exp)
require.NoError(t, err)
time.Sleep(1100 * time.Millisecond)
+result, err := testStore.Get(key)
+require.NoError(t, err)
+require.Zero(t, len(result))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| testStore, err := newTestStore(t) | |
| require.NoError(t, err) | |
| defer testStore.Close() | |
| err = testStore.Set(key, val, exp) | |
| testStore, err := newTestStore(t) | |
| require.NoError(t, err) | |
| defer testStore.Close() | |
| err = testStore.Set(key, val, exp) | |
| require.NoError(t, err) | |
| time.Sleep(1100 * time.Millisecond) | |
| result, err := testStore.Get(key) | |
| require.NoError(t, err) | |
| require.Zero(t, len(result)) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
mssql/mssql_test.go (2)
14-21: Consider adding version constraints documentationThe MSSQL image version is hardcoded to a specific version. Consider documenting version compatibility requirements and testing strategy for future upgrades.
Add a comment block explaining:
- Minimum supported version
- Version upgrade policy
- Testing requirements for version changes
Line range hint
57-307: Add stress tests for production-like scenariosConsider adding the following test cases to better simulate production conditions:
- Concurrent operations test
- Large dataset handling
- Connection pool exhaustion scenarios
Example concurrent test:
func Test_MSSQL_Concurrent_Operations(t *testing.T) { testStore, err := newTestStore(t) require.NoError(t, err) defer testStore.Close() const goroutines = 10 var wg sync.WaitGroup wg.Add(goroutines) for i := 0; i < goroutines; i++ { go func(id int) { defer wg.Done() key := fmt.Sprintf("key-%d", id) value := []byte(fmt.Sprintf("value-%d", id)) err := testStore.Set(key, value, 0) require.NoError(t, err) result, err := testStore.Get(key) require.NoError(t, err) require.Equal(t, value, result) }(i) } wg.Wait() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/benchmark.ymlis excluded by!**/*.ymlmssql/go.modis excluded by!**/*.mod
📒 Files selected for processing (1)
mssql/mssql_test.go(12 hunks)
🔇 Additional comments (1)
mssql/mssql_test.go (1)
23-56: Enhance error handling test coverage
The newTestStore function should include tests for database connection failures and retry mechanisms.
Let's verify the current error handling coverage:
Consider adding:
- Test case for invalid connection string
- Test case for network timeouts
- Retry mechanism for transient failures
mssql/mssql_test.go
Outdated
| func newTestStore(t testing.TB) (*Storage, error) { | ||
| t.Helper() | ||
|
|
||
| img := mssqlImage | ||
| if imgFromEnv := os.Getenv(mssqlImageEnvVar); imgFromEnv != "" { | ||
| img = imgFromEnv | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
|
|
||
| // Due to licensing restrictions you are required to explicitly accept an EULA for this container image. | ||
| c, err := mssql.Run(ctx, img, | ||
| mssql.WithPassword(mssqlPass), | ||
| mssql.WithAcceptEULA(), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| t.Cleanup(func() { | ||
| if c != nil { | ||
| require.NoError(t, c.Terminate(ctx)) | ||
| } | ||
| }) | ||
|
|
||
| conn, err := c.ConnectionString(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return New(Config{ | ||
| ConnectionURI: conn, | ||
| Reset: true, | ||
| }), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add container health check for reliable test initialization
The container setup should include a health check to ensure MSSQL is fully operational before proceeding with tests.
Add health check options to the container configuration:
c, err := mssql.Run(ctx, img,
mssql.WithPassword(mssqlPass),
mssql.WithAcceptEULA(),
+ mssql.WithWaitStrategy(wait.ForSQL(1433).WithStartupTimeout(30*time.Second)),
)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
mssql/mssql_test.go (4)
14-21: LGTM! Well-structured constants with clear documentation.The constants are well-organized and documented. The inclusion of an environment variable override for the MSSQL image provides good flexibility for testing.
Consider adding a comment explaining the significance of the default password complexity, as it might not be immediately obvious why "MsSql!1234" was chosen (it meets MSSQL's password complexity requirements).
234-242: Enhance SSL mode test coverageThe SSL mode test could be more comprehensive by testing different SSL modes and connection scenarios.
Consider adding tests for:
- Valid SSL mode configurations
- Different SSL mode options
- Invalid certificate scenarios
Line range hint
261-290: Add concurrent operation benchmarksThe current benchmarks test single operations, but it would be valuable to test concurrent operations to ensure thread safety.
Consider adding:
- Concurrent Set operations benchmark
- Concurrent Get operations benchmark
- Mixed concurrent operations benchmark
23-56: Add error path test coverageThe
newTestStorefunction has error handling, but there are no tests for these error paths.Consider adding tests for:
- Invalid image name
- Container startup failures
- Connection string retrieval failures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-mssql.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
mssql/mssql_test.go(12 hunks)
🔇 Additional comments (3)
mssql/mssql_test.go (3)
31-32: 🛠️ Refactor suggestion
Add context timeout for container operations
Using context.Background() without a timeout could lead to hanging tests if container operations take too long.
-ctx := context.Background()
+ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+defer cancel()Likely invalid or redundant comment.
114-121: 🛠️ Refactor suggestion
Verify key expiration in the test
The test sets a key with expiration but doesn't verify that the key has actually expired.
err = testStore.Set(key, val, exp)
require.NoError(t, err)
time.Sleep(1100 * time.Millisecond)
+
+// Verify the key has expired
+result, err := testStore.Get(key)
+require.NoError(t, err)
+require.Zero(t, len(result))Likely invalid or redundant comment.
34-37: 🛠️ Refactor suggestion
Add container health check and configuration options
The container setup should include health checks and proper configuration for reliable test initialization.
c, err := mssql.Run(ctx, img,
mssql.WithPassword(mssqlPass),
mssql.WithAcceptEULA(),
+ mssql.WithWaitStrategy(wait.ForSQL(1433).WithStartupTimeout(30*time.Second)),
+ mssql.WithInitDatabase(mssqlDatabase),
)Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
minio/minio_test.go (1)
Line range hint
24-53: Enhance container initialization with timeout and health checkThe container initialization could be improved in two ways:
- Replace
context.Background()with a timeout context- Add a health check strategy to ensure MinIO is ready
Apply this diff to improve container initialization:
func newTestStore(t testing.TB) (*Storage, error) { t.Helper() - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() c, err := minio.Run(ctx, img, minio.WithUsername(minioUser), minio.WithPassword(minioPass), + testcontainers.WithWaitStrategy( + wait.ForLog("Status: 1 Online, 0 Offline"). + WithStartupTimeout(20*time.Second), + ), )mssql/mssql_test.go (1)
231-239: Improve SSL mode panic test accuracyThe test's panic message and configuration don't clearly represent the SSL requirement scenario.
Apply this diff to improve test clarity:
require.Panics(t, func() { _ = New(Config{ - Database: "fiber", - Username: "username", - Password: "password", + ConnectionURI: "sqlserver://localhost:1433?sslmode=require", Reset: true, - SslMode: "require", }) -}, "Expected panic when connecting to MSSQL with SSL mode set to require") +}, "Expected panic when connecting with SSL required but no valid certificate")postgres/postgres_test.go (1)
28-33: Add timeout to context for better resource managementWhile the wait strategy is well implemented, the background context should have a timeout.
Apply this diff:
-ctx := context.Background() +ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) +defer cancel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clickhouse/clickhouse_test.go(4 hunks)couchbase/couchbase_test.go(11 hunks)minio/minio_test.go(16 hunks)mongodb/mongodb_test.go(11 hunks)mssql/mssql_test.go(12 hunks)mysql/mysql_test.go(13 hunks)postgres/postgres_test.go(12 hunks)
🔇 Additional comments (11)
mongodb/mongodb_test.go (2)
22-46: Good Use of newTestStore Function for Test Isolation
The introduction of the newTestStore function enhances test isolation by providing a fresh MongoDB instance for each test. Resource cleanup is properly handled with defer testStore.Close(), and error handling is appropriately managed.
25-28: Verify Custom MongoDB Image Compatibility
When overriding the MongoDB image using the TEST_MONGODB_IMAGE environment variable, please ensure that the custom image is compatible with the test setup to prevent unexpected failures.
Run the following script to check if the environment variable is set and verify the image:
✅ Verification successful
Let me check the default MongoDB image and test configuration to better understand the compatibility requirements.
Custom MongoDB Image Override is Well-Implemented
The code is using MongoDB 7.x as the default image and allows for custom image override through the TEST_MONGODB_IMAGE environment variable. The implementation is safe because:
- It uses a stable default image (
mongo:7) - The test container is created using the official testcontainers-go MongoDB module
- The container setup includes proper authentication with username and password
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if TEST_MONGODB_IMAGE is set and output its value.
if [ -n "$TEST_MONGODB_IMAGE" ]; then
echo "TEST_MONGODB_IMAGE is set to: $TEST_MONGODB_IMAGE"
# Optionally, pull the image to ensure it's available
docker pull "$TEST_MONGODB_IMAGE"
else
echo "TEST_MONGODB_IMAGE is not set. Using default image."
fi
Length of output: 128
Script:
#!/bin/bash
# Find the default MongoDB image and test configuration
rg -A 5 "mongoDBImage.*="
rg "mongo" mongodb/mongodb_test.go
Length of output: 1251
mysql/mysql_test.go (2)
19-20: Confirm MySQL Credentials Are Correctly Assigned
The constants for mysqlUser and mysqlPass are defined as:
mysqlUser string = "username"
mysqlPass string = "password"Ensure that these credentials match the expected values for the MySQL container. Previously, there was an issue where these values were swapped.
50-57: 🛠️ Refactor suggestion
Refactor mustStartMySQL to Return Error Instead of Failing Test
In the mustStartMySQL function, calling require.NoError(t, err) causes the test to fail immediately if there's an error starting the MySQL container. It's better practice to return the error and handle it in newTestStore, providing clearer error propagation and maintaining consistency in error handling.
Apply this diff to modify error handling:
func mustStartMySQL(t testing.TB) (*mysql.MySQLContainer, error) {
img := mysqlImage
if imgFromEnv := os.Getenv(mysqlImageEnvVar); imgFromEnv != "" {
img = imgFromEnv
}
ctx := context.Background()
c, err := mysql.Run(ctx, img,
mysql.WithPassword(mysqlPass),
mysql.WithUsername(mysqlUser),
mysql.WithDatabase(mysqlDatabase),
)
- testcontainers.CleanupContainer(t, c)
- require.NoError(t, err)
+ if err != nil {
+ return nil, err
+ }
+ testcontainers.CleanupContainer(t, c)
return c, nil
}Update newTestStore to handle the error:
func newTestStore(t testing.TB) (*Storage, error) {
t.Helper()
- c := mustStartMySQL(t)
+ c, err := mustStartMySQL(t)
+ if err != nil {
+ return nil, err
+ }
ctx := context.Background()
conn, err := c.ConnectionString(ctx)
if err != nil {
return nil, err
}
return New(Config{
ConnectionURI: conn,
Reset: true,
}), nil
}Likely invalid or redundant comment.
couchbase/couchbase_test.go (1)
16-16: Verify Compatibility with Updated Couchbase Image
The Couchbase image has been updated to enterprise-7.6.3. Please verify that this version is compatible with your test suite, as there may be breaking changes or deprecations compared to enterprise-7.1.1.
Run the following script to check for any known issues in the new Couchbase version:
✅ Verification successful
Let me search for test files and configurations to understand how Couchbase is being used in the tests.
Let me check the implementation details of the Couchbase storage to understand the features being used.
Let me check the dependencies to verify the Couchbase SDK version being used.
Let me try a different approach to check the go.mod file.
Based on the analysis of the codebase, I can now provide the final response:
Couchbase Version Update Appears Safe
The test suite uses basic Couchbase operations (Get, Set, Delete, Reset) through the gocb/v2 SDK, and these core operations remain stable across Couchbase versions. The tests are using testcontainers with simple bucket configuration and basic CRUD operations that are well-supported across versions.
- The test implementation uses container-based testing that allows version override via
TEST_COUCHBASE_IMAGEenvironment variable - Only standard KV operations are used: Get, Set, Delete, Reset
- No usage of advanced features that might be version-sensitive
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Couchbase release notes for version 7.6.3
# Prompt to review release notes manually
echo "Please review the Couchbase 7.6.3 release notes for any breaking changes or deprecations:"
echo "https://docs.couchbase.com/server/7.6/release-notes/relnotes.html"
Length of output: 327
Script:
#!/bin/bash
# Search for Couchbase-related test files and configurations
echo "=== Searching for Couchbase test files ==="
fd -e go -e yaml -e json . | grep -i "couchbase"
echo -e "\n=== Checking test implementation details ==="
rg -l "couchbase" --type go
echo -e "\n=== Examining test implementation ==="
cat couchbase/couchbase_test.go
Length of output: 4991
Script:
#!/bin/bash
# Check the implementation details
echo "=== Examining Couchbase implementation ==="
cat couchbase/couchbase.go
echo -e "\n=== Examining Couchbase config ==="
cat couchbase/config.go
Length of output: 3497
Script:
#!/bin/bash
# Check module dependencies
echo "=== Examining go.mod for Couchbase SDK version ==="
cat go.mod | grep -A 5 "couchbase"
Length of output: 145
Script:
#!/bin/bash
# Check entire go.mod content
echo "=== Examining complete go.mod ==="
cat go.mod
Length of output: 132
clickhouse/clickhouse_test.go (2)
197-206: Addition of TestClose_ShouldReturn_NoError Enhances Test Coverage
The new test TestClose_ShouldReturn_NoError appropriately verifies that the Close method performs as expected without errors, improving the overall test coverage for the ClickHouse storage implementation.
43-43:
Ensure Error Handling Before Cleanup in getTestConnection
In getTestConnection, the call to testcontainers.CleanupContainer(t, c) should occur after checking for errors. If err is not nil, the container c may not be properly initialized, leading to potential panics or undefined behavior when attempting cleanup.
Apply this diff to adjust the order:
c, err := clickhouse.Run(ctx,
img,
clickhouse.WithUsername(clickhouseUser),
clickhouse.WithPassword(clickhousePass),
clickhouse.WithDatabase(clickhouseDB),
)
-if err != nil {
- return nil, err
-}
-testcontainers.CleanupContainer(t, c)
+if err != nil {
+ return nil, err
+}
+testcontainers.CleanupContainer(t, c)Ensure that testcontainers.CleanupContainer(t, c) is called only after confirming err is nil.
Likely invalid or redundant comment.
minio/minio_test.go (2)
Line range hint 111-119: Add verification for key expiration
The test sets a key with expiration but doesn't verify if the key actually expired.
Apply this diff to verify the expiration:
err = testStore.Set(key, val, exp)
require.NoError(t, err)
time.Sleep(1100 * time.Millisecond)
+
+result, err := testStore.Get(key)
+require.NoError(t, err)
+require.Zero(t, len(result))Line range hint 258-305: LGTM: Benchmarks are well-implemented
The benchmark implementations correctly handle resource initialization and cleanup.
mssql/mssql_test.go (1)
24-53: 🛠️ Refactor suggestion
Enhance MSSQL container initialization with timeout and health check
Similar to MinIO, the container initialization needs timeout context and health check strategy.
Apply this diff to improve container initialization:
func newTestStore(t testing.TB) (*Storage, error) {
t.Helper()
- ctx := context.Background()
+ ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+ defer cancel()
c, err := mssql.Run(ctx, img,
mssql.WithPassword(mssqlPass),
mssql.WithAcceptEULA(),
+ testcontainers.WithWaitStrategy(
+ wait.ForSQL(1433).
+ WithStartupTimeout(25*time.Second),
+ ),
)Likely invalid or redundant comment.
postgres/postgres_test.go (1)
115-122: Add verification for key expiration
The test sets a key with expiration but doesn't verify if the key actually expired.
Apply this diff to verify the expiration:
err = testStore.Set(key, val, exp)
require.NoError(t, err)
time.Sleep(1100 * time.Millisecond)
+
+result, err := testStore.Get(key)
+require.NoError(t, err)
+require.Zero(t, len(result))
|
@ReneWerner87 I think this PR is ready to go BUT there is a crash in the MSSQL server benchmarks that I'm not able to fix, as it requires tuning up the I think I linked to some issues above regarding this, so it's documented in the PR. We can keep this PR open as is until the ubuntu + mssql limitation is fixed, and then decide what to do. I'm sorry it's taking that long 😞 |
|
BTW, I can remove MSSQL from this PR, and move on with the rest of the storages, wdyt? |
|
Another question: given the amount of commits here, what's the preferred way of sending PRs: squashing commits to just one, or they will be squashed when merged? |
|
@ReneWerner87 the benchmarks for the modules I modified here are passing, the current failure is in the S3 module, so I guess it's certain flakiness. Could you please re-run the benchmarks build? I think, now that MSSQL server is back to the original state, we are really close to merge this one 😄 |
|
sure |
What does this PR do?
It uses testcontainers-go in the integration tests for the given stores, refactoring the tests to always consume a new test store, backed by a Docker container.
WHy is it important?
Reproducibility: any developer will be able to run the tests locally exactly equals as in the Github Actions.
Related issues
For reference, please check out previous work for Couchbase and Minio (#1508) and ClickHouse (#1506)
There is more ongoing work, as defined in #1507
Summary by CodeRabbit
New Features
Bug Fixes
Refactor