feat(etcd): use testcontainers-go for etcd tests #1721
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 (1)
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 changes update the etcd storage test suite to improve test isolation and reproducibility. The test setup now uses a helper function to spin up a temporary etcd cluster for each test using testcontainers-go, replacing the previous global test store and static initialization. The import of the etcd client library in the main code is also updated to use an explicit alias. No core logic or exported API changes are made to the storage implementation itself; all functional changes are limited to the test setup and teardown. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant TestStore as newTestStore
participant TestContainers as testcontainers-go
participant EtcdCluster as Ephemeral Etcd Cluster
participant Storage as Storage Instance
Test->>TestStore: Call newTestStore(t)
TestStore->>TestContainers: Start ephemeral etcd cluster
TestContainers->>EtcdCluster: Spin up etcd container(s)
TestStore->>EtcdCluster: Get client endpoint
TestStore->>Storage: Create Storage with endpoint
TestStore-->>Test: Return Storage instance
Test->>Storage: Run test logic
Test->>Storage: Defer store.Close()
Test->>TestContainers: Container cleanup on test completion
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: 1
🧹 Nitpick comments (2)
etcd/etcd_test.go (2)
80-88: Speed up the TTL-expiry test to keep the suite snappySleeping six seconds per run makes the test suite noticeably slower (especially in benchmarks where it may run multiple times). etcd honours 1-second leases; shaving the values keeps intent while reducing wall-clock time.
-err := testStore.Set("test", []byte("fiber_test_value"), 3*time.Second) +err := testStore.Set("test", []byte("fiber_test_value"), 1*time.Second) - time.Sleep(6 * time.Second) + time.Sleep(2 * time.Second)
44-56: Enablet.Parallel()to unlock concurrent execution (optional)All tests now spin up isolated containers, so they are safe to run in parallel. Adding
t.Parallel()at the top of each test accelerates the suite on multi-core CI runners. Keep an eye on resource limits, though, as each test will start its own etcd cluster.
📜 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-etcd.ymlis excluded by!**/*.ymletcd/go.modis excluded by!**/*.modetcd/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (2)
etcd/etcd.go(1 hunks)etcd/etcd_test.go(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
etcd/etcd_test.go (2)
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-07-01T15:48:53.094Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
🧬 Code Graph Analysis (1)
etcd/etcd_test.go (1)
etcd/etcd.go (2)
Storage(10-12)New(14-33)
🔇 Additional comments (2)
etcd/etcd.go (1)
7-7: Explicit alias is a welcome readability boostUsing a package alias (
clientv3) makes downstream references clearer and avoids potential name clashes. No further action required.etcd/etcd_test.go (1)
32-34:⚠️ Potential issueCall
require.NoErrorbefore scheduling cleanup to avoid a nil-pointer panicIf
etcd.Runfails,cisnil. Passing that nil reference intotestcontainers.CleanupContainermay dereference a nil pointer inside the helper. Swap the two calls:-c, err := etcd.Run(ctx, img, etcd.WithNodes("etcd-1", "etcd-2"), etcd.WithClusterToken("test-cluster")) -testcontainers.CleanupContainer(t, c) -require.NoError(t, err) +c, err := etcd.Run(ctx, img, etcd.WithNodes("etcd-1", "etcd-2"), etcd.WithClusterToken("test-cluster")) +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.
| ctx := context.Background() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a timeout-bound context to prevent hanging tests
context.Background() has an unlimited lifetime. If the container pull or startup hangs (for example, due to a flaky network or Docker daemon issue), the test will block indefinitely. Consider adding a reasonable timeout:
-ctx := context.Background()
+ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+defer cancel()This keeps the test suite responsive in CI environments.
📝 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.
| ctx := context.Background() | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| defer cancel() |
🤖 Prompt for AI Agents (early access)
In etcd/etcd_test.go around lines 29 to 30, replace the use of context.Background() with a context that has a timeout to prevent the test from hanging indefinitely. Create a context with a timeout using context.WithTimeout, specifying a reasonable duration (e.g., a few seconds), and ensure to defer the cancellation function to release resources. This change will keep the test suite responsive by automatically cancelling the context if the operation takes too long.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the etcd tests to use Testcontainers Go, enabling the creation of ephemeral 2-node etcd clusters for improved test isolation and reliability while also updating related workflows.
- Refactored test initialization by replacing a global test resource with a helper function that creates new etcd clusters for each test run.
- Updated the docker image references and Go versions in CI workflows to support the new testing approach.
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| etcd/etcd_test.go | Introduced newTestStore to spin up temporary etcd clusters using Testcontainers Go. |
| etcd/etcd.go | Updated import alias for the etcd client package. |
| .github/workflows/test-etcd.yml | Updated image references and Go versions for etcd testing. |
| .github/workflows/benchmark.yml | Removed deprecated docker-based etcd setup and added environment variable settings. |
Files not reviewed (1)
- etcd/go.mod: Language not supported
|
I see the benchmarks do fail with a massive degradation 🤔 Do you think it's because of the etcd cluster? I verified the containers are started before the the benchmarks code, so they should not interfere, right? |
|
@mdelapenya @ReneWerner87 The bitnami image was faster for some reason, and it also supports semver. |
|
Ok then let's take this |
|
I'll take a look, as I'm not sure if the testcontainers module supports the image. I'm on PTO until Monday, will check it then 🙏 |
|
@mdelapenya My guess is that a single etcd is more performant than a cluster in this case. The cluster is adding network latency, replication, etc |
|
We have just merged a PR fixing 1-node clusters in etcd: testcontainers/testcontainers-go#3149 I think we can create a tc-go release soon to address that. I'm putting this PR as draft until then |
What does this PR do?
It uses Testcontainers Go's etcd module for testing the etcd Storage module.
It uses the latest release of the official etcd Docker image (there is no latest tag in the repo), which is replacing the
bitnami/etcd:latestthat was used before these changes.The test store that is created spins up an etcd cluster of two nodes.
Why is it important?
Enable local testing of the module, also making the CI reproducible.
Summary by CodeRabbit