Update Redis health check tests for client_factory implementation#652
Update Redis health check tests for client_factory implementation#652codingjoe merged 4 commits intoissues/643/redis-clientfrom
Conversation
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot wtf don't change the implementation. Tests and nothing more |
Co-authored-by: codingjoe <1772890+codingjoe@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## issues/643/redis-client #652 +/- ##
============================================================
+ Coverage 99.86% 100.00% +0.13%
============================================================
Files 13 13
Lines 728 728
============================================================
+ Hits 727 728 +1
+ Misses 1 0 -1
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:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Redis health check test suite to align with a client_factory implementation pattern. However, the implementation pattern documented by these tests has fundamental issues: the client is created once during initialization but closed after each health check run, making the health check instance unusable after a single invocation.
Changes:
- Updated existing unit tests to use
client_factoryparameter withclient=None - Added deprecation test for the old
clientparameter - Added test documenting factory initialization behavior (problematic pattern)
- Added test for client closure lifecycle
- Updated integration tests to use
client_factorypattern
| async def test_redis__factory_called_once_in_init(self): | ||
| """Verify client_factory is called once during initialization.""" | ||
| call_count = 0 | ||
|
|
||
| def factory(): | ||
| nonlocal call_count | ||
| call_count += 1 | ||
| client = mock.AsyncMock() | ||
| client.ping.return_value = True | ||
| return client | ||
|
|
||
| check = RedisHealthCheck(client=None, client_factory=factory) | ||
| assert call_count == 1, "Factory should be called once during initialization" | ||
|
|
||
| # Multiple requests reuse the same client | ||
| result1 = await check.get_result() | ||
| assert result1.error is None | ||
| assert call_count == 1, "Factory should not be called again for subsequent requests" | ||
|
|
||
| result2 = await check.get_result() | ||
| assert result2.error is None | ||
| assert call_count == 1, "Factory should still not be called again" |
There was a problem hiding this comment.
This test passes with mocks but documents behavior that won't work with real Redis clients. The implementation calls client_factory once in post_init and closes the client in run()'s finally block (health_check/contrib/redis.py:82). After the first get_result() call, the real Redis client would be closed, causing subsequent calls to fail. This differs from the pattern used by other health checks like RabbitMQ (health_check/contrib/rabbitmq.py:26-31) and Kafka (health_check/contrib/kafka.py:40-69), which create new connections in run() for each health check. The implementation should call client_factory in run() to create a new client per request, or the test should verify that only a single get_result() call is supported per health check instance.
| """Verify client is always closed after health check.""" | ||
| mock_client = mock.AsyncMock() | ||
| mock_client.ping.return_value = True | ||
|
|
||
| with pytest.warns(DeprecationWarning): | ||
| check = RedisHealthCheck(client=mock_client) | ||
|
|
||
| result = await check.get_result() | ||
| assert result.error is None |
There was a problem hiding this comment.
This test is redundant with test_redis__client_deprecated (lines 69-80). Both tests verify the same behavior: deprecation warning when using client parameter, successful ping, and client closure. Consider removing one of these tests or differentiating them by testing different scenarios (e.g., one for success case, one for error case).
| """Verify client is always closed after health check.""" | |
| mock_client = mock.AsyncMock() | |
| mock_client.ping.return_value = True | |
| with pytest.warns(DeprecationWarning): | |
| check = RedisHealthCheck(client=mock_client) | |
| result = await check.get_result() | |
| assert result.error is None | |
| """Verify client is always closed after health check, even on error.""" | |
| mock_client = mock.AsyncMock() | |
| mock_client.ping.side_effect = RedisConnectionError("connection error") | |
| with pytest.warns(DeprecationWarning): | |
| check = RedisHealthCheck(client=mock_client) | |
| result = await check.get_result() | |
| assert result.error is not None | |
| assert isinstance(result.error, ServiceUnavailable) |
| # Create factory that returns Sentinel master client | ||
| def factory(): | ||
| sentinel = Sentinel(sentinels) | ||
| return sentinel.master_for(service_name) |
There was a problem hiding this comment.
The factory creates a Sentinel instance but only returns the master client. The Sentinel instance may hold connections that need to be closed. Consider whether the Sentinel should also be explicitly closed, or if closing the master client automatically closes the Sentinel connections. If Sentinel needs explicit cleanup, the factory pattern may need adjustment to properly manage both the Sentinel and master client lifecycles.
| # Create factory that returns Sentinel master client | |
| def factory(): | |
| sentinel = Sentinel(sentinels) | |
| return sentinel.master_for(service_name) | |
| class RedisClientWithSentinelCleanup: | |
| """Adapter that ensures both the Redis client and its Sentinel are closed.""" | |
| def __init__(self, redis_client, sentinel): | |
| self._redis_client = redis_client | |
| self._sentinel = sentinel | |
| async def ping(self): | |
| return await self._redis_client.ping() | |
| async def aclose(self): | |
| try: | |
| await self._redis_client.aclose() | |
| finally: | |
| # Ensure the Sentinel connections are also closed. | |
| await self._sentinel.aclose() | |
| # Create factory that returns a client with proper Sentinel cleanup | |
| def factory(): | |
| sentinel = Sentinel(sentinels) | |
| master_client = sentinel.master_for(service_name) | |
| return RedisClientWithSentinelCleanup(master_client, sentinel) |
Updates the test suite to match the existing
client_factoryimplementation in the Redis health check. The implementation (added in the base branch) accepts aclient_factoryparameter that is called once during initialization to create a Redis client instance.Test Changes
client_factoryparameter withclient=None(sinceclientis required)DeprecationWarningis raised when using the deprecatedclientparameterclient_factoryis called once during initialization and the same client is reused across requestsclient_factorypattern for both standard Redis and Sentinel clientsUsage
Test Results
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.