Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request transitions the Redis integration tests to use an ephemeral Redis container via testcontainers-go when REDIS_ADDRESS is not configured, removing the dependency on Memorystore Redis in the CI configuration. Feedback on the changes highlights a potential nil pointer dereference in setupRedisContainer where testcontainers.CleanupContainer is registered before checking if the container initialization failed.
| testcontainers.CleanupContainer(t, container) | ||
| if err != nil { | ||
| t.Fatalf("failed to start Redis container: %s", err) | ||
| } |
There was a problem hiding this comment.
Calling testcontainers.CleanupContainer(t, container) before checking if err != nil can cause a nil pointer dereference and panic during test cleanup if container startup fails. This is because CleanupContainer registers a cleanup function that attempts to terminate the container, which will be nil if GenericContainer returned an error. Moving the cleanup registration after the error check ensures that we only attempt to clean up successfully started containers.
| testcontainers.CleanupContainer(t, container) | |
| if err != nil { | |
| t.Fatalf("failed to start Redis container: %s", err) | |
| } | |
| if err != nil { | |
| t.Fatalf("failed to start Redis container: %s", err) | |
| } | |
| testcontainers.CleanupContainer(t, container) |
Description
Migrate Redis integration tests to testcontainers.
PR Checklist
CONTRIBUTING.md
bug/issue
before writing your code! That way we can discuss the change, evaluate
designs, and agree on the general idea
review
!if this involve a breaking change🛠️ Fixes #<issue_number_goes_here>