Skip to content

Added safety tests for AdapterCacheRedis error and edge-case paths#27479

Merged
vershwal merged 2 commits into
mainfrom
add-redis-cache-adapter-safety-tests
Apr 22, 2026
Merged

Added safety tests for AdapterCacheRedis error and edge-case paths#27479
vershwal merged 2 commits into
mainfrom
add-redis-cache-adapter-safety-tests

Conversation

@vershwal
Copy link
Copy Markdown
Member

ref #19627 HKG-1709

  • We're preparing to merge changes to the Redis cache adapter(concurrent read deduplication). These tests lock in the current error-handling behavior, so any behavioral regression is caught immediately.
  • The current adapter silently swallows errors in several paths and the exact behavior isn't documented by existing tests. The concurrent read deduplication PR will change the get method structure to share in-flight promises, which makes error paths more sensitive — one failure now affects all callers waiting on the same key. We need to know exactly how errors behave today before changing anything.

@vershwal vershwal force-pushed the add-redis-cache-adapter-safety-tests branch 2 times, most recently from 7ad6196 to 2f759fc Compare April 21, 2026 10:01
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 75266337-7e12-443a-b10b-9b4196fb7a49

📥 Commits

Reviewing files that changed from the base of the PR and between 2f759fc and 4ad177f.

📒 Files selected for processing (2)
  • ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js
  • ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
  • ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js
  • ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js

Walkthrough

Adds new integration and unit tests for the Redis cache adapter covering error and retry scenarios. Integration tests assert that a rejected fetch function is not cached, that a subsequent cache.get retries and returns the recovered value, and that complex nested objects are preserved. Unit tests cover: fetchData rejection on cache miss (returns undefined, logs error, retries on next call), Redis get rejection (does not call fetchData, returns undefined, logs), background refresh failures (stale value retained, error logged), and Redis set rejection (returns undefined, logs). No public APIs were changed.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding safety tests for error handling and edge-case paths in the AdapterCacheRedis component.
Description check ✅ Passed The description clearly explains the purpose of the tests, their relationship to the concurrent read deduplication work, and why error-handling behavior documentation is important.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch add-redis-cache-adapter-safety-tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js`:
- Around line 202-239: The test must assert the background refresh actually ran:
after creating the RedisCache and stubbing fetchData to resolve then reject,
call cache.get(KEY, fetchData) twice and then assert that fetchData was invoked
twice (e.g., fetchData.calledTwice) to prove the refresh attempt occurred; also
stub the cache/logger error method (or processLogger.error depending on how
RedisCache logs) before the second call and assert that the error logger was
called with the refresh error to ensure the refresh failure path was exercised —
reference the RedisCache constructor, cache.get, and the fetchData stub when
adding these assertions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b112399f-161f-4fde-a160-18fec512f24a

📥 Commits

Reviewing files that changed from the base of the PR and between 431788d and 2f759fc.

📒 Files selected for processing (2)
  • ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js
  • ghost/core/test/unit/server/adapters/lib/redis/adapter-cache-redis.test.js

ref #19627

- We're preparing to merge changes to the Redis cache adapter(concurrent read deduplication from PR #19627). These tests lock in the current error-handling behavior, so any behavioral regression is caught immediately.
- The current adapter silently swallows errors in several paths and the exact behavior isn't documented by existing tests. PR #19627 changes the `get` method structure to share in-flight promises, which makes error paths more sensitive — one failure now affects all callers waiting on the same key. We need to know exactly how errors behave today before changing anything.
The existing test verified the return value was correct but never
proved the refresh actually ran or that the error was handled. It
would still pass if the background refresh code path were removed
entirely. Added assertions that fetchData was called twice (proving
the refresh attempt) and that logging.error received the structured
error object from the .catch handler (proving the failure path was
exercised). A short setTimeout drains the microtask queue so the
fire-and-forget .catch callback has settled before we assert.
@vershwal vershwal force-pushed the add-redis-cache-adapter-safety-tests branch from 7424702 to 4ad177f Compare April 21, 2026 11:56
@sonarqubecloud
Copy link
Copy Markdown

@vershwal vershwal requested a review from allouis April 21, 2026 12:41
@vershwal vershwal merged commit 9e007dd into main Apr 22, 2026
43 checks passed
@vershwal vershwal deleted the add-redis-cache-adapter-safety-tests branch April 22, 2026 10:02
@vershwal vershwal restored the add-redis-cache-adapter-safety-tests branch April 22, 2026 10:11
vershwal added a commit that referenced this pull request Apr 22, 2026
…paths" (#27502)

Reverts #27479

Reverting this for now — the tests passed on my branch, but it wasn’t rebased onto the latest main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants