feat(coherence|memcached): use testcontainers-go in tests #1724
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 (2)
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 WalkthroughThe test suites for both the Coherence and Memcache storage packages were refactored to utilize dynamically created, containerized instances managed by the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Testcontainers as testcontainers-go
participant Container as Storage Container
participant Store as Storage Instance
Test->>Testcontainers: Launch container (Coherence/Memcache)
Testcontainers->>Container: Start and wait for readiness
Container-->>Testcontainers: Ready with host/port
Testcontainers-->>Test: Return host/port
Test->>Store: Create Storage instance with container config
Store-->>Test: Storage ready
Test->>Store: Run test operations
Test->>Store: Close storage (cleanup)
Test->>Testcontainers: Terminate container
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🔭 Outside diff range comments (1)
memcache/memcache_test.go (1)
108-115: 🛠️ Refactor suggestion
Test_Memcache_Set_Expirationnever asserts the key expiryThe test sleeps for 1 s but does not verify that the value actually expired,
so it can pass even if expiration is broken.time.Sleep(1100 * time.Millisecond) + +// verify the item vanished after the TTL +result, err := testStore.Get(key) +require.NoError(t, err) +require.Zero(t, len(result), "expected key to be expired after TTL")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.github/workflows/benchmark.ymlis excluded by!**/*.yml.github/workflows/test-memcache.ymlis excluded by!**/*.ymlcoherence/go.modis excluded by!**/*.modcoherence/go.sumis excluded by!**/*.sum,!**/*.summemcache/go.modis excluded by!**/*.modmemcache/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (2)
coherence/coherence_test.go(14 hunks)memcache/memcache_test.go(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: compare (coherence)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
🔇 Additional comments (2)
memcache/memcache_test.go (2)
38-45: 🛠️ Refactor suggestionCall
require.NoErrorbefore using the container reference
CleanupContainerregisters at.Cleanupcallback that dereferencesc.
Iferris non-nil,ccould benil, producing a panic inside the cleanup.
Swap the two lines to guarantee thatcis valid before it is captured:- testcontainers.CleanupContainer(t, c) - require.NoError(t, err) + 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.
31-38:❌ Incorrect review comment
Check the
testcontainersAPI –Runmight not exist (compile-time risk)The call sequence assumes a helper named
testcontainers.Run.
In current testcontainers-go releases the idiomatic entry points are either:testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{ … })or
testcontainers.RunContainer(ctx, …)
Run(alone) does not exist in most versions and will fail to compile.
Please double-check the library version you vendored and switch to the corresponding helper to avoid a build break.Example fix (for the modern builder API):
- c, err := testcontainers.Run(ctx, - img, - testcontainers.WithExposedPorts(memcachedPort), - testcontainers.WithWaitStrategy( - wait.ForListeningPort(memcachedPort), - ), - ) + c, err := testcontainers.RunContainer(ctx, + testcontainers.WithImage(img), + testcontainers.WithExposedPorts(memcachedPort), + testcontainers.WithWaitStrategy(wait.ForListeningPort(memcachedPort)), + )
🌐 Web query:
Does the Go testcontainers-go library export a function named Run in its latest releases?💡 Result:
Based on the search results, yes, testcontainers-go does export a
Runfunction in its latest releases. TheRunfunction serves as a convenience function that creates a new container and starts it[5]. It's designed to be the main entrypoint for modules in the testcontainers-go ecosystem.Function Signature and Usage
The
Runfunction typically follows this signature:func Run(ctx context.Context, img string, opts ...testcontainers.ContainerCustomizer) (*Container, error)This function accepts three parameters:
- A Go context (
context.Context)- The Docker image to use as a string
- A variadic argument for container customization options[9]
Status in Different Modules
The
Runfunction is being standardized across the testcontainers-go ecosystem. According to the documentation, it's becoming the preferred entrypoint for module initialization, while the previously usedStartContainerfunction is being deprecated[2].For some GCloud-related modules (BigQuery, BigTable, Datastore, Firestore), the documentation indicates that the
Runfunction is "Not available until the next release of testcontainers-go main"[9], suggesting that it's being actively implemented across all modules.Implementation Details
When implementing modules for testcontainers-go, developers are instructed to:
- Make sure a public
Containertype exists that embeds thetestcontainers.Containertype- Ensure a public
Runfunction exists as the entrypoint to the module- Define container options leveraging the
testcontainers.ContainerCustomizerinterface[2]The
Runfunction initializes the container request with default values, processes any customization options, and returns the configured container instance along with any potential errors[2].Citations:
- 1: https://github.com/testcontainers/testcontainers-go/releases
- 2: https://golang.testcontainers.org/modules/
- 3: https://github.com/testcontainers/testcontainers-go
- 4: https://golang.testcontainers.org/quickstart/
- 5: https://pkg.go.dev/github.com/testcontainers/testcontainers-go
- 6: https://www.atomicjar.com/2023/10/simplify-testing-golang-apps-with-testcontainers-go/
- 7: https://www.docker.com/blog/tag/go/feed/
- 8: https://www.docker.com/blog/local-development-of-go-applications-with-testcontainers/
- 9: https://golang.testcontainers.org/modules/gcloud/
- 10: https://pkg.go.dev/github.com/ikolomiyets/testcontainers-go
No changes needed to
testcontainers.RunusageThe
testcontainers-golibrary does export aRun(ctx, img string, opts …ContainerCustomizer)convenience function as its primary entry point. Your existing call totestcontainers.Runis valid and will compile against current releases—no update toRunContainerorGenericContaineris required.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull Request Overview
This PR integrates testcontainers-go into the test suites for both Memcached and Coherence, improving test isolation and reliability.
- Refactored test helper functions to launch containerized instances for tests.
- Updated CI/CD workflows with new environment variables and Go versions.
- Enhanced test configuration for both Memcached and Coherence.
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| memcache/memcache_test.go | Introduced a newTestStore function using testcontainers-go to launch and cleanup a container. |
| coherence/coherence_test.go | Refactored test configuration and store setup to use testcontainers-go for containerized tests. |
| .github/workflows/test-memcache.yml | Updated Go version matrix and set TEST_MEMCACHED_IMAGE environment variable for consistency. |
| .github/workflows/test-coherence.yml | Adjusted environment variables to set TEST_COHERENCE_IMAGE and streamlined container usage. |
| .github/workflows/benchmark.yml | Added environment variables for TEST_COHERENCE_IMAGE and TEST_MEMCACHED_IMAGE for benchmarks. |
Files not reviewed (2)
- coherence/go.mod: Language not supported
- memcache/go.mod: Language not supported
|
can you take a look I restarted the bench, maybe it was flakyless |
Summary by CodeRabbit