chore: use testcontainers-go for couchbase and minio #1508
Conversation
mdelapenya
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe recent changes enhance the testing infrastructure by introducing environment variables for Couchbase and MinIO images, streamlining the testing workflows. The modifications reduce complexity, improve error handling, and ensure isolation between tests. Utilizing predefined Docker images simplifies setup and improves consistency across testing environments, leading to more robust and maintainable test code. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Functions
participant Store as NewTestStore Function
participant Storage as Storage Instance
Test->>Store: Call newTestStore()
Store->>Storage: Initialize MinIO or Couchbase
Store-->>Test: Return Storage, Error (if any)
Test->>Test: Proceed with tests if no error
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 as PR comments)
Additionally, you can add 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
Outside diff range, codebase verification and nitpick comments (3)
minio/minio_test.go (2)
22-56: Consider adding logging for debugging purposes.Adding logs for container initialization and connection string retrieval can aid in debugging issues during test setup.
Here's a suggestion:
log.Printf("Initializing MinIO container with image: %s", img) ... log.Printf("MinIO connection string: %s", conn)
Line range hint
65-279: Consider adding assertions to verify state changes.Adding assertions to verify the state of the storage after operations can enhance test coverage and reliability.
For example, after
testStore.Set, you can assert that the key exists in the storage..github/workflows/benchmark.yml (1)
177-178: Document the new environment variables for clarity.Consider updating the documentation to include
TEST_COUCHBASE_IMAGEandTEST_MINIO_IMAGEto ensure clarity for future contributors.You can add comments in the workflow file or update the project's README.md.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
couchbase/go.modis excluded by!**/*.modcouchbase/go.sumis excluded by!**/*.sum,!**/*.summinio/go.modis excluded by!**/*.modminio/go.sumis excluded by!**/*.sum,!**/*.sum
Files selected for processing (6)
- .github/workflows/benchmark.yml (1 hunks)
- .github/workflows/test-couchbase.yml (1 hunks)
- .github/workflows/test-minio.yml (1 hunks)
- clickhouse/clickhouse_test.go (1 hunks)
- couchbase/couchbase_test.go (8 hunks)
- minio/minio_test.go (11 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/test-couchbase.yml
Additional comments not posted (5)
.github/workflows/test-minio.yml (1)
29-30: LGTM! The workflow change enhances consistency.The introduction of the
TEST_MINIO_IMAGEenvironment variable improves the reliability and consistency of the testing environment by using a predefined Docker image.couchbase/couchbase_test.go (2)
22-53: LGTM! Enhanced error handling and flexibility.The
newTestStorefunction now returns an error, improving error handling in test cases. The use of an environment variable for the Couchbase image enhances configuration flexibility.
Line range hint
57-179: LGTM! Improved robustness in test cases.The addition of error handling in test cases ensures that tests only proceed if the store is successfully created, preventing misleading results.
clickhouse/clickhouse_test.go (2)
26-26: LGTM! Simplified function interface.The removal of the
TestOrBenchinterface in favor of usingtesting.TBdirectly simplifies the code and reduces complexity.
30-30: LGTM! Enhanced maintainability with environment variables.Using an environment variable for the ClickHouse image improves maintainability and clarity in the code.
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 (1)
couchbase/go.modis excluded by!**/*.mod
Files selected for processing (1)
- .github/workflows/benchmark.yml (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/benchmark.yml
|
Based on our policies, this would be a breaking change. We have to discuss it since it bumps the min version of |
|
I found that the Go version is not consistently set across the different stores, that's why I went ahead with the bump, supported by the fact that Go 1.21 is EOL (https://endoflife.date/go):
Besides that, I also found similar issues as with clickhouse regarding compilation, and remembered the comment about bumping the Go version for clickhouse mod file. In any case, I'm open to any discussion about this, doing what's best for the project. Thanks! |
|
I did not mention but, once decided which version to use, I can help and submit a PR updating the version. |
|
thx LGTM |
|
there is a problem with the benchmark |
|
or maybe its not related -> i will retrigger it |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/test-couchbase.yml (1 hunks)
- .github/workflows/test-minio.yml (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/test-couchbase.yml
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/test-minio.yml
|
strange i triggered the benchmark in the master , lets see, then it is there i will just merge it |
|
@ReneWerner87 thanks for this! I'm preparing another batch PR. What Go versions should I use in the CI matrix? I.e. mongodb tests against Go 1.19, 1.20 and 1.21. Should we standardise to use 3 versions (current, current -1 and current -2)? |
|
thank you with the goversions it is such a thing, can we make the adjustments without changing the golang versions? each adapter is independent and has its own major version unless it is necessary, we want to spare the consumer a major version switch for as long as possible |
|
Makes sense, thanks. I'll update the store tests only, leaving to you the decision of bumping the Go versions based on the special needs for each one. |
What does this PR do?
This PR adds testcontainers-go to the Couchbase and Minio modules, updating the current code to be consistent across modules in how the store is created for testing purpose (minio created one container per test suite, instead of per test method).
Why is it important?
Increase adoption of testcontainers-go for consistency when running the integration tests on contributions.
Follow-ups
Consider a) running one container per test suite Vs b) one container per test function. While a) is optimal for resource allocation, it can lead to test data polution if tests are not written correctly to create/destroy its data. OTOH b) can lead to have more containers running, but given the type of tests here, I'd say this is meaningless for the current state of the project.
Summary by CodeRabbit
New Features
Improvements
Refactor
testing.TBtype.