chore: adopt testcontainers-go for Postgres, MySQL 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 (15)
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
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 (
|
![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 |
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 |
|
Relates #1536, so I'd postpone the review of this one after the other is approved & merged. |
* main: (71 commits) Update dependabot.yml Update benchmark.yml chore(ci): simplify change detection using GH action chore(deps): bump dependabot/fetch-metadata from 2.2.0 to 2.3.0 test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow bump pkg´s test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow test new release drafter combi workflow dynamic change detection based on the folders in the first level of the repository dynamic generation of the release drafter config for the changed folders ...
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
mssql/mssql_test.go (1)
Line range hint
105-112: Improve expiration test verification.The test sets expiration but doesn't verify that the key actually expired.
Add verification after sleep:
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), "key should have expired")
♻️ Duplicate comments (2)
mssql/mssql_test.go (2)
58-60: 🛠️ Refactor suggestionAdd defer cleanup to all test store initializations.
Each test initializes a store but doesn't ensure cleanup.
Add
defer testStore.Close()after store initialization in all test functions:testStore, err := newTestStore(t) require.NoError(t, err) + defer testStore.Close()Also applies to: 71-73, 87-89, 105-107, 117-119, 126-128, 140-142, 157-159, 181-183, 206-208, 233-235, 240-242
247-249: 🛠️ Refactor suggestionAdd defer cleanup to all benchmark store initializations.
Each benchmark initializes a store but doesn't ensure cleanup.
Add
defer testStore.Close()after store initialization in all benchmark functions:testStore, err := newTestStore(b) require.NoError(b, err) + defer testStore.Close()Also applies to: 261-264, 278-280
🧹 Nitpick comments (1)
mssql/mssql_test.go (1)
Line range hint
219-232: Improve panic recovery test.The SSL mode test's panic recovery could be more explicit and provide better error messages.
Consider this improvement:
func Test_SslRequiredMode(t *testing.T) { + expectedMsg := "SSL mode 'require' is not supported" defer func() { - if recover() == nil { - require.Equalf(t, true, nil, "Connection was established with a `require`") + if r := recover(); r == nil { + t.Error("expected panic but got none") + } else if r != expectedMsg { + t.Errorf("expected panic with message %q but got %v", expectedMsg, r) } }() _ = New(Config{
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-mssql.ymlis excluded by!**/*.ymlmongodb/go.modis excluded by!**/*.modmongodb/go.sumis excluded by!**/*.sum,!**/*.sums3/go.modis excluded by!**/*.mods3/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
mssql/mssql_test.go(12 hunks)
🔇 Additional comments (2)
mssql/mssql_test.go (2)
14-21: LGTM! Well-structured constants with clear naming.The constants are well-organized and provide flexibility through environment variables. The default MSSQL image version is explicitly specified, which is good for reproducibility.
31-39: 🛠️ Refactor suggestionAdd timeout and health check for container initialization.
The current implementation has several potential issues:
- Uses non-cancellable context
- No health check to ensure MSSQL is ready
- No explicit cleanup mechanism
Apply this diff to improve container initialization:
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + // 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(), + mssql.WithWaitStrategy( + wait.ForSQL(1433). + WithStartupTimeout(25*time.Second), + ), )Also add container cleanup:
+ t.Cleanup(func() { + if err := c.Terminate(context.Background()); err != nil { + t.Logf("failed to terminate container: %v", err) + } + })Likely invalid or redundant comment.
* main: chore(deps): bump github.com/aws/aws-sdk-go-v2/credentials in /s3
This reverts commit 6742f2a.
mdelapenya
changed the title
It was released a few weeks ago
|
Hi folks @ReneWerner87 @gaby ! 👋 I've updated this PR with current main, and I see that the current client code "could" come with certain flakiness, as I do not see deterministic failures in the tests: they can fail when creating the client connection to the given store, so I'm not sure if the given package should handle retries 🤔 E.g. the clickhouse tests pass in 1.22, but they fail in 1.23 (because of a failed handshake) Another feasible thing is the container is not reliable enough (or not yet), maybe because the current default wait strategy is not enough. I could look more in depth about this adding some retries at the test code, or defining more consistent wait strategies (like for the health-check http endpoint for the given store, if it exits). Thoughts? |
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.
This PR also bumps testcontainers-go from v0.33.0 to v0.35.0
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
Other concerns
I started updating MSSQL module, but the container's pid 1 crashes, so I'm going to postpone its addition, using a separate PR for that.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor