test(cache): add comprehensive unit tests for cache functions#1434
Merged
jensneuse merged 5 commits intofeat/add-caching-supportfrom Mar 6, 2026
Merged
test(cache): add comprehensive unit tests for cache functions#1434jensneuse merged 5 commits intofeat/add-caching-supportfrom
jensneuse merged 5 commits intofeat/add-caching-supportfrom
Conversation
Add 48+ new unit tests covering critical cache functions that previously only had indirect E2E coverage: - validateItemHasRequiredData: 22 subtests for nullable/non-nullable combinations, nested objects, arrays, type mismatches - normalize/denormalize round-trip: 8 subtests with CacheArgs suffix handling and alias combinations - computeArgSuffix: 7 subtests for xxhash determinism and edge cases - mergeEntityFields: 6 subtests for field merging semantics - L2 error resilience: 3 subtests for error handling and graceful fallthrough - Mutation L2 skip: 1 subtest verifying mutations bypass L2 reads All tests pass with race detector enabled. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
jensneuse
requested review from
Noroth,
StarpTech,
devsergiy,
dkorittki and
ysmolski
as code owners
Contributor
📝 WalkthroughWalkthroughAdds L2 cache test helpers and error-resilience/mutation-bypass tests, greatly expands L1 cache normalization/denormalization/validation tests, and applies minor formatting fixes to GraphQL subscription handler tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/resolve/cache_load_test.go`:
- Around line 1981-1985: The struct ErrorLoaderCache embeds *FakeLoaderCache and
then declares regular fields getErr and setErr without a separating empty line;
update the struct definition for ErrorLoaderCache to add a blank line between
the embedded field (*FakeLoaderCache) and the subsequent regular fields (getErr,
setErr) so the embedded and non-embedded fields are separated per linter rules.
- Around line 2173-2181: The test pre-populates the fake cache with Key
"Product:prod-1" which doesn't match the real key format produced by
productCacheKeyTemplate, so the lookup misses for the wrong reason; update the
setup to build the cache key using productCacheKeyTemplate (or the same
key-generation helper used by the resolver) for id "prod-1" and use that string
as the CacheEntry.Key when calling NewFakeLoaderCache().Set with the corrupted
JSON value so the loader hits the entry and fails during JSON parsing rather
than due to a key mismatch.
- Around line 2216-2225: The test TestMutationSkipsL2Read is seeding the fake
cache with a key "Product:prod-1" that does not match the actual cache key
format produced by userCacheKeyTemplate (which is JSON like
{"__typename":"Product","key":{"id":"prod-1"}}), so the cached entry is never
found and the test doesn't validate the L2 bypass; fix by populating the cache
using the real key format — either call userCacheKeyTemplate (or the helper that
builds keys) to generate the cache key for the CacheEntry used in
NewFakeLoaderCache.Set, or construct the JSON key string exactly as
userCacheKeyTemplate produces for
{"__typename":"Product","key":{"id":"prod-1"}}, then keep the stale Value and
assert the code path that mutation operations skip L2 reads (i.e., that the
loader fetch occurs despite the stale cache entry).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21ec3c5c-b3df-44cf-8fcb-93ad4829e5cb
📒 Files selected for processing (2)
v2/pkg/engine/resolve/cache_load_test.gov2/pkg/engine/resolve/l1_cache_test.go
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
v2/pkg/engine/resolve/cache_load_test.go (2)
2224-2226:⚠️ Potential issue | 🟠 MajorUse the template-generated key in the mutation-bypass setup.
The stale entry is seeded under
Product:prod-1, but this fetch reads the JSON cache key rendered from the template. As written, the entity fetch happens because L2 misses outright, so the test does not prove that mutations bypass a matching L2 entry.Proposed fix
_ = cache.Set(t.Context(), []*CacheEntry{ - {Key: "Product:prod-1", Value: []byte(`{"__typename":"Product","id":"prod-1","name":"Old Name"}`)}, + {Key: `{"__typename":"Product","key":{"id":"prod-1"}}`, Value: []byte(`{"__typename":"Product","id":"prod-1","name":"Old Name"}`)}, }, 30*time.Second)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/resolve/cache_load_test.go` around lines 2224 - 2226, The seeded stale entry uses the literal key "Product:prod-1" but the fetch reads the JSON cache key rendered from the template, so change the cache.Set call to use the template-generated key (the same key the fetch path constructs) instead of the hardcoded "Product:prod-1"; i.e., compute/render the JSON cache key the test/fetch will use and pass that string as CacheEntry.Key in the cache.Set call so the mutation-bypass path exercises a matching L2 entry (keep the call to cache.Set and CacheEntry structure unchanged).
2180-2182:⚠️ Potential issue | 🟠 MajorSeed the corrupted entry under the real cache key.
This test currently inserts
Product:prod-1, but the fetch uses the JSON key format produced by the cache template. Right now the request falls through because of a lookup miss, not because the cached payload is invalid JSON, so the corrupted-entry path is not actually covered.Proposed fix
_ = cache.Set(t.Context(), []*CacheEntry{ - {Key: "Product:prod-1", Value: []byte(`{not valid json!!!}`)}, + {Key: `{"__typename":"Product","key":{"id":"prod-1"}}`, Value: []byte(`{not valid json!!!}`)}, }, 30*time.Second)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/engine/resolve/cache_load_test.go` around lines 2180 - 2182, The test is inserting the corrupted value under the literal key "Product:prod-1" so the fetch misses; change the cache.Set call to seed the corrupted bytes under the exact cache key the fetch uses (i.e., generate the key with the same cache template/helper the fetch path uses instead of the literal "Product:prod-1"), keeping the value as []byte(`{not valid json!!!}`) and the same TTL; update the CacheEntry Key to that generated key so the invalid-JSON path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/resolve/cache_load_test.go`:
- Around line 2055-2059: The test constructs a FetchInfo for the entity fetch
but hardcodes OperationType to ast.OperationTypeQuery; update the entity fetch
construction to use the helper/test-local operationType variable instead (i.e.,
set FetchInfo.OperationType = operationType). Locate the entity fetch that
creates a FetchInfo (symbol: FetchInfo) around where RootFields/ProvidesData are
set and replace the literal ast.OperationTypeQuery with the operationType used
by the test helper so mutation fixtures exercise mutation-path behavior
consistently.
---
Duplicate comments:
In `@v2/pkg/engine/resolve/cache_load_test.go`:
- Around line 2224-2226: The seeded stale entry uses the literal key
"Product:prod-1" but the fetch reads the JSON cache key rendered from the
template, so change the cache.Set call to use the template-generated key (the
same key the fetch path constructs) instead of the hardcoded "Product:prod-1";
i.e., compute/render the JSON cache key the test/fetch will use and pass that
string as CacheEntry.Key in the cache.Set call so the mutation-bypass path
exercises a matching L2 entry (keep the call to cache.Set and CacheEntry
structure unchanged).
- Around line 2180-2182: The test is inserting the corrupted value under the
literal key "Product:prod-1" so the fetch misses; change the cache.Set call to
seed the corrupted bytes under the exact cache key the fetch uses (i.e.,
generate the key with the same cache template/helper the fetch path uses instead
of the literal "Product:prod-1"), keeping the value as []byte(`{not valid
json!!!}`) and the same TTL; update the CacheEntry Key to that generated key so
the invalid-JSON path is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f753e37b-0b0e-49db-91e6-55d012be4667
📒 Files selected for processing (1)
v2/pkg/engine/resolve/cache_load_test.go
…ensneuse/cache-test-gaps
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ntity fetch Address CodeRabbit review feedback: - Use real cache key format (JSON with __typename and key) instead of "Product:prod-1" so tests validate actual cache hit/miss behavior - Propagate operationType to entity fetch FetchInfo so mutation tests exercise mutation-path behavior consistently - Add cache log assertion to verify corrupted entry is actually found Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/engine/resolve/cache_load_test.go`:
- Around line 1988-1999: The wrapper ErrorLoaderCache currently returns early
when e.getErr or e.setErr is set, which prevents FakeLoaderCache.log from being
updated and hides whether L2 was exercised; change ErrorLoaderCache.Get and
ErrorLoaderCache.Set to always invoke the underlying FakeLoaderCache.Get/Get or
Set (to allow it to record to FakeLoaderCache.log) and then, if e.getErr or
e.setErr is non-nil, return that injected error (i.e., call
FakeLoaderCache.Get/Set first to record the operation, capture its result, then
override the returned error with e.getErr/e.setErr before returning).
- Around line 2230-2279: The test must assert that no L2 read occurs for
mutations: replace or augment the fake cache to verify Get is not called (e.g.,
use a mock implementing LoaderCache or add a spy counter on NewFakeLoaderCache
and assert cache.Get call count == 0 after loader.LoadGraphQLResponseData).
Specifically, keep using NewFakeLoaderCache / cache.Set to pre-populate, then
after calling Loader.LoadGraphQLResponseData(ctx, response, resolvable) assert
the fake cache's Get was never invoked (or set an expectation of zero calls on a
mock LoaderCache) so the test fails if any L2 Get occurs during a mutation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fc874eea-d566-4165-a72b-72cafa780ba8
📒 Files selected for processing (1)
v2/pkg/engine/resolve/cache_load_test.go
Comment on lines
+1988
to
+1999
| func (e *ErrorLoaderCache) Get(ctx context.Context, keys []string) ([]*CacheEntry, error) { | ||
| if e.getErr != nil { | ||
| return nil, e.getErr | ||
| } | ||
| return e.FakeLoaderCache.Get(ctx, keys) | ||
| } | ||
|
|
||
| func (e *ErrorLoaderCache) Set(ctx context.Context, entries []*CacheEntry, ttl time.Duration) error { | ||
| if e.setErr != nil { | ||
| return e.setErr | ||
| } | ||
| return e.FakeLoaderCache.Set(ctx, entries, ttl) |
Contributor
There was a problem hiding this comment.
Record injected cache failures in the wrapper.
Get/Set return before updating FakeLoaderCache.log, so the resilience subtests cannot prove the L2 path was actually exercised. As written, they still pass if the loader skips L2 entirely.
Suggested change
func (e *ErrorLoaderCache) Get(ctx context.Context, keys []string) ([]*CacheEntry, error) {
if e.getErr != nil {
+ e.mu.Lock()
+ e.log = append(e.log, CacheLogEntry{
+ Operation: "get",
+ Keys: append([]string(nil), keys...),
+ Hits: make([]bool, len(keys)),
+ })
+ e.mu.Unlock()
return nil, e.getErr
}
return e.FakeLoaderCache.Get(ctx, keys)
}
func (e *ErrorLoaderCache) Set(ctx context.Context, entries []*CacheEntry, ttl time.Duration) error {
if e.setErr != nil {
+ keys := make([]string, 0, len(entries))
+ for _, entry := range entries {
+ if entry != nil {
+ keys = append(keys, entry.Key)
+ }
+ }
+ e.mu.Lock()
+ e.log = append(e.log, CacheLogEntry{
+ Operation: "set",
+ Keys: keys,
+ })
+ e.mu.Unlock()
return e.setErr
}
return e.FakeLoaderCache.Set(ctx, entries, ttl)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2/pkg/engine/resolve/cache_load_test.go` around lines 1988 - 1999, The
wrapper ErrorLoaderCache currently returns early when e.getErr or e.setErr is
set, which prevents FakeLoaderCache.log from being updated and hides whether L2
was exercised; change ErrorLoaderCache.Get and ErrorLoaderCache.Set to always
invoke the underlying FakeLoaderCache.Get/Get or Set (to allow it to record to
FakeLoaderCache.log) and then, if e.getErr or e.setErr is non-nil, return that
injected error (i.e., call FakeLoaderCache.Get/Set first to record the
operation, capture its result, then override the returned error with
e.getErr/e.setErr before returning).
Comment on lines
+2230
to
+2279
| cache := NewFakeLoaderCache() | ||
| // Pre-populate cache with stale data using the real key format | ||
| _ = cache.Set(t.Context(), []*CacheEntry{ | ||
| {Key: `{"__typename":"Product","key":{"id":"prod-1"}}`, Value: []byte(`{"__typename":"Product","id":"prod-1","name":"Old Name"}`)}, | ||
| }, 30*time.Second) | ||
|
|
||
| userCacheKeyTemplate := &EntityQueryCacheKeyTemplate{ | ||
| Keys: NewResolvableObjectVariable(&Object{ | ||
| Fields: []*Field{ | ||
| {Name: []byte("__typename"), Value: &String{Path: []string{"__typename"}}}, | ||
| {Name: []byte("id"), Value: &String{Path: []string{"id"}}}, | ||
| }, | ||
| }), | ||
| } | ||
| providesData := &Object{ | ||
| Fields: []*Field{ | ||
| {Name: []byte("name"), Value: &Scalar{}}, | ||
| }, | ||
| } | ||
|
|
||
| rootDS := NewMockDataSource(ctrl) | ||
| rootDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()). | ||
| DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) { | ||
| return []byte(`{"data":{"updateUser":{"__typename":"Product","id":"prod-1"}}}`), nil | ||
| }).Times(1) | ||
|
|
||
| entityDS := NewMockDataSource(ctrl) | ||
| entityDS.EXPECT().Load(gomock.Any(), gomock.Any(), gomock.Any()). | ||
| DoAndReturn(func(ctx context.Context, headers any, input []byte) ([]byte, error) { | ||
| return []byte(`{"data":{"_entities":[{"__typename":"Product","id":"prod-1","name":"New Name"}]}}`), nil | ||
| }).Times(1) // Must fetch fresh data despite cache having stale entry | ||
|
|
||
| response := buildProductEntityResponse(rootDS, entityDS, userCacheKeyTemplate, providesData, ast.OperationTypeMutation) | ||
|
|
||
| loader := &Loader{caches: map[string]LoaderCache{"default": cache}} | ||
| ctx := NewContext(t.Context()) | ||
| ctx.ExecutionOptions.DisableSubgraphRequestDeduplication = true | ||
| ctx.ExecutionOptions.Caching.EnableL1Cache = true | ||
| ctx.ExecutionOptions.Caching.EnableL2Cache = true | ||
|
|
||
| ar := arena.NewMonotonicArena(arena.WithMinBufferSize(1024)) | ||
| resolvable := NewResolvable(ar, ResolvableOptions{}) | ||
| err := resolvable.Init(ctx, nil, ast.OperationTypeMutation) | ||
| require.NoError(t, err) | ||
|
|
||
| err = loader.LoadGraphQLResponseData(ctx, response, resolvable) | ||
| require.NoError(t, err) | ||
|
|
||
| out := fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors) | ||
| assert.Equal(t, `{"data":{"updateUser":{"__typename":"Product","id":"prod-1","name":"New Name"}}}`, out, "mutation should fetch fresh data, not use cached stale data") |
Contributor
There was a problem hiding this comment.
Assert that mutations never perform an L2 read.
Right now this only proves the stale value was not served. If the loader still does an L2 Get before fetching, the test stays green and misses the regression this PR is meant to catch.
Suggested change
_ = cache.Set(t.Context(), []*CacheEntry{
{Key: `{"__typename":"Product","key":{"id":"prod-1"}}`, Value: []byte(`{"__typename":"Product","id":"prod-1","name":"Old Name"}`)},
}, 30*time.Second)
+ cache.ClearLog()
@@
out := fastjsonext.PrintGraphQLResponse(resolvable.data, resolvable.errors)
assert.Equal(t, `{"data":{"updateUser":{"__typename":"Product","id":"prod-1","name":"New Name"}}}`, out, "mutation should fetch fresh data, not use cached stale data")
+
+ for _, entry := range cache.GetLog() {
+ assert.NotEqual(t, "get", entry.Operation, "mutations should bypass L2 reads entirely")
+ }
})
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2/pkg/engine/resolve/cache_load_test.go` around lines 2230 - 2279, The test
must assert that no L2 read occurs for mutations: replace or augment the fake
cache to verify Get is not called (e.g., use a mock implementing LoaderCache or
add a spy counter on NewFakeLoaderCache and assert cache.Get call count == 0
after loader.LoadGraphQLResponseData). Specifically, keep using
NewFakeLoaderCache / cache.Set to pre-populate, then after calling
Loader.LoadGraphQLResponseData(ctx, response, resolvable) assert the fake
cache's Get was never invoked (or set an expectation of zero calls on a mock
LoaderCache) so the test fails if any L2 Get occurs during a mutation.
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
1 participant
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.
Add 48+ new unit tests covering critical cache functions (validateItemHasRequiredData, normalize/denormalize round-trip, computeArgSuffix, mergeEntityFields, L2 error resilience, mutation L2 skip) that previously only had indirect E2E coverage. Tests cover edge cases including nullable/non-nullable combinations, nested objects, arrays, type mismatches, CacheArgs suffix handling, xxhash determinism, field merging semantics, and error handling. All tests pass with race detector enabled.
Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit