Repository navigation
test: add connPool, rttTracker, delegationCache tests and fix cache benchmark - #5
Conversation
📝 WalkthroughWalkthroughAdds comprehensive unit test coverage for resolver package components including cache resolution benchmarking, connection pool management, delegation cache operations, and RTT tracking behavior. Modifies existing cache benchmark to use real cache store infrastructure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![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 (3)
internal/resolver/conn_pool_test.go (2)
77-103:TestConnPoolIdleExpirydoesn't verify the expired connection was actually pruned.The test only checks
require.NotNil(t, conn2)but never confirmsconn2is a different connection fromconn1. Ifprunesilently fails (e.g., the internalconns[0].lastUsedmanipulation had no effect), the test would still pass becauseGetwould return the old connection. Add an address-inequality check, or inspect pool length before and after the forced prune to confirm an entry was actually evicted.🔍 Suggested strengthening
conn2, release2, err := p.get(ctx) require.NoError(t, err) require.NotNil(t, conn2) +// Confirm a fresh connection was created, not the expired one +require.NotEqual(t, conn1.LocalAddr().String(), conn2.LocalAddr().String(), + "expected pruned connection to be replaced with a fresh one") release2(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/resolver/conn_pool_test.go` around lines 77 - 103, Update TestConnPoolIdleExpiry to assert the expired connection was actually evicted: after obtaining conn1 from p.get and forcing the prune (by setting p.lastPrune = time.Time{} and manipulating p.conns[0].lastUsed), capture an identity for conn1 (e.g., the pointer value or a unique field), then call p.get again and assert that conn2's identity is different from conn1's; alternatively, inspect p.conns length before and after forcing prune to confirm an entry was removed. Reference TestConnPoolIdleExpiry, p.conns, lastUsed, ConnPoolIdleTimeout, get and prune when adding the assertion.
50-75: Acquiring 128 real network connections in a unit test is resource-intensive.
ConnPoolMaxConns = 128means this test opens 129 real UDP sockets. In constrained CI environments (low fd limits, shared sandbox) this may cause fd exhaustion or measurable slowdown. Consider either mocking the dial function or usingt.Parallel()awareness so this test doesn't run alongside other heavy tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/resolver/conn_pool_test.go` around lines 50 - 75, TestConnPoolMaxConns opens 129 real UDP sockets which can exhaust fds in CI; instead inject or override the dialer used by newConnPool to return a lightweight mock connection (implementing the minimal methods used by p.get/release) or add a constructor newConnPoolWithDial(dialFunc) so the test can pass a fake dial that returns in-memory/net.Pipe connections; update TestConnPoolMaxConns to use that mocked dial (or reduce the loop count for unit tests) and optionally mark the test with t.Parallel() if safe. Ensure you change references to newConnPool and p.get in the test to use the injected dial so no real sockets are created.internal/resolver/delegation_cache_test.go (1)
19-33:TestDelegationCacheTTLClampingdoesn't actually verify clamping behavior.Both sub-cases call
Getimmediately afterSet, so the assertions pass regardless of whether TTL clamping is applied:
- The 48h → 24h case: an un-clamped 48h TTL wouldn't have expired either, so
ok==truetells you nothing.- The 1s → 5s case: the test returns within 1s, so even a truly 1s TTL is still valid when
Getis called.To actually exercise clamping, inspect the internal expiry field after
Set, or use a fake/controllable clock. For the lower-bound case specifically, a time-manipulation approach like advancing a synthetic clock past 1s (but before 5s) would give a real signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/resolver/delegation_cache_test.go` around lines 19 - 33, TestDelegationCacheTTLClamping currently calls DelegationCache.Set then Get immediately so it never observes clamping; modify the test to either (A) expose or inspect the internal expiry timestamp set by newDelegationCache()/Set (e.g., verify that the stored expiry for "com." is now time.Now()+24h and for "net." is time.Now()+5s) or (B) inject/initialize the cache with a controllable clock and advance it: after setting "net." with a 1s TTL advance the fake clock to >1s but <5s and assert Get("net.") still returns true, and for "com." assert the expiry corresponds to 24h not 48h. Target symbols: TestDelegationCacheTTLClamping, newDelegationCache, (DelegationCache).Set, (DelegationCache).Get and the internal expiry field or clock used by the cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/resolver/conn_pool_test.go`:
- Around line 77-103: Update TestConnPoolIdleExpiry to assert the expired
connection was actually evicted: after obtaining conn1 from p.get and forcing
the prune (by setting p.lastPrune = time.Time{} and manipulating
p.conns[0].lastUsed), capture an identity for conn1 (e.g., the pointer value or
a unique field), then call p.get again and assert that conn2's identity is
different from conn1's; alternatively, inspect p.conns length before and after
forcing prune to confirm an entry was removed. Reference TestConnPoolIdleExpiry,
p.conns, lastUsed, ConnPoolIdleTimeout, get and prune when adding the assertion.
- Around line 50-75: TestConnPoolMaxConns opens 129 real UDP sockets which can
exhaust fds in CI; instead inject or override the dialer used by newConnPool to
return a lightweight mock connection (implementing the minimal methods used by
p.get/release) or add a constructor newConnPoolWithDial(dialFunc) so the test
can pass a fake dial that returns in-memory/net.Pipe connections; update
TestConnPoolMaxConns to use that mocked dial (or reduce the loop count for unit
tests) and optionally mark the test with t.Parallel() if safe. Ensure you change
references to newConnPool and p.get in the test to use the injected dial so no
real sockets are created.
In `@internal/resolver/delegation_cache_test.go`:
- Around line 19-33: TestDelegationCacheTTLClamping currently calls
DelegationCache.Set then Get immediately so it never observes clamping; modify
the test to either (A) expose or inspect the internal expiry timestamp set by
newDelegationCache()/Set (e.g., verify that the stored expiry for "com." is now
time.Now()+24h and for "net." is time.Now()+5s) or (B) inject/initialize the
cache with a controllable clock and advance it: after setting "net." with a 1s
TTL advance the fake clock to >1s but <5s and assert Get("net.") still returns
true, and for "com." assert the expiry corresponds to 24h not 48h. Target
symbols: TestDelegationCacheTTLClamping, newDelegationCache,
(DelegationCache).Set, (DelegationCache).Get and the internal expiry field or
clock used by the cache.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/resolver/cache_bench_test.gointernal/resolver/conn_pool_test.gointernal/resolver/delegation_cache_test.gointernal/resolver/rtt_tracker_test.go
| func TestConnPoolBadRelease(t *testing.T) { | ||
| p := newConnPool() | ||
| ctx := context.Background() | ||
|
|
||
| conn1, release1, err := p.get(ctx) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, conn1) | ||
| localAddr1 := conn1.LocalAddr().String() | ||
|
|
||
| // Release as bad - should close and remove from pool | ||
| release1(true) | ||
|
|
||
| // Next get should create a new connection (different local addr) | ||
| conn2, release2, err := p.get(ctx) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, conn2) | ||
| require.NotEqual(t, localAddr1, conn2.LocalAddr().String()) | ||
| release2(false) | ||
| } |
There was a problem hiding this comment.
require.NotEqual(localAddr1, conn2.LocalAddr()) is a potentially flaky assertion.
After a bad release the pool dials a new UDP socket, but the OS may reassign the same ephemeral port if the kernel's port-reuse window kicks in (or the process exhausts the port space in a tight loop). This makes the assertion non-deterministic. A more robust approach is to assert the pool's internal state (e.g., the old entry is absent) rather than relying on an OS-level side effect.
1 participant
Summary by CodeRabbit