Added integration tests for AdapterCacheRedis#27467
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27467 +/- ##
==========================================
+ Coverage 72.85% 73.04% +0.18%
==========================================
Files 1553 1553
Lines 125079 125079
Branches 15104 15140 +36
==========================================
+ Hits 91129 91361 +232
+ Misses 32996 32764 -232
Partials 954 954
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js (1)
153-163: Consider using sinon.stub for timeout simulation.The direct monkey-patch of
cache.cache.getworks but bypasses sinon's automatic cleanup. SinceafterEachquits the client anyway, this is acceptable, but usingsinon.stub(cache.cache, 'get')would be more idiomatic and self-documenting.♻️ Optional refactor using sinon.stub
it('returns null when the underlying get exceeds the timeout', async function () { const cache = createCache({getTimeoutMilliseconds: 1}); await cache.set('slow', 'value'); const original = cache.cache.get.bind(cache.cache); - cache.cache.get = k => new Promise((resolve) => { - setTimeout(() => original(k).then(resolve), 50); - }); + sinon.stub(cache.cache, 'get').callsFake(k => new Promise((resolve) => { + setTimeout(() => original(k).then(resolve), 50); + })); assert.equal(await cache.get('slow'), null); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js` around lines 153 - 163, Replace the direct monkey-patch of cache.cache.get with a sinon stub: capture the original method into a variable (original = cache.cache.get), then call sinon.stub(cache.cache, 'get').callsFake(k => new Promise(resolve => setTimeout(() => original.call(cache.cache, k).then(resolve), 50))); remove the direct assignment (cache.cache.get = ...) so sinon handles replacement and automatic cleanup; reference symbols: cache.cache.get, original, and sinon.stub(...).callsFake.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js`:
- Around line 153-163: Replace the direct monkey-patch of cache.cache.get with a
sinon stub: capture the original method into a variable (original =
cache.cache.get), then call sinon.stub(cache.cache, 'get').callsFake(k => new
Promise(resolve => setTimeout(() => original.call(cache.cache, k).then(resolve),
50))); remove the direct assignment (cache.cache.get = ...) so sinon handles
replacement and automatic cleanup; reference symbols: cache.cache.get, original,
and sinon.stub(...).callsFake.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a25e131-f6c5-4900-adb2-f11980483861
📒 Files selected for processing (4)
.github/workflows/ci.ymlghost/core/core/shared/config/env/config.testing-mysql.jsonghost/core/core/shared/config/env/config.testing.jsonghost/core/test/integration/adapters/redis/adapter-cache-redis.test.js
ref no-issue The redis cache adapter only has unit tests today, and they stub the underlying cache layer completely - so they verify the implementation but not the behavioural contract against a real Redis. We're planning some changes to the redis cache and want full coverage of the current behaviour locked in before we touch the implementation, so any behavioural regression is caught immediately. Wires redis as a CI service alongside mysql and adds a default adapters.Redis block to the testing config so the suite reads the host through the standard config layer.
allouis
force-pushed
the
add-redis-adapter-integration-tests
branch
from
270b0d1 to
0da19c3
Compare
|
2 participants
Why
The redis cache adapter only has unit tests today, and they stub the cache layer completely. They verify the implementation but not the behavioural contract against a real Redis. We have changes to the redis cache coming up and want the current behaviour locked in before we touch anything.
What
ghost/core/test/integration/adapters/redis/adapter-cache-redis.test.jswith 12 tests against a real Redis:set→getround-trip and overwriteset → reset → get === null, andfetchDatais re-called after a resetkeyPrefixisolation:reset()on prefix A does not affect prefix Bget(k, fetchData)cache-miss / cache-hit semanticskeyPrefixconfiguredgetTimeoutMillisecondsshort-circuit (under and over the timeout)config.testing.json/config.testing-mysql.json: added a defaultadapters.Redisblock (127.0.0.1:6379) so the test reads the host via the standard config layer. Override via env (adapters__Redis__host,adapters__Redis__port)..github/workflows/ci.yml: addedredis:7.0as a service to theAcceptance testsjob, matching howmysqlis already wired up.Coverage
Coverage of
core/server/adapters/lib/redis/AdapterCacheRedis.jsfrom this suite alone:What's not covered:
refreshAheadFactor); the existing unit tests already cover this, and timing-based integration tests are flakykeys()is about to be replaced with a stub that throws (see Stubbed AdapterCacheRedis.keys() with an IncorrectUsageError #27465); the unit-level assertion for the new behaviour lives thereCluster: out of scope
The adapter has a
#getPrimaryRedisNodebranch that runs when the underlying client is an ioredisCluster, used to routeSCANto a single primary node. This suite doesn't cover it. Adding it would mean spinning up a multi-node cluster (something likegrokzen/redis-cluster) as a separate CI service. The value isn't there right now, since the planned changes shrink this code path further. Worth revisiting if cluster correctness becomes a concern.Notes for reviewers
keyPrefix(viacrypto.randomBytes) so tests don't collide. Connections are closed inafterEach; any data left behind ages out via the prefix.