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 (7)
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 WalkthroughThis pull request refactors the test setups across multiple storage backends by replacing static initialization of test stores with dynamic instantiation using Testcontainers. The changes add new functions—primarily Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Case
participant N as newTestStore
participant C as Testcontainers
participant S as Storage Instance
T->>N: Call newTestStore(t)
N->>C: Request container startup
C->>N: Signal container readiness (ports, health checks)
N->>T: Return new Storage instance
T->>S: Execute test operations
S-->>T: Cleanup on test completion
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 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 comments (1)
mysql/mysql_test.go (1)
137-152:⚠️ Potential issueAdd expiration verification in Test_MYSQL_Set_Expiration.
The test sets expiration and waits but doesn't verify that the key has actually expired.
func Test_MYSQL_Set_Expiration(t *testing.T) { var ( key = "john" val = []byte("doe") exp = 1 * time.Second ) 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) + + // Verify key has expired + result, err := testStore.Get(key) + require.NoError(t, err) + require.Zero(t, len(result), "Key should have expired") }
🧹 Nitpick comments (3)
mongodb/mongodb_test.go (1)
25-58: Consider adding retries for MongoDB connection.While the wait strategy ensures the container is ready, MongoDB might need additional time to accept connections in some cases.
func newTestStore(t testing.TB) (*Storage, error) { t.Helper() img := mongoDBImage if imgFromEnv := os.Getenv(mongoDBImageEnvVar); imgFromEnv != "" { img = imgFromEnv } ctx := context.Background() + + // Add retry logic for MongoDB connection + var store *Storage + var err error + for i := 0; i < 3; i++ { + store, err = tryConnect(ctx, t, img) + if err == nil { + return store, nil + } + time.Sleep(time.Second) + } + return store, err } + +func tryConnect(ctx context.Context, t testing.TB, img string) (*Storage, error) { c, err := mongodb.Run( ctx, img, mongodb.WithUsername(mongoDBUser), mongodb.WithPassword(mongoDBPass), testcontainers.WithWaitStrategy( wait.ForAll( wait.ForListeningPort(mongoDBPort), wait.ForLog(mongoDBReadyLog), ), ), ) testcontainers.CleanupContainer(t, c) if err != nil { return nil, err } conn, err := c.ConnectionString(ctx) if err != nil { return nil, err } return New(Config{ ConnectionURI: conn, Reset: true, }), nilpostgres/postgres_test.go (1)
34-40: Enhance wait strategy for more reliable container startup.The current basic wait strategy might not be sufficient in all environments.
c, err := postgres.Run(ctx, img, postgres.WithUsername(postgresUser), postgres.WithPassword(postgresPass), postgres.WithDatabase(postgresDatabase), - postgres.BasicWaitStrategies(), + testcontainers.WithWaitStrategy( + wait.ForAll( + wait.ForLog("database system is ready to accept connections").WithOccurrence(2), + wait.ForListeningPort("5432/tcp"), + ), + ), )mysql/mysql_test.go (1)
51-61: Consider adding health check in wait strategy.The current wait strategy checks for port and log, but a health check would provide stronger guarantees.
c, err := mysql.Run(ctx, img, mysql.WithPassword(mysqlPass), mysql.WithUsername(mysqlUser), mysql.WithDatabase(mysqlDatabase), testcontainers.WithWaitStrategy( wait.ForListeningPort("3306/tcp"), wait.ForLog("port: 3306 MySQL Community Server"), + wait.ForSQL("3306/tcp", "mysql", func(port nat.Port) string { + return fmt.Sprintf("root:%s@(localhost:%s)/%s", mysqlPass, port.Port(), mysqlDatabase) + }).WithStartupTimeout(time.Second * 30), ), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
clickhouse/clickhouse_test.go(5 hunks)minio/minio_test.go(17 hunks)mongodb/mongodb_test.go(10 hunks)mysql/mysql_test.go(13 hunks)neo4j/neo4j_test.go(2 hunks)postgres/postgres_test.go(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- neo4j/neo4j_test.go
- minio/minio_test.go
🧰 Additional context used
🧠 Learnings (1)
mysql/mysql_test.go (2)
Learnt from: mdelapenya
PR: gofiber/storage#1515
File: mysql/mysql_test.go:18-18
Timestamp: 2025-02-18T17:22:10.331Z
Learning: The `mysql:9` Docker image exists and is actively maintained, with regular updates on Docker Hub. The image was verified through Docker Hub's API and is suitable for testing purposes.
Learnt from: mdelapenya
PR: gofiber/storage#1515
File: mysql/mysql_test.go:18-18
Timestamp: 2025-02-18T17:22:10.331Z
Learning: The `mysql:9` Docker image exists and can be used for testing, although it represents a development version of MySQL. The image is available at docker.io/mysql:9.
🪛 GitHub Actions: Tests Clickhouse
clickhouse/clickhouse_test.go
[error] 170-170: Received unexpected error: [handshake] unexpected packet [72] from server in Test_Delete
[error] 192-192: Received unexpected error: [handshake] unexpected packet [72] from server in Test_Reset
[error] 214-214: Received unexpected error: [handshake] unexpected packet [72] from server in TestClose_ShouldReturn_NoError
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: compare (s3)
- GitHub Check: compare (mongodb)
- GitHub Check: compare (clickhouse)
- GitHub Check: compare (minio)
- GitHub Check: compare (mysql)
- GitHub Check: compare (postgres)
- GitHub Check: compare (neo4j)
- GitHub Check: compare (couchbase)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.22.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.21.x)
- GitHub Check: Tests (1.22.x)
- GitHub Check: Tests (1.21.x)
🔇 Additional comments (1)
mongodb/mongodb_test.go (1)
15-23: LGTM! Well-structured constants.The constants are well-organized and provide clear configuration for the MongoDB container setup.
| testcontainers.WithWaitStrategy( | ||
| wait.ForAll( | ||
| wait.ForListeningPort(clickhouseHttpPort), | ||
| wait.NewHTTPStrategy("/").WithPort(clickhouseHttpPort).WithStatusCodeMatcher(func(status int) bool { | ||
| return status == clickhouseSuccessCode | ||
| }), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Fix handshake errors by adjusting wait strategy.
The current wait strategy is causing handshake errors in the pipeline. The HTTP check might complete before the native protocol port is ready.
testcontainers.WithWaitStrategy(
wait.ForAll(
wait.ForListeningPort(clickhouseHttpPort),
- wait.NewHTTPStrategy("/").WithPort(clickhouseHttpPort).WithStatusCodeMatcher(func(status int) bool {
- return status == clickhouseSuccessCode
- }),
+ wait.ForLog("Ready for connections").WithStartupTimeout(time.Second * 30),
+ wait.ForListeningPort("9000/tcp"),
),
),📝 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.
| testcontainers.WithWaitStrategy( | |
| wait.ForAll( | |
| wait.ForListeningPort(clickhouseHttpPort), | |
| wait.NewHTTPStrategy("/").WithPort(clickhouseHttpPort).WithStatusCodeMatcher(func(status int) bool { | |
| return status == clickhouseSuccessCode | |
| }), | |
| ), | |
| ), | |
| testcontainers.WithWaitStrategy( | |
| wait.ForAll( | |
| wait.ForListeningPort(clickhouseHttpPort), | |
| wait.ForLog("Ready for connections").WithStartupTimeout(time.Second * 30), | |
| wait.ForListeningPort("9000/tcp"), | |
| ), | |
| ), |
|
Hi again, sorry for taking that long for this PR, too many tasks on my plate 🤦 In any case, I improved the wait strategies in the modules in the project, which potential additions for the upstream modules in testcontainers-go. It's failing for couchbase because the pipeline takes more than the 120s that are defined in the GH workflow. Is increasing that timeout an issue for you? |
|
@mdelapenya Make it 300s (5mins) |
|
In my Mac M1: |
* main: quickfix: update tests fix: uses existing table if exists Add migrate.md Update README.md Update README.md Update README.md Update README.md Update test-leveldb.yml Update README.md DBPath Converted to Path Small things fixed Update leveldb_test.go coderabbitai recommendations were evaluated and added leveldb_test update Update config_test.go workflow added leveldb support added
|
@gaby @ReneWerner87 this branch is now green! It took time, but we finally made it. Major highlight here was a timeout for the couchbase benchmarks. I increased the timeout to 500s to bypass it. In general, all benchmarks take 1-2 minutes, but for some reason couchbase not. I rebased the branch to cover the recent changes in the postgres module, so I think it's ready for start the review. As always, please let me know anything you need here. |
|
Will review today after work, thanks 💪 |
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