feat(redis): use testcontainers in rueidis and valkey #1723
Conversation
## Walkthrough
This set of changes introduces a new `testredis` helper package to centralize and simplify the management of Redis containers for integration tests across the codebase. Test suites for `redis`, `valkey`, and `rueidis` storage backends are refactored to use this helper, replacing previous custom container setup logic with standardized options for connection mode, TLS, and image selection. The `.github/scripts/gen-test-certs.sh` script, previously used for generating test TLS certificates, is deleted as its functionality is now encapsulated within the new helper and associated test resources. Test logic remains unchanged, but setup and cleanup are streamlined and isolated per test.
## Changes
| Files / Areas Changed | Summary |
|------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------|
| .github/scripts/gen-test-certs.sh | Deleted Bash script for generating test TLS certificates for regression tests. |
| testhelpers/redis/redis.go | Introduced new `testredis` Go package to manage Redis containers for testing with flexible options for TLS, URL, and image. |
| testhelpers/redis/redis_test.go | Added comprehensive unit tests for `testredis` package covering container startup and configuration options. |
| redis/redis_test.go | Refactored to use `testredis` for container setup; removed custom container logic and updated option handling. |
| valkey/valkey_test.go | Refactored to use `testredis` for Redis container setup; removed global store, improved test isolation and TLS test coverage. |
| rueidis/rueidis_test.go | Refactored to use `testredis` for containerized Redis setup; added helpers, improved TLS and connection option testing. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Test as Test Case
participant testredis as testredis Package
participant RedisContainer as Redis Container
Test->>testredis: Start(opts)
testredis->>RedisContainer: Launch with options (TLS, address, image)
RedisContainer-->>testredis: Provide connection details (URL, TLS config)
testredis-->>Test: Return Container info
Test->>Test: Run storage tests using container info
Test->>testredis: Cleanup (on test end)Possibly related PRs
Suggested reviewers
Poem
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
rueidis/rueidis_test.go (1)
92-100:⚠️ Potential issue
Test_Rueidis_Set_Expirationdoesn’t assert that the key expiredThe test only sleeps after setting a TTL but never checks that the value is gone, so it can’t detect a regression in expiration logic.
err := testStore.Set(key, val, exp) require.NoError(t, err) time.Sleep(1100 * time.Millisecond) + + // New assertions: key should have expired + result, err := testStore.Get(key) + require.NoError(t, err) + require.Zero(t, len(result), "expected value to expire after TTL")valkey/valkey_test.go (1)
107-115:⚠️ Potential issue
Test_Valkey_Set_Expirationlacks post-TTL assertionLike its Rueidis counterpart, this test doesn’t verify that the value actually expires.
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), "expected value to expire after TTL")
🧹 Nitpick comments (2)
testhelpers/redis/redis.go (1)
159-165: Ambiguous TLS URL handling may lead to mismatched schemesWhen
UseTLSis true butSecureURLis false, the helper deliberately rewritesrediss://toredis://, while still returning a non-nilTLSConfig.
Although Rueidis & Valkey drivers work today, the mismatch is surprising and may break other callers that rely on the scheme to determine TLS.Consider:
- Documenting the intended behaviour clearly, or
- Introducing an explicit
WithInsecureSchemeOverTLS()option to avoid conflating concerns.No code change is strictly required, but clarifying intent will prevent future confusion.
redis/redis_test.go (1)
387-420: Consider refactoring cluster test in the futureThe Redis cluster test is currently not utilizing the new testredis helper package. This is appropriately explained by the TODO comment indicating that testcontainers-go Redis module doesn't yet support clustering. Consider refactoring this test when clustering support becomes available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-rueidis.ymlis excluded by!**/*.yml.github/workflows/test-valkey.ymlis excluded by!**/*.ymlredis/go.modis excluded by!**/*.modredis/go.sumis excluded by!**/*.sum,!**/*.sumrueidis/go.modis excluded by!**/*.modrueidis/go.sumis excluded by!**/*.sum,!**/*.sumtesthelpers/redis/go.modis excluded by!**/*.modtesthelpers/redis/go.sumis excluded by!**/*.sum,!**/*.sumvalkey/go.modis excluded by!**/*.modvalkey/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
.github/scripts/gen-test-certs.sh(0 hunks)redis/redis_test.go(9 hunks)rueidis/rueidis_test.go(11 hunks)testhelpers/redis/redis.go(1 hunks)valkey/valkey_test.go(11 hunks)
💤 Files with no reviewable changes (1)
- .github/scripts/gen-test-certs.sh
🧰 Additional context used
🧬 Code Graph Analysis (2)
redis/redis_test.go (2)
testhelpers/redis/redis.go (7)
Option(38-38)Start(87-168)Port(19-19)WithHostPort(57-61)WithTLS(41-47)WithAddress(50-54)WithURL(64-68)redis/redis.go (1)
Storage(12-14)
rueidis/rueidis_test.go (1)
testhelpers/redis/redis.go (6)
Option(38-38)Start(87-168)WithTLS(41-47)WithHostPort(57-61)WithAddress(50-54)WithURL(64-68)
🔇 Additional comments (13)
testhelpers/redis/redis.go (1)
130-133:⚠️ Potential issueCrash-on-error:
CleanupContaineris executed even whenredis.Runfails
redis.Runcan return a non-nil error together with anilcontainer.
Callingtestcontainers.CleanupContainer(t, c)before assertingerrtherefore risks a nil-pointer dereference and will panic instead of producing a clean test failure.- c, err := redis.Run(ctx, img, tcOpts...) - testcontainers.CleanupContainer(t, c) - require.NoError(t, err) + c, err := redis.Run(ctx, img, tcOpts...) + require.NoError(t, err) + testcontainers.CleanupContainer(t, c)⛔ Skipped due to learnings
Learnt from: mdelapenya PR: gofiber/storage#1665 File: cassandra/cassandra_test.go:35-38 Timestamp: 2025-04-20T23:52:03.362Z Learning: In testcontainers-go, calling `testcontainers.CleanupContainer(t, c)` before checking the error from container creation is safe due to the implementation details of the library. The CleanupContainer function handles nil or partially initialized containers gracefully.rueidis/rueidis_test.go (1)
280-308: Cluster test depends on a locally running 6-node clusterThe cluster test still targets fixed ports on
localhost. On CI or contributor machines without a Valkey/Rueidis cluster, it will fail.
Untiltestcontainers-gosupports Redis cluster, consider guarding the test witht.Skip()(as done in the Valkey suite) or wiring it through the new helper.redis/redis_test.go (11)
9-9: Good addition of the testredis package importThe test helper package import is correctly added, which enables the refactoring of Redis container management to use the centralized helper.
12-16: Documentation updated to reflect new implementationThe function documentation has been properly updated to reflect that it now uses the testredis helper package for container management, while maintaining its purpose of creating a Redis configuration for tests.
16-30: Good refactoring of container setup logicThe container setup logic has been successfully refactored to delegate the responsibility to the
testredis.Starthelper function, simplifying this code while maintaining the same functionality. The Config struct is properly populated from the container attributes.
37-39: Function signature updated to use new option typeThe function signature has been correctly updated to use the new
testredis.Optiontype, maintaining compatibility with all test cases.
202-202: Test case updated to use new option helperThe test case has been updated to use the
testredis.WithHostPort()option, maintaining the same behavior as before but with a cleaner interface.
220-220: TLS test cases updated to use new option helperThe TLS test cases have been updated to use the
testredis.WithTLS()option with the appropriate parameters, maintaining the same test coverage for both secure/insecure URL and MTLS configurations.
262-262: Universal Addrs test updated with new option helperThe test for address-based connections now uses the
testredis.WithAddress()option, maintaining the same test behavior.
288-288: URL undefined test updated with combination of optionsThis test now correctly uses a combination of
testredis.WithAddress()andtestredis.WithURL(false)options to test the behavior when URL is explicitly undefined.
313-313: URL defined test updated with appropriate optionsThe test for defined URL connections now correctly uses a combination of
testredis.WithAddress()andtestredis.WithURL(true)options.
339-339: Host/Port with Address test updated with appropriate optionsThe test now correctly uses a combination of
testredis.WithAddress(),testredis.WithHostPort(), andtestredis.WithURL(false)options to test the precedence of connection methods.
364-364: Host/Port with URL test updated with appropriate optionsThe test for Host/Port with URL now correctly uses a combination of all three options:
testredis.WithAddress(),testredis.WithHostPort(), andtestredis.WithURL(true).
|
@mdelapenya Only question I had is, does the |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a reusable test helper module for running Redis containers with Testcontainers, which is then integrated into the test suites for Valkey, Rueidis, and Redis. This improves test isolation, reduces duplicated setup code, and updates workflow configurations to support the new testing approach.
- Introduced new helper functions (newConfigFromContainer and newTestStore) in the testhelpers/redis module.
- Updated test files in valkey, rueidis, and redis packages to use the new helper module.
- Adjusted GitHub workflow files for test image and filter configuration.
Reviewed Changes
Copilot reviewed 13 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| valkey/valkey_test.go | Replaced static Redis setup with container-based configuration via newTestStore |
| testhelpers/redis/redis.go | Added reusable Redis container helper functions using Testcontainers |
| rueidis/rueidis_test.go | Integrated new test container API for consistent configuration across tests |
| redis/redis_test.go | Migrated tests to use the shared testcontainers abstraction, eliminating custom container logic |
| .github/workflows/*.yml | Updated workflow configurations to use updated image tags and filter patterns |
Files not reviewed (5)
- .github/scripts/gen-test-certs.sh: Language not supported
- redis/go.mod: Language not supported
- rueidis/go.mod: Language not supported
- testhelpers/redis/go.mod: Language not supported
- valkey/go.mod: Language not supported
Comments suppressed due to low confidence (2)
valkey/valkey_test.go:189
- [nitpick] Use require.NotNil(t, testStore.Conn()) instead of comparing to nil for improved readability and consistency with testify standards.
require.True(t, testStore.Conn() != nil)
valkey/valkey_test.go:250
- In tests where testStore is already being closed using a deferred call, the explicit call to testStore.Close() can be redundant. Consider removing one of these calls to avoid potential double closure.
require.Nil(t, testStore.Close())
|
@gaby I started with a module to avoid pushing the dependencies to the root package. Probably by my lack of knowledge about who is importing this root module. If it's fine to push them to the root, then I'm more than happy to convert it into a package, maybe internal: |
I honestly don't know how the root go.mod is used. @ReneWerner87 probably remembers. Another note, redis-go 9.8.0 came out a few minutes ago with full support for Redis 8.0 |
|
@gaby @mdelapenya |
* main: chore: make sure the client gets the variable locally fix(coherence): set coherence log level to ERROR in benchmarks Update coherence storage to v2.2.0 chore(deps): bump dependabot/fetch-metadata from 2.3.0 to 2.4.0 chore(deps): bump github.com/microsoft/go-mssqldb in /mssql
|
can we refresh this PR |
* main: chore(deps): bump github.com/ClickHouse/clickhouse-go/v2 in /clickhouse chore(deps): bump github.com/minio/minio-go/v7 in /minio chore(deps): bump github.com/valkey-io/valkey-go in /valkey chore(deps): bump github.com/jackc/pgx/v5 in /postgres
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
testhelpers/redis/redis_test.go (2)
21-41: Consider cleaning up environment variables after testThe environment variable test at line 33 uses
t.Setenv()which is good, but be aware that in older Go versions this might not automatically clean up after the test. In Go 1.17+ this is handled automatically.
43-55: Inconsistent parallelization in TLS test casesSome of the TLS test subtests use
t.Parallel()while others don't. Consider making all subtests parallel for consistency and improved test performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/golangci-lint.ymlis excluded by!**/*.yml.github/workflows/release-drafter.ymlis excluded by!**/*.yml.github/workflows/test-teshelpers-redis.ymlis excluded by!**/*.ymlredis/go.modis excluded by!**/*.modredis/go.sumis excluded by!**/*.sum,!**/*.sumvalkey/go.modis excluded by!**/*.modvalkey/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
testhelpers/redis/redis_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
testhelpers/redis/redis_test.go (1)
testhelpers/redis/redis.go (9)
Start(87-168)Port(19-19)WithImage(71-75)ImageEnvVar(18-18)WithTLS(41-47)WithHostPort(57-61)WithAddress(50-54)WithURL(64-68)Image(17-17)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint (rueidis)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.23.x, 7)
🔇 Additional comments (6)
testhelpers/redis/redis_test.go (6)
1-9: Well-structured package and importsThe package name and imports are clear and focused, using only what's needed for the tests.
10-19: LGTM: Good baseline test for default configurationThe default configuration test establishes a clear baseline by verifying all expected container properties.
80-90: Parameter values don't match test nameSimilar to the previous issue, there's a mismatch between the test name "mtls-enabled" and the parameter passed to
WithTLS(false, false). If mTLS should be enabled, the second parameter should befalse(not disabled).
94-113: Good isolation of host/port and address configuration optionsThese tests properly verify that the host/port and address options work as expected and populate the correct fields in the container.
115-157: Well-structured combination testsThe tests with multiple options are thorough and verify that options can be combined correctly, including the important URL/address and TLS combinations.
159-192: Good unit tests for configuration optionsThese unit tests for the configuration option functions are clean, concise, and verify that each option correctly modifies the Config struct.
|
I refreshed this PR. If you find the helper go module as a non good-practice here, I'm fine in moving to any other place, like an |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
valkey/valkey_test.go (1)
131-140: 🛠️ Refactor suggestionTest still doesn't verify actual key expiration behavior.
The
Test_Valkey_Get_Expiredtest continues to check for a non-existent key rather than testing actual expiration. This duplicates the "not-exist" scenario instead of validating TTL behavior.Consider fixing this test to actually verify expiration:
func Test_Valkey_Get_Expired(t *testing.T) { - key := "john" + var ( + key = "john" + val = []byte("doe") + exp = 100 * time.Millisecond + ) testStore := newTestStore(t) defer testStore.Close() + err := testStore.Set(key, val, exp) + require.NoError(t, err) + + time.Sleep(150 * time.Millisecond) + result, err := testStore.Get(key) require.NoError(t, err) require.Zero(t, len(result)) }
🧹 Nitpick comments (1)
testhelpers/redis/redis.go (1)
93-95: Consider improving error message clarity.The panic message mentions "using WithImage()" but there's no such function in the current API. The image is passed directly as a parameter to Start().
- panic("Redis image is not set: callers must provide an image using WithImage()") + panic("Redis image is not set: callers must provide a non-empty image parameter")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
testhelpers/redis/go.modis excluded by!**/*.modtesthelpers/redis/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
redis/redis_test.go(9 hunks)testhelpers/redis/redis.go(1 hunks)testhelpers/redis/redis_test.go(1 hunks)valkey/valkey_test.go(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- redis/redis_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
testhelpers/redis/redis_test.go (1)
testhelpers/redis/redis.go (7)
Start(83-168)Port(15-15)WithTLS(34-40)WithHostPort(50-54)WithAddress(43-47)WithURL(57-61)Config(19-28)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.23.x, 8)
🔇 Additional comments (14)
testhelpers/redis/redis_test.go (4)
1-11: Well-structured test package with appropriate imports.The package declaration and imports are clean and include all necessary dependencies for comprehensive testing of the Redis container helper.
16-193: Comprehensive test coverage with good use of parallel execution.The
TestStartfunction provides excellent test coverage for various Redis container configurations:
- Validates panic behavior for missing image
- Tests default configuration
- Covers TLS scenarios with proper parameter combinations
- Tests different connection modes (host/port, address, URL)
- Includes a real connectivity test with go-redis client
The parallel execution of subtests is well-implemented to improve test efficiency.
170-192: Integration test properly addresses previous feedback.The connectivity test effectively validates that the Redis container actually works end-to-end by:
- Parsing the connection URL
- Creating a go-redis client with proper configuration
- Performing a ping operation
- Asserting the expected "PONG" response
This addresses the previous review suggestion for adding integration testing beyond configuration validation.
195-231: Clean unit tests for configuration options.The
TestConfigfunction provides focused unit tests for each configuration option function, ensuring they correctly modify the Config struct fields. The parallel execution and clear test structure are good practices.valkey/valkey_test.go (5)
3-12: Clean imports and dependencies for containerized testing.The new imports properly include the testredis helper package and necessary sync primitives for the singleton benchmark store pattern.
27-67: Well-designed helper functions for test isolation.The helper functions provide excellent abstractions:
initBenchmarkStore: Implements singleton pattern with proper sync.Once usage for benchmarksnewConfigFromContainer: Creates Redis config from container with environment variable override supportnewTestStore: Provides clean interface for creating isolated test storesThe documentation clearly explains container lifecycle and caller responsibilities.
207-245: Excellent parameterized TLS testing with comprehensive coverage.The refactored TLS tests provide thorough coverage of all TLS scenarios:
- Secure/insecure URL combinations
- mTLS enabled/disabled scenarios
- Proper parameter passing to the testredis helper
This is a significant improvement over previous static TLS testing approaches.
310-339: Appropriate handling of cluster testing limitations.The test is properly skipped with a clear TODO comment explaining the dependency on testcontainers-go Valkey module cluster support. This is a reasonable approach until the upstream dependency provides the required functionality.
341-384: Efficient benchmark implementation with singleton pattern.The benchmarks correctly use the singleton store pattern to avoid container startup overhead during performance testing. The
initBenchmarkStoreensures the container is created once and reused across all benchmark iterations.testhelpers/redis/redis.go (5)
1-16: Clean package structure with clear constants.The package declaration and port constant are well-defined. The testcontainers and Redis module imports are appropriate for the functionality provided.
18-31: Well-designed configuration structure using functional options pattern.The Config struct clearly separates different aspects of Redis container configuration:
- TLS settings (UseTLS, SecureURL, MTLSDisabled)
- Connection modes (UseAddress, UseHostPort, UseURL)
- Container lifecycle (name for reuse)
The functional options pattern with the Option type provides a clean, extensible API.
33-70: Comprehensive option functions with clear semantics.All option functions are well-implemented:
WithTLS: Properly handles both URL security and mTLS configurationWithAddress,WithHostPort,WithURL: Provide different connection mode optionsWithReuse: Enables container reuse for performance optimizationThe function signatures and parameter names clearly indicate their purpose.
72-80: Container struct provides comprehensive connection information.The Container struct exposes all necessary connection details for different client libraries and usage patterns:
- URL for simple connections
- Addrs for cluster/sentinel scenarios
- Host/Port for direct connection
- TLSConfig for secure connections
- cmds for debugging/testing purposes
This flexibility supports various Redis client libraries and connection approaches.
82-168: Robust Start function with proper error handling and lifecycle management.The Start function implementation is well-structured:
- Proper helper marking and default configuration
- Panic for missing image (fail-fast approach)
- Correct TLS command construction for Redis server
- Proper container lifecycle management with cleanup
- Flexible connection information extraction based on options
The logic correctly handles the different URL schemes and TLS configurations.
|
@ReneWerner87 @gaby I did a deep research on why the valkey module suffered a huge regression after the changes in this PR (before the singleton container applied to the valkey benchmarks, see 331a84b). Let me explain why I chose to initialise the container just once for the valkey benchmarks and let's validate if this is a valid concern. Thanks Cursor for the support 🤖 : Root Cause AnalysisThe performance regression in the benchmarks was caused by the interaction between Go's benchmark framework and testcontainers-go's container lifecycle management. Here's what was happening: 1. Benchmark Framework Behavior:
2. Original Implementation Issues:
3. Impact on Performance:
Interesting discoverySo in the Redis module:
This makes the situation even more interesting:
SolutionThe solution implemented a singleton pattern with container reuse: 1. Container Reuse:
2. Thread-Safe Singleton:
3. Removed Cleanup Overhead:
ResultsThe changes restored performance to optimal levels:
This solution aligns with the main branch's approach of using a singleton store while leveraging testcontainers-go's container reuse capabilities for efficient testing. Key Takeaways:
|
| @@ -28,7 +28,7 @@ jobs: | |||
| - name: Generate filters | |||
| id: filter-setup | |||
| run: | | |||
| filters=$(find . -maxdepth 1 -type d ! -path ./.git ! -path . -exec basename {} \; | grep -v '^\.' | awk '{printf "%s: \"%s/**\"\n", $1, $1}') | |||
| filters=$(find . -maxdepth 1 -type d ! -path ./.git ! -path . ! -path ./testhelpers -exec basename {} \; | grep -v '^\.' | awk '{printf "%s: \"%s/**\"\n", $1, $1}') | |||
There was a problem hiding this comment.
question: the helper tests won't be executed because of this. Have you considered moving to go.work so that it's easier to identify the module paths?
What does this PR do?
It creates a
testhelpers/redistest module, abstracting the current test implementation for running Redis with testcontainers. This test module has been applied to redis, rueidis and valkey, as they all start a redis/valkey container for testing.Why is it important?
Do not repeat the same code for starting redis/valkey across three different modules.
Other concerns
Please let me know if this is too over-engineered 🙏 so I can step back and provide a simpler solution. I chose this way because there are multiple combinations for creating redis in the current tests: with/out TLS, using secure/unsecure schema, using/not using MTLS, using URL, using HostPort...
Summary by CodeRabbit
New Features
Refactor
Chores
Summary by CodeRabbit