feat(redis): support testcontainers-go in the Redis module (no cluster support yet)#1676
Conversation
|
""" WalkthroughThe Redis storage test suite has been refactored to dynamically create and configure Redis test containers for each test using a new helper function. This approach replaces static Redis configurations with containerized instances that support various connection parameters and TLS settings. The refactor introduces flexible configuration options, container-managed TLS setup, and automated container lifecycle management. All existing tests and benchmarks have been updated to utilize this new setup, ensuring isolated, configurable, and reproducible test environments. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Helper as newTestStore
participant TC as testcontainers-go
participant Redis as Redis Container
Test->>Helper: Call newTestStore(opts)
Helper->>TC: Request Redis container (with TLS/settings)
TC->>Redis: Start container with configured options
Helper->>Redis: Wait for readiness on appropriate port
Helper->>Test: Return configured Storage instance
Test->>Storage: Execute test logic
Test->>TC: Container cleanup deferred after test
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
mdelapenya
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
|
@gaby @ReneWerner87 I think this can add value enabling local testing of redis, although testing the cluster support is not included... yet. If you like this approach, I can work on contributing the TLS support to the upstream Redis module in tc-go, so that upcoming releases benefit from it. Wdyt? |
![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
🧹 Nitpick comments (6)
redis/redis_test.go (6)
29-39: Use consistent Go naming for “withMTLSdisabled”Variable naming in Go typically prefers consistent casing (e.g.,
withMTLSDisabled). Updating this field name would improve readability:- withMTLSdisabled bool + withMTLSDisabled bool
73-75: Clarify function comment to correctly reference Redis server certificatesThe comment mentions generating a “nats certificate,” but it actually creates a Redis server certificate. Update the comment for clarity:
-// createTLSCerts creates a CA certificate, a client certificate and a nats certificate, +// createTLSCerts creates a CA certificate, a client certificate and a Redis server certificate,
184-189: Consider restricting file permissions for private keysMounting the private key with file mode 0o644 may be fine in ephemeral test containers, but stricter permissions like 0o600 can reduce risk:
- FileMode: 0o644, + FileMode: 0o600,
404-404: Fix spelling typo in test function name“Test_Redis_Initalize_WithHostPort” is missing the second “i” in “Initialize”:
-func Test_Redis_Initalize_WithHostPort(t *testing.T) { +func Test_Redis_Initialize_WithHostPort(t *testing.T) {
424-424: Fix spelling typo in test function name“Test_Redis_Initalize_WithURL_TLS_Verify” also has a missing “i” in “Initialize”:
-func Test_Redis_Initalize_WithURL_TLS_Verify(t *testing.T) { +func Test_Redis_Initialize_WithURL_TLS_Verify(t *testing.T) {
627-629: Optimize container management in benchmarksRepeatedly spinning up a container in each benchmark may lead to longer execution times. If your goal is to measure the overhead of interactions rather than container creation, consider reusing a single container across benchmark tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-redis.ymlis excluded by!**/*.ymlredis/go.modis excluded by!**/*.modredis/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
redis/redis_test.go(13 hunks)
🔇 Additional comments (1)
redis/redis_test.go (1)
4-9: Imports look goodThese newly introduced imports appear necessary and are properly utilized:
• "bytes" for Buffer I/O when mounting TLS certificates
• "context" for container orchestration
• "net" for IP handling
• "os" for environment variables
• "strings" for scheme trimming
|
Away from keyboard now, but the tests fail because of the nitpick suggestion by @coderabbitai on changing the cert permissions 😅 |
|
Your organization has reached its limit of developer seats under the Pro Plan. In order to use the Chat feature, you need to buy additional seats |
This reverts commit 06e9b1f.
|
I could not resist it: testcontainers/testcontainers-go#3115 |
Awesome! 💪 |
* main: (48 commits) chore(deps): bump github.com/microsoft/go-mssqldb in /mssql chore(deps): bump github.com/testcontainers/testcontainers-go Add missing Go versions Remove new benchmarks, they are not needed. Update redis/README.md Add missing condition Add missing condition for Valkey during Benchmarks Update dependencies, add script for updating in the future Add unit tests for Redis connection Add example for using existing Redis connection Added TLS/SSL options in Readme Added TLS/SSL options chore(deps): bump github.com/aws/aws-sdk-go-v2/feature/dynamodb/attributevalue chore(deps): bump github.com/aws/aws-sdk-go-v2/service/dynamodb chore(deps): bump github.com/mdelapenya/tlscert in /nats chore(deps): bump github.com/minio/minio-go/v7 in /minio chore(deps): bump github.com/Azure/azure-sdk-for-go/sdk/storage/azblob chore(deps): bump github.com/aws/aws-sdk-go-v2/feature/s3/manager in /s3 chore(deps): bump github.com/stretchr/testify in /mssql chore: apply suggestions from code review ...
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
redis/redis_test.go (1)
89-90: Uset.Context()(Go 1.20+) instead ofcontext.Background()Tying the context to the test lifecycle allows automatic cancellation when a test is aborted or times out.
-ctx := context.Background() +ctx := t.Context()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-redis.ymlis excluded by!**/*.ymlredis/go.modis excluded by!**/*.modredis/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (1)
redis/redis_test.go(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests (1.24.x, 7)
🔇 Additional comments (1)
redis/redis_test.go (1)
120-123:⚠️ Potential issueCheck
errbefore registering cleanup to prevent nil‐pointer issues
testcontainers.CleanupContainer(t, c)is called before verifying thatredis.Runsucceeded.
Iferr != nil,cisnil; some future change in the helper could dereference the container pointer and panic.- 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
redis/redis_test.go (6)
71-78: Consider adding unit tests for option functionsWhile your options pattern is well-implemented, consider adding unit tests that verify each option function correctly modifies the settings struct. This would ensure configuration behavior remains consistent if options are modified in the future.
506-538: Document future cluster support plansThe Test_Redis_Cluster test still uses hardcoded addresses instead of testcontainers. Consider adding a comment explaining that cluster support is pending testcontainers-go Redis module enhancements, as mentioned in the PR description.
func Test_Redis_Cluster(t *testing.T) { + // TODO: Replace with testcontainers-based setup once Redis cluster support + // is available in testcontainers-go Redis module testStoreUniversal := New(Config{ Addrs: []string{
17-22: Consider adding validation for environment variable inputWhen reading the Redis image from an environment variable, consider validating that it's a valid image name to prevent potential failures with invalid inputs.
func newTestStore(t testing.TB, opts ...testStoreOption) *Storage { // ... img := redisImage if imgFromEnv := os.Getenv(redisImageEnvVar); imgFromEnv != "" { + // Validate image name format + if !strings.Contains(imgFromEnv, "/") && !strings.Contains(imgFromEnv, ":") { + t.Logf("Warning: Potentially invalid Redis image format: %s, using default", imgFromEnv) + } img = imgFromEnv } // ... }
85-87: Consider making Reset configurableYou're always setting
Reset: truein the test Config. For some test scenarios, it might be useful to make this configurable through an option.// Add a new option function +func withReset(reset bool) testStoreOption { + return func(o *testStoreSettings) { + o.reset = reset + } +} // Update the testStoreSettings struct type testStoreSettings struct { withAddress bool withHostPort bool withURL bool + reset bool // TLS settings withSecureURL bool withMTLSDisabled bool withTLS bool } // Update the initialization in newTestStore settings := &testStoreSettings{ withURL: true, // by default, the URL will be set to the URI provided by the testcontainer withAddress: false, withHostPort: false, + reset: true, // by default, reset the Redis database } // Update the Config creation cfg := Config{ - Reset: true, + Reset: settings.reset, }
68-158: Consider adding error handling with context for container creation failuresThe error handling for container creation is minimal. Consider adding more context to the error to help with debugging container initialization issues.
c, err := redis.Run(ctx, img, tcOpts...) testcontainers.CleanupContainer(t, c) -require.NoError(t, err) +if err != nil { + t.Fatalf("Failed to create Redis container with image %s: %v", img, err) +}
94-95: Consider customizable container startup timeoutThe container startup timeout is hardcoded to 10 seconds. For CI environments with slower resources, this might not be enough. Consider making this configurable.
// Add a constant for default timeout +const defaultContainerStartupTimeout = 10 * time.Second // Add to testStoreSettings type testStoreSettings struct { // ... existing fields + startupTimeout time.Duration } // Add a new option function +func withStartupTimeout(timeout time.Duration) testStoreOption { + return func(o *testStoreSettings) { + o.startupTimeout = timeout + } +} // Update initialization settings := &testStoreSettings{ // ... existing initialization + startupTimeout: defaultContainerStartupTimeout, } // Update the waitStrategy waitStrategies := []wait.Strategy{ - wait.ForListeningPort(redisPort).WithStartupTimeout(time.Second * 10), + wait.ForListeningPort(redisPort).WithStartupTimeout(settings.startupTimeout), }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
redis/redis_test.go(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.24.x, 6)
- GitHub Check: Tests (1.23.x, 6)
🔇 Additional comments (4)
redis/redis_test.go (4)
97-117: TLS configuration looks goodThe approach to conditionally set TLS configuration for Redis container looks correct. You're properly setting the TLS certificates and handling the MTLS disabled case.
143-145: Good fix for address scheme handlingProperly trimming both
redis://andrediss://schemes from the URI addresses a common issue when using TLS connections.
335-377: Well-structured TLS test casesThe parametrized test approach with distinct sub-tests for all four combinations of secure URL and MTLS settings is well organized. Good job passing the testing.T parameter to the test function to fix the previous issue with sub-test reporting.
57-66: Good documentation on the withURL optionThe detailed comments explaining the purpose and behavior of the withURL option function are very helpful for maintainers and future contributors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
redis/redis_test.go (3)
104-111: Simplify TLS auth-client flag handlingThe current conditional logic for
--tls-auth-clientscould be simplified for better readability.if settings.withMTLSDisabled { cmds = append(cmds, "--tls-auth-clients", "no") } else { cmds = append(cmds, "--tls-auth-clients", "yes") } + // Alternative approach: + // cmds = append(cmds, "--tls-auth-clients", func() string { + // if settings.withMTLSDisabled { + // return "no" + // } + // return "yes" + // }())
211-233: Consider polling instead of fixed sleep to avoid flaky testsThe test uses a fixed sleep duration (1100ms) to wait for key expiration, which could lead to flaky tests if the system is under load or in CI environments.
Consider using a polling approach with timeout instead:
- time.Sleep(1100 * time.Millisecond) + // Poll with timeout to make test more reliable + timeout := time.Now().Add(2 * time.Second) + var result []byte + var err error + for time.Now().Before(timeout) { + result, err = testStore.Get(key) + if err != nil || len(result) == 0 { + break + } + time.Sleep(100 * time.Millisecond) + } - result, err := testStore.Get(key) require.NoError(t, err) require.Zero(t, len(result))
23-32: Consider renaming struct fields for consistencyThe
testStoreSettingsstruct fields use awithprefix which is redundant since the option functions also use this prefix.type testStoreSettings struct { - withAddress bool - withHostPort bool - withURL bool + address bool + hostPort bool + url bool // TLS settings - withSecureURL bool - withMTLSDisabled bool - withTLS bool + secureURL bool + mtlsDisabled bool + tls bool }This would require updating the field references in the rest of the code. While this is a pure style improvement, it would make the code more consistent and readable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
redis/redis_test.go(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: compare (redis)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.23.x, 7)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.24.x, 6)
🔇 Additional comments (4)
redis/redis_test.go (4)
67-151: Well-structured container configuration function with good separation of concernsThe
newConfigFromContainerfunction is comprehensive and follows good practices:
- Proper use of the options pattern
- Marking as test helper for better errors
- Configuring TLS correctly
- Ensuring container cleanup
- Supporting multiple connection modes (URL, host/port, address)
332-374: Excellent TLS testing structure with comprehensive coverageThis test is well-structured and covers all combinations of secure/insecure URLs and mTLS enabled/disabled. The parameterized test function with subtests is a clean approach.
537-583: Well-implemented benchmarks using test containersThe benchmarks properly use the new container-based test infrastructure, ensuring consistent testing methodology between tests and benchmarks.
503-535:Details
❓ Verification inconclusive
Consider containerizing the Redis cluster test for consistency
While I understand that cluster support isn't yet available in testcontainers-go Redis module (as mentioned in the PR description), it would be good to add a TODO comment explaining why this test isn't using containers like the others.
🏁 Script executed:
#!/bin/bash # Check if cluster support is now available in testcontainers-go cd /tmp go mod init example.com/check go get github.com/testcontainers/testcontainers-go go get github.com/testcontainers/testcontainers-go/modules/redis echo "Checking for cluster support in testcontainers-go Redis module:" find $GOPATH/pkg/mod/github.com/testcontainers -type f -name "*.go" | xargs grep -l "cluster" | grep -i "redis"Length of output: 1434
Add a TODO for why this test isn’t containerized
Test_Redis_Cluster in redis/redis_test.go (lines 503–535) currently runs against a local cluster, whereas other tests leverage testcontainers-go. Since the Redis module in testcontainers-go doesn’t yet support cluster mode, please add a
// TODO:comment above this test explaining the discrepancy (e.g. “TODO: migrate to testcontainers-go Redis cluster container once supported; see PR description for details”).
- File: redis/redis_test.go
- Lines: 503–535
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
redis/redis_test.go (5)
34-46: Add documentation to option functionsWhile the option functions are well-implemented, they lack documentation explaining their purpose, similar to how
withURLis documented.Consider adding documentation comments to the
withAddressandwithHostPortfunctions like:+// withAddress sets the test store to use address-based connection. func withAddress() testStoreOption { return func(o *testStoreSettings) { o.address = true } } +// withHostPort sets the test store to use host and port based connection. func withHostPort() testStoreOption { return func(o *testStoreSettings) { o.hostPort = true } }
48-54: Document the withTLS option functionThe
withTLSfunction lacks documentation explaining its purpose and parameters.Consider adding documentation like:
+// withTLS configures the test store to use TLS. +// Parameters: +// - secureURL: when true, uses "rediss://" scheme, otherwise uses "redis://" +// - mtlsDisabled: when true, disables mutual TLS authentication (--tls-auth-clients no) func withTLS(secureURL bool, mtlsDisabled bool) testStoreOption { return func(o *testStoreSettings) { o.tls = true o.secureURL = secureURL o.mtlsDisabled = mtlsDisabled } }
67-87: Add documentation to newConfigFromContainer functionThe
newConfigFromContainerfunction is well-implemented but lacks documentation explaining its purpose.Consider adding documentation like:
+// newConfigFromContainer creates a Redis configuration using a test container. +// It configures the container based on the provided options and returns a Config +// that can be used to connect to the container. +// The container is cleaned up when the test completes. func newConfigFromContainer(t testing.TB, opts ...testStoreOption) Config { t.Helper() // rest of function...
154-156: Add documentation to newTestStore functionThe
newTestStorefunction lacks documentation explaining its purpose.Consider adding documentation like:
+// newTestStore creates a new Redis storage instance backed by a test container. +// It configures the container based on the provided options and returns a Storage +// instance connected to the container. The caller is responsible for calling +// Close() on the returned Storage when done. func newTestStore(t testing.TB, opts ...testStoreOption) *Storage { return New(newConfigFromContainer(t, opts...)) }
504-536: Consider containerizing the Redis cluster testThe Redis cluster test still uses a static configuration without containers, which differs from the approach used in all other tests. According to the PR objectives, this is because cluster support is not yet available in the testcontainers-go Redis module. Consider adding a comment explaining this limitation.
Add a comment explaining why this test doesn't use the containerized approach:
func Test_Redis_Cluster(t *testing.T) { + // TODO: Replace with containerized cluster when testcontainers-go Redis module supports clustering testStoreUniversal := New(Config{ Addrs: []string{ "localhost:7000", "localhost:7001",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
redis/redis_test.go(15 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Tests (1.23.x, 6)
- GitHub Check: Tests (1.24.x, 7)
- GitHub Check: Tests (1.24.x, 6)
- GitHub Check: Tests (1.23.x, 7)
🔇 Additional comments (8)
redis/redis_test.go (8)
4-14: Import and package organization looks goodThe import additions for the test container dependencies and context management are appropriate. Including both the main testcontainers package and the Redis module is a good practice.
16-21: Constants are well-defined with commentsThe constants provide good flexibility with default values and environment variable override capability for the Redis image. This approach allows for testing with different Redis versions.
23-32: Well-structured test settings structThe
testStoreSettingsstruct provides a clean way to configure test containers with different connection and TLS options. The comments for the TLS settings section add good clarity.
104-109: Good fix for the TLS auth clients flag issueYour implementation of the TLS auth clients flag setting correctly addresses the past review comment by using a function to conditionally return the appropriate value rather than adding duplicate flags.
137-139: Good handling of URL schemesThe code properly handles both
redis://andrediss://URI schemes when extracting the address, which addresses the previous review comment.
333-358: Well-structured TLS test with proper test helper patternThe TLS test function is well-implemented, using a helper function that takes the testing.T instance to properly report errors in subtests. This addresses the previous review comment about sub-tests reporting to the wrong testing.T instance.
589-590: Good use of container for NewFromConnection testUsing the container configuration for the NewFromConnection test ensures consistency with the other tests and avoids relying on a local Redis instance.
538-541:Details
❓ Verification inconclusive
Consider benchmark implications of using containers
Using containers in benchmarks might affect the results due to container overhead. Consider documenting this potential impact or providing a way to use a non-containerized approach for accurate benchmarking.
While the current implementation works, containers add overhead that might impact benchmark results. Consider if it makes sense to document this limitation or provide a flag to use a non-containerized Redis instance for benchmarks.
Also applies to: 553-556, 570-573
🏁 Script executed:
#!/bin/bash # Test if the benchmark results differ significantly between containerized and non-containerized Redis # Run this only if you have a local Redis instance running on the default port # First, let's check if non-containerized Redis is available if redis-cli ping > /dev/null 2>&1; then echo "Local Redis instance is available for benchmarking comparison" else echo "No local Redis instance available, skipping comparison" exit 0 fi # Extract a benchmark function to a temporary file to run with both approaches cat > /tmp/redis_benchmark.go << 'EOF' package main import ( "testing" "github.com/gofiber/storage/redis" ) func BenchmarkRedisSet(b *testing.B) { store := redis.New(redis.Config{Reset: true}) defer store.Close() b.ResetTimer() for i := 0; i < b.N; i++ { store.Set("key", []byte("value"), 0) } } func main() { b := testing.B{} BenchmarkRedisSet(&b) } EOF # This is a simplified check and won't actually run - would need more setup to execute properly echo "This comparison would need a proper benchmark harness to execute"Length of output: 144
Consider container overhead in Redis benchmarks
We couldn’t validate the performance delta locally (no non-containerized Redis instance detected), so the risk of container overhead remains. It’s still worth:
- Documenting that these benchmarks run against a containerized Redis and may include extra latency.
- Providing an option or flag to run benchmarks against a local/non-containerized instance for closer-to-bare-metal measurements.
Locations to update:
- redis/redis_test.go: lines 538–541, 553–556, 570–573
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
redis/redis_test.go (1)
156-159: Consider enhancing URL scheme replacementThe URL scheme replacement works, but it might be more robust to check if the URL starts with "rediss://" before attempting to replace it.
- if !settings.secureURL { - // Replace the scheme with the insecure one - cfg.URL = strings.Replace(cfg.URL, "rediss://", "redis://", 1) + if !settings.secureURL && strings.HasPrefix(cfg.URL, "rediss://") { + // Replace the scheme with the insecure one + cfg.URL = strings.Replace(cfg.URL, "rediss://", "redis://", 1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
redis/redis_test.go(16 hunks)
🔇 Additional comments (9)
redis/redis_test.go (9)
16-21: Good use of constants and environment variable for Redis configurationThe constants provide a clear way to configure the Redis test container, and the environment variable allows for flexible testing with different Redis versions, which is a great practice for compatibility testing.
23-32: Clean and well-structured test settings implementationThe
testStoreSettingsstruct provides a clear separation of concerns between connection methods and TLS configuration, making the test setup highly flexible and maintainable.
36-71: Well-implemented functional options patternGreat job using the functional options pattern for configuring test stores. The functions are well-documented and provide clear semantics for different connection methods and TLS configurations.
103-123: TLS configuration is properly handledThe TLS configuration properly handles both secure and mutual TLS scenarios. The approach of using a function to determine the
--tls-auth-clientsvalue is elegant and avoids duplicate flag issues.
145-150: Correct handling of both Redis URL schemesGood job properly trimming both
redis://andrediss://schemes when populating theAddrsfield, ensuring addresses are always in the formathost:port.
164-170: Elegant and concise test store creationThe
newTestStorefunction provides a clean API for test setup, encapsulating the container creation and configuration logic.
347-389: Well-structured TLS verification testsExcellent organization of TLS tests using subtests for different configurations. The helper function properly uses
t.Helper()and receives the test context parameter, enabling accurate test failure reporting.
518-520: Clear TODO for future containerized cluster supportGood job documenting the future enhancement for containerized cluster testing when the testcontainers-go Redis module adds this capability.
73-162: Comprehensive container configuration with proper resource managementThe
newConfigFromContainerfunction is well-implemented with:
- Proper container cleanup using
testcontainers.CleanupContainer- Flexible configuration based on provided options
- Dynamic TLS setup with proper certificate paths
- Proper error handling throughout
This approach ensures tests run in isolated, reproducible environments while managing resources efficiently.
| connection := New(newConfigFromContainer(t)) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a cleanup for the connection in Test_Redis_NewFromConnection
The connection created in this test is never closed, which could lead to resource leaks during testing.
- connection := New(newConfigFromContainer(t))
+ connection := New(newConfigFromContainer(t))
+ defer connection.Close()📝 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.
| connection := New(newConfigFromContainer(t)) | |
| connection := New(newConfigFromContainer(t)) | |
| defer connection.Close() |
|
@gaby @ReneWerner87 after the latest release of testcontainers-go, v0.37.0, Redis comes with TLS support 💪 so this PR has been updated to include it. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
redis/redis_test.go (1)
601-605:⚠️ Potential issueAdd cleanup for the connection in Test_Redis_NewFromConnection
The connection created in this test is never closed, which could lead to resource leaks during testing.
- connection := New(newConfigFromContainer(t)) + connection := New(newConfigFromContainer(t)) + defer connection.Close()
🧹 Nitpick comments (4)
redis/redis_test.go (4)
518-520: Consider using a Skip mechanism for the cluster testThe TODO comment is helpful, but this test might fail if the user doesn't have a local Redis cluster running on the specified ports. Consider adding a t.Skip() mechanism until containerized cluster support is available.
func Test_Redis_Cluster(t *testing.T) { // TODO: Replace with containerized cluster when testcontainers-go Redis module supports clustering + // Skip this test if the local Redis cluster isn't running + conn, err := net.DialTimeout("tcp", "localhost:7000", 1*time.Second) + if err != nil { + t.Skip("Redis cluster not available, skipping test") + } + conn.Close() + testStoreUniversal := New(Config{
4-14: Consider importing packages in alphabetical orderWhile not critical, many Go projects follow the convention of organizing imports in alphabetical order, with the standard library packages first, followed by external packages.
import ( "context" "os" "strings" "testing" "time" - "github.com/stretchr/testify/require" - "github.com/testcontainers/testcontainers-go" "github.com/testcontainers/testcontainers-go/modules/redis" + + "github.com/stretchr/testify/require" )
89-92: Consider adding a comment explaining why an environment variable override is neededAdding a brief comment about why someone might want to override the Redis image would be helpful for maintainers.
img := redisImage + // Allow overriding the Redis image via environment variable (e.g., for testing with different versions) if imgFromEnv := os.Getenv(redisImageEnvVar); imgFromEnv != "" { img = imgFromEnv }
104-123: Document why TLS port 6379 is used instead of the standard portThe TLS configuration is using port 6379 for TLS while disabling the standard port (--port 0). Adding a comment explaining this decision would help future maintainers understand the rationale.
if settings.tls { tcOpts = append(tcOpts, redis.WithTLS()) + // Disable standard Redis port and use only TLS port to ensure all connections use TLS // Use Redis module's TLS options, but for the MTLS case, disable the auth-clients flag cmds := []string{ "--port", "0", "--tls-port", "6379",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
redis/redis_test.go(16 hunks)
🔇 Additional comments (2)
redis/redis_test.go (2)
73-162: Great implementation of containerized Redis tests with TLS support!The
newConfigFromContainerfunction is well-structured and handles all the necessary configuration for Redis test containers, including TLS support. The cleanup is properly registered withtestcontainers.CleanupContainer(t, c)before checking for errors, which is good practice.I particularly like how you've implemented the flexible TLS configuration in lines 114-119, addressing the previous comment about duplicate flags.
347-389: Well-structured parameterized tests for TLS configurations!Great job refactoring the TLS tests to use a parameterized approach that covers all combinations of secure URL and mutual TLS settings. You've properly addressed the previous review comment by passing the testing.T parameter to the test function.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for testing the Redis module using testcontainers-go, including a proof-of-concept implementation for TLS support.
- Updated the test workflow to dynamically configure Redis containers using testcontainers-go.
- Revised Redis version specifications in the workflow matrix and consolidated test environment variable configurations.
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/workflows/test-redis.yml | Updated Redis image version matrix and environment variable setup to align with testcontainers-go usage and TLS PoC. |
| .github/workflows/benchmark.yml | Removed manual Redis setup steps and hardcoded Redis image version for benchmarks. |
Files not reviewed (1)
- redis/go.mod: Language not supported
Comments suppressed due to low confidence (2)
.github/workflows/test-redis.yml:21
- [nitpick] Confirm that changing the Redis version tag from '6.x' to '6' matches the expected Docker image tags to avoid potential version mismatches.
+ - '6'
.github/workflows/test-redis.yml:22
- [nitpick] Confirm that switching the Redis version tag from '7.x' to '7' aligns with the official Docker image tags for Redis.
+ - '7'
|
@mdelapenya I think the |
What does this PR do?
It uses the tescontainers-go Redis module to enable local testing of the Redis module.
I intentionally not added the cluster support as the tc-go redis module does not support it.
Besides, this PR represents a PoC for adding TLS support for the Redis module in tc-go, so you'll see code to configure the container with on-the-fly TLS certs, using
mdelapenya/tlscert. My idea is, if this PR gets accepted, port the logic to the tc-go module, so everybody in the community gets benefit from the TLS support.Why is it important?
Enable local testing of the Redis module, serving as an example for the Ruedis and Valkey modules
Summary by CodeRabbit