redis cache: re-enable scheme/not-installed tests + fix not-installed guard#724
Closed
wbarnha wants to merge 2 commits into
Closed
redis cache: re-enable scheme/not-installed tests + fix not-installed guard#724wbarnha wants to merge 2 commits into
wbarnha wants to merge 2 commits into
Conversation
Both were skipped "Needs fixing" and had drifted against redis-py 8.x and the current cache backend: * Test_RedisScheme referenced `redis.StrictRedis`/`aredis.RedisCluster` as the backend module's client types, which no longer exist there. Assert instead against the scheme->client-class map the backend actually builds (`_client_by_scheme`): single-node resolves to redis-py 8.x's concrete `redis.client.Redis`; cluster resolves to `redis.RedisCluster`. The single-node client is built for real (redis-py connects lazily) and its `connection_pool.connection_kwargs` are checked; the cluster class -- which would connect on construction -- is mocked like the existing `mocked_redis` fixture, while scheme selection and db-stripping run for real. * test_redis__aredis_is_not_installed now patches the `redis` module name the backend's on_start guard actually reads (`if redis is None`), so it raises ImproperlyConfigured without opening a socket. Only external redis client classes are mocked; the backend logic runs for real. NB: the backend imports `redis.asyncio as aredis` but never guards on it -- a possible latent bug left for a separate source-side change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHPL4VFWQRQPpjR1gXSKyL
The redis cache backend's import guard set only `aredis = None` on ImportError, never `redis`. But `on_start` (and the module-level scheme/error guards) key off `if redis is None`, and `redis` stays unbound when the library isn't installed -- so instead of the intended `ImproperlyConfigured`, importing the backend raised `NameError: name 'redis' is not defined`. Bind `redis = None` in the except block so the guard works, and drop the `import redis.asyncio as aredis` alias, which was dead: `aredis` was never referenced anywhere in the backend (only assigned None), and the backend's clients come from the sync `redis` module. Rename the now-accurate test `test_redis__aredis_is_not_installed` -> `test_redis__is_not_installed` (it exercises the `redis is None` guard). Verified: with `redis` patched to None, `on_start` now raises ImproperlyConfigured (previously NameError); redis cache tests still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHPL4VFWQRQPpjR1gXSKyL
wbarnha
changed the title
wbarnha
changed the base branch from
claude/reenable-redis-cache-tests
to
master
July 20, 2026 21:13
This was referenced Jul 20, 2026
Member
Author
|
Superseded by #725. Investigating "can we test redis in CI" revealed the backend is actually broken against a real server (it Generated by Claude Code |
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.
Description
Re-enables the skipped redis cache tests and fixes the latent backend bug they surfaced. Independent, off
master(consolidates the former #721 + the source fix; #721 is closed in favour of this).Source fix —
faust/web/cache/backends/redis.pyThe import guard bound only
aredison failure:but the backend guards on
if redis is None(on_start, scheme/error setup). With redis genuinely uninstalled,redisstays unbound and the firstif redis is NoneraisesNameErrorat import instead of the intendedImproperlyConfigured. Fixed by bindingredis = None, and dropped the deadimport redis.asyncio as aredisalias (only ever assignedNone, never referenced — its presence is what made the guard look correct).Tests —
tests/functional/web/test_cache.pyTest_RedisScheme::test_single_client/test_cluster_client— rewritten for redis-py 8.x: assert against the scheme→client map the backend actually builds (_client_by_scheme); single-node resolves toredis.client.Redis(built for real; connects lazily), cluster toredis.RedisCluster(mocked like the existingmocked_redisfixture, since it connects on construction). Scheme selection anddb-stripping run for real.test_redis__is_not_installed(renamed from…aredis_is_not_installed) — patches theredisname the guard reads and assertsImproperlyConfiguredwithout opening a socket.Verification
tests/functional/web/test_cache.py→ 32 passed; the guard now raisesImproperlyConfigured(previouslyNameError), verified directly. flake8/black/isort clean.Not addressed (separate, larger concern)
The backend maps schemes to sync clients (
redis.StrictRedis/RedisCluster) yetawaits them in_get/_set— fine under the mocked tests, likely broken for real async use. Worth a dedicated look.🤖 Generated with Claude Code