chore: overhaul the test suite - #230
Merged
Merged
Conversation
Replace the per-test endure boilerplate with a Start helper that boots the container, waits for the rpc listener and tears down through t.Cleanup. The five tests spent about 28 seconds in fixed sleeps, 22 of them waiting for TTLs to lapse inside one function. Split them by theme and poll for expiry instead. Cover what the old suite did not: MGet returning the stored bytes, TTL reporting a remaining lifetime for a key that has one, a key without a TTL surviving an expiry, Delete touching only the named key, and an unknown storage name being rejected. Drop tests/env/docker-compose-redis.yaml. It served redis on 6379 and 6378, no config references those ports, and nothing in CI starts it. The suite talks to the redis-server CI installs on 16379 and 16380. Add root unit tests for Init, Name, Collects, KvFromConfig and the MetricsCollector nil guard. Ignore the certificates get-test-certs.sh generates so a local run cannot commit them.
Widen coverpkg to the whole module so the kv driver is measured, wait for redis to answer instead of sleeping 5s, drop -failfast, and fail the codecov job when the merged summary maps to no plugin source.
There was a problem hiding this comment.
Pull request overview
This PR (Wave 2) modernizes and expands the Redis KV plugin test suite and aligns CI coverage reporting with the code actually exercised by the driver, while removing unused legacy test infrastructure.
Changes:
- Replaces the old monolithic e2e test with a larger, faster suite that uses polling (
require.Eventually) instead of fixed sleeps, and adds new behavioral coverage (MGet bytes, TTL semantics, key expiry behavior, Delete isolation, unknown storage rejection, TLS round trip). - Adds reusable test harness helpers for starting an Endure container and probing readiness, plus new root-module unit tests for the Redis plugin’s init/config/metrics behaviors.
- Updates CI to (a) generate and upload combined coverage across both the root module and the tests module, (b) remove fixed sleeps in favor of a retry loop, and (c) enforce a minimal “coverage maps to sources” guard; also removes unused docker-compose and gitignores generated cert artifacts.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tests/tls_test.go |
Adds an end-to-end TLS KV round-trip test to exercise TLS config/wiring. |
tests/kv_test.go |
Introduces the main RPC-based KV e2e test suite with polling-based expiry checks and added coverage. |
tests/config_test.go |
Adds e2e tests for global config fallback, missing config failure, and metrics export validation. |
tests/helpers/rr.go |
Adds a helper harness for booting/stopping an Endure container with readiness probing and RPC client creation. |
tests/storage_plugin_test.go |
Removes the legacy long-sleeping e2e test suite in favor of the new tests. |
tests/env/docker-compose-redis.yaml |
Deletes unused docker-compose infrastructure that wasn’t referenced by configs/CI. |
plugin_test.go |
Adds root-module unit tests for plugin init/name/collects/config error propagation and metrics collector guard. |
.github/workflows/linux.yml |
Updates CI to run root + tests-module coverage, widen -coverpkg, remove fixed sleeps, and add a coverage sanity guard. |
.gitignore |
Prevents accidental committing of generated TLS cert/key artifacts under tests/certs. |
go.mod / go.sum |
Adds testify (and related sums) required by new tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Wave 2 overhaul. No dependency bump: these tests are pure goridge RPC with no PHP worker, and
kv/v6 beta.6still depends onapi-go beta.13, which is wherekv/v2lives.Runtime. Five tests carrying seven fixed sleeps totalling about 28 seconds, 22 of which sat inside one function waiting for a 5s and a 10s TTL to lapse. Polling with
require.Eventuallyinstead: ~30s to ~3.1s, and twelve tests instead of five.New coverage. The old suite never checked that
kv.MGetreturned the stored bytes, thatkv.TTLreports a remaining lifetime, that a key without a TTL survives an expiry, thatDeletetouches only the named key, or that an unconfigured storage name is rejected. All covered now. The TLS test previously only proved the container booted; it now does a full Set/Has/MGet round trip over the TLS port.Dead infrastructure removed.
tests/env/docker-compose-redis.yamlserved redis on 6379 and 6378. No config references either port, and no workflow step ever started it — CI installs redis-server via apt and runs it on 16379/16380. Deleted.Root unit tests are new:
Init,Name,Collects,KvFromConfigincluding the decode-error path, and theMetricsCollectornil guard — asking for collectors before any storage exists must return nothing rather than a slice holding a nil collector, which would panic the prometheus registry.Also gitignored the certificates
get-test-certs.shwrites intotests/certs. They were untracked, so a local run could commit a private key by accident.CI.
-coverpkgwas scoped toredis/v6without/..., so thekvpackage holding the entire driver was never measured. Widened. Thesleep 5after starting redis is now a retry loop on the TLS ping. Dropped-failfast, added the coverage guard (347 blocks against a floor of 10).Local results: root unit 19.8%, e2e 68.8%, merged 69.8% against a current badge of 81%. The badge is higher than the merged figure because it was measured with the narrow coverpkg that excluded the driver package — this number covers more code, so it is the honest one.
Verified against a real redis on both the plain and TLS ports, stable across 4 consecutive shuffled runs.