Repository navigation
test: Fix flaky MultiDbConnectionProviderTest caused by shared test key pollution - #4644
Conversation
The test read key "foo" on the shared standalone0/standalone1 endpoints without cleanup. Earlier suites (e.g. TupleSortedSetTest) leave "foo" holding a non-string type, so GET sporadically failed with WRONGTYPE in CI. Use a unique per-test key (test name + short random id) instead.
Generalizes the per-test key helper from MultiDbConnectionProviderTest into a reusable utility so any test can avoid key collisions on the shared Redis endpoints. Keys are namespaced by ClassName.methodName, auto-registered, and deletable via cleanup() with a per-slot DEL fallback on cluster CROSSSLOT errors.
A registry created in @BeforeAll now derives a class-scoped identifier (ClassName, no method suffix), so keys shared across a class's tests survive between tests and are cleaned once in @afterall. Scope is a property of where the registry is created and cleaned; the two scopes cannot collide since method keys always contain ".methodName".
bKey(pattern) mirrors key(pattern) for byte[] command overloads, encoding via SafeEncoder. Keys are now stored internally as bytes (ByteBuffer for content equality), so string- and binary-created keys share one registry entry and cleanup stays a single binary DEL. register(byte[]) stores a copy so caller mutation cannot break cleanup.
flushAll() ran only in @beforeeach, so the last test of each subclass left its keys on the shared endpoints, breaking later tests that read well-known key names (WRONGTYPE). Flush in @AfterEach as well so no subclass leaks keys to the rest of the suite.
Test Results 206 files ± 0 206 suites ±0 10m 24s ⏱️ +31s Results for commit 4cb571b. ± Comparison against base commit 7aef6e8. This pull request removes 2 and adds 14 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
The formatter plugin validates only explicitly listed files; TestKeyRegistry was missing, failing formatter:validate in CI.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49eea46ff6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
TestKeyRegistry.cleanup() now accepts KeyBinaryCommands so any Jedis client type can perform cleanup. MultiDbConnectionProviderTest holds control Jedis clients for both endpoints (opened in @BeforeAll, released in @afterall) and deletes each test's keys in @AfterEach.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1d446fb01
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a1d446f. Configure here.
- Flush on a fresh connection, best-effort: - Fall back to per-slot DEL on JedisClusterOperationException - Null-guard @afterall control-client release
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cb571b485
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
What
Fix sporadic WRONGTYPE failures in
MultiDbConnectionProviderTestcaused by test-key pollution on the shared Redis endpoints, and add a reusableTestKeyRegistryutility to prevent this class of flakiness.Why
MultiDbConnectionProviderTestreads the well-known keyfooon the sharedstandalone0/standalone1endpoints.JedisCommandsTestBasesubclasses flush only in@BeforeEach, so the last test of each class leaks its keys — e.g.TupleSortedSetTestleavesfooas a zset, and a laterGET foofails withWRONGTYPEdepending on class execution order.Fixes #4643
Changes
TestKeyRegistry(new test utility): generates unique, human-readable keys namespaced byClassName.methodName; every key is auto-registered and deletable viacleanup()with a per-slotDELfallback on clusterCROSSSLOTerrors.@BeforeAllderives aClassNameidentifier, so class-level keys survive between tests and are cleaned once in@AfterAll.bKey(pattern)mirrorskey(pattern)viaSafeEncoder; keys are stored internally as bytes so string- and binary-created keys share one entry and cleanup is a single binaryDEL.MultiDbConnectionProviderTest: migrated the three tests using"foo"to registry-generated per-test keys.JedisCommandsTestBase: alsoflushAll()in@AfterEach, so no subclass leaks keys to the rest of the suite.Verified by planting
fooas a zset on both endpoints: the unmodified test reproduces the exact CI failure; with this change all tests pass.Note
Low Risk
Test-only changes (new utility, integration test keys, and teardown flush); no production client behavior is modified.
Overview
Addresses sporadic WRONGTYPE failures when integration tests reuse shared Redis endpoints and collide on keys like
"foo".Introduces
TestKeyRegistry, a test helper that builds per-test namespaced keys (ClassName.methodName:…), tracks them, and deletes them viacleanup()(with per-slotDELwhen cluster clients reject cross-slot multi-key deletes).MultiDbConnectionProviderTestswitches its failover scenarios from"foo"to registry keys and tears them down through dedicated controlJedisclients on both standalones.JedisCommandsTestBasealso runsflushAll()in@AfterEach(errors ignored) so subclasses are less likely to leave data on the shared endpoint after a class finishes.Reviewed by Cursor Bugbot for commit 4cb571b. Bugbot is set up for automated code reviews on this repo. Configure here.