Test expired KV entries in @fedify/cfworkers - #1021
Conversation
WorkersKvStore.get() applies expiry via a metadata.expires field independent of Workers KV's own TTL, but the miniflare-backed integration suite (test/kv.test.ts) only tested that set() with a TTL stores that metadata, not that an expired entry is actually excluded when read through the real KV binding. The mocked unit suite in src/mod.test.ts already covered this (added in fedify-dev#987), but not against the real binding. Changelog: none Assisted-by: Claude Code:claude-sonnet-5
Complements the previous commit's expired-entry test: verifies that an entry whose metadata.expires is still in the future is returned normally by get(), not just excluded when it isn't. Changelog: none Assisted-by: Claude Code:claude-sonnet-5
Completes the expiry coverage in test/kv.test.ts: list() applies the same metadata.expires check as get(), skipping both the exact-prefix key and matching children when they're expired. Matches the mocked coverage already in src/mod.test.ts (added in fedify-dev#987), now verified against the real KV binding. Changelog: none Assisted-by: Claude Code:claude-sonnet-5
✅ Deploy Preview for fedify-json-schema canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughAdded tests for ChangesKV expiration filtering
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to The added expiration tests improve KV coverage, but a reported duplicate declaration may prevent the test file from compiling. Confirm and remove the duplicate before merging. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The PR covers the primary requirements in issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
dahlia
added
component/kv
2 participants
Closes #876
Background
@fedify/cfworkers'sWorkersKvStoreapplies its own expiry check via ametadata.expiresfield, independent of Workers KV's native TTL (Workers KV enforces a 60-second minimum TTL and doesn't expire entries immediately).src/mod.test.tsalready had unit tests for this against a hand-written mock namespace (added in #987), but the miniflare-backed integration suite intest/kv.test.ts— which exercises the real KV binding — only tested thatset()with a TTL stores the metadata, not that an expired entry is actually excluded when read back.Changes
Add tests in packages/cfworkers/test/kv.test.ts that seed an already-expired entry directly via
env.KV1.put()(bypassingset()'s 60-second TTL floor) and verify:get()excludes an expired entry.get()still returns an entry whose expiry is in the future.list()excludes expired entries, for both the exact-prefix key and matching children.Testing
pnpm --filter @fedify/cfworkers test(all 47 tests pass, including the 3 new ones)AI disclosure
Tests were written by me. Claude Code (
claude-sonnet-5) helped me find the gap between the mocked unit coverage and the real-binding integration suite, explained the pattern for seeding an already-expired entry via a directput()call, and pointed out a couple of formatting issues and one test-assertion bug (comparing an array to a bare object) that I then fixed myself.