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 skippedDraft detected. Please check the settings in the CodeRabbit UI or the 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
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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.
etcd/etcd_test.go
Outdated
| 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 |
|
Currently blocked from the PR and release of the testcontainer or ? |
|
@mdelapenya can we continue with this? |
|
I need to release tc-go with the fix for etcd 🙏 I'll go back to this once released, which will happen asap Thanks for the ping @ReneWerner87 🙇 |
* main: (149 commits) chore(deps): bump github.com/surrealdb/surrealdb.go in /surrealdb chore(deps): bump the utils-modules group across 3 directories with 1 update chore(deps): bump go.etcd.io/etcd/client/v3 from 3.6.1 to 3.6.2 in /etcd chore(deps): bump the utils-modules group across 3 directories with 1 update chore(deps): bump lewagon/wait-on-check-action from 1.3.4 to 1.4.0 fix: use new bbolt bucket error chore: expand dependabot groups chore(deps): bump the utils-modules group across 3 directories with 1 update chore(deps): bump go.etcd.io/etcd/client/v3 in /etcd chore(deps): bump github.com/microsoft/go-mssqldb in /mssql chore(deps): bump go.etcd.io/bbolt from 1.3.9 to 1.4.2 in /bbolt Update README.md Update dependabot.yml Update dependabot.yml Update dependabot.yml update release policy apply some ai reviews update nodejs version from 18 to 20 ERROR: Wrangler requires at least Node.js v20.0.0. You are using v18.20.8. Please update your version of Node.js. add dummy ctx to leveldb update docs ...
|
Testcontainers Go v0.38.0 was released yesterday, I'm working on this branch with single cluster support |
|
I'm not sure why there is so much overhead when running the benchmarks... Any idea? I'm pretty sure the numbers are higher for starting a container programatically than compare simply calling docker run in the same shell. But we did not see that in other modules, did't we? |
|
I've run the benchmarks in the main branch, running And next, did the same for this branch, but multiple times: ➜ etcd (etcd-tc-go) ✗ set -o pipefail;go test ./... -timeout 900s -benchmem -run=^$ -bench . | tee -a output.txt
goos: darwin
goarch: arm64
pkg: github.com/gofiber/storage/etcd/v2
cpu: Apple M1 Pro
Benchmark_Etcd_Set-8 4 303310073 ns/op 36172 B/op 344 allocs/op
Benchmark_Etcd_Get-8 38 31038329 ns/op 7851 B/op 135 allocs/op
Benchmark_Etcd_SetAndDelete-8 9 199249176 ns/op 31407 B/op 418 allocs/op
PASS
ok github.com/gofiber/storage/etcd/v2 44.388s
➜ etcd (etcd-tc-go) ✗ set -o pipefail;go test ./... -timeout 900s -benchmem -run=^$ -bench . | tee -a output.txt
goos: darwin
goarch: arm64
pkg: github.com/gofiber/storage/etcd/v2
cpu: Apple M1 Pro
Benchmark_Etcd_Set-8 3 428007931 ns/op 71349 B/op 472 allocs/op
Benchmark_Etcd_Get-8 37 31356338 ns/op 7855 B/op 135 allocs/op
Benchmark_Etcd_SetAndDelete-8 1 1068802084 ns/op 122144 B/op 905 allocs/op
PASS
ok github.com/gofiber/storage/etcd/v2 21.665s
➜ etcd (etcd-tc-go) ✗ set -o pipefail;go test ./... -timeout 900s -benchmem -run=^$ -bench . | tee -a output.txt
goos: darwin
goarch: arm64
pkg: github.com/gofiber/storage/etcd/v2
cpu: Apple M1 Pro
Benchmark_Etcd_Set-8 3 383360347 ns/op 43090 B/op 372 allocs/op
Benchmark_Etcd_Get-8 37 31712941 ns/op 7858 B/op 136 allocs/op
Benchmark_Etcd_SetAndDelete-8 3 461405903 ns/op 78384 B/op 598 allocs/op
PASS
ok github.com/gofiber/storage/etcd/v2 24.615sI decided to go with a singleton container for benchmarks only. Added comments in the test code for that. PLMK if that's enough as a warning for other contributors when working on benchmarks of the etcd module. 🙏 Results now are: |
|
They fail on GH:
I wonder why that huge difference 🤔 Could it be possible that the stored benchmarks are not accurate? The above table is obtained from the GH action for comparing prev Vs current. I'm building the same table but with my local experiment with the main branch (previous comment's values)
Is it possible to get the baseline benchmarks and repeat them again to verify their accuracy? |
|
BTW I think there is something wrong with the storage benchmarks: https://gofiber.github.io/storage/benchmarks/ Last udpate is from Aug '24. Weird 🤔 |
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