Add tests to cover exception and edge cases in server routes #5
Conversation
Expands unit test coverage for server endpoints by adding tests for malformed inputs, Redis and queue exceptions, and various lifespan and requeue scenarios. Improves reliability by ensuring robust handling of error paths and edge cases.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaced startup call to the queue's private Changes
Sequence Diagram(s)(Skipped — changes are small control-flow adjustment and expanded tests; no new multi-component feature requiring sequence visualization.) Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_routes.py`:
- Around line 573-589: Rename the test function from
test_lifespaninitializes_queue to test_lifespan_initializes_queue to fix the
typo, and replace the bare "except: pass" around awaiting
lifespan_cm.__aexit__(...) with specific exception handling (e.g., catch
asyncio.CancelledError and optionally Exception as e) so you only silence
expected cancellation/errors; update references in the test to use
server._lifespan, lifespan_cm, and server._requeue_task accordingly.
🧹 Nitpick comments (5)
tests/test_routes.py (5)
13-13: Unused import:LockError
LockErroris imported but not used anywhere in this test file. Consider removing it to keep imports clean.🧹 Suggested fix
-from redis.exceptions import LockError
306-309: Weak assertion can be made deterministic.Based on the server implementation, when
get_queue_length()fails,current_queue_lengthdefaults to0. Since0 < 5(themax_queued_length), the enqueue will proceed and return status201. The assertion should be explicit.✅ Suggested fix
- # Even if get_queue_length fails, enqueue proceeds (prints error) - # The exception is caught and printed; enqueue still attempts - # Check if response indicates the error - self.assertIn(response.status_code, [201, 400]) + # When get_queue_length fails, current_queue_length defaults to 0 + # Since 0 < max_queued_length (5), enqueue proceeds successfully + self.assertEqual(response.status_code, 201) + self.assertEqual(response.json()["status"], "queued")
418-426: Misleading test name and duplicate test logic.This test is named
test_clear_queue_malformed_jsonbut doesn't actually test malformed JSON parsing—it tests the exception path via mockingclear_queue, which is identical totest_clear_queue_exceptionbelow. Either rename this test to reflect what it actually tests or implement actual malformed JSON testing.✅ Option 1: Actually test malformed JSON
async def test_clear_queue_malformed_json(self): - """Test clear_queue - testing through the server's request body parsing.""" - # Note: httpx doesn't easily let us send raw body with DELETE, - # so we test the exception path via mocking instead - with patch.object(self.queue, "clear_queue", side_effect=Exception("Clear error")): - # The server will still try to parse a body even if empty - response = await self.client.delete("/deletequeue/sms/johndoe/") - self.assertEqual(response.status_code, 400) - self.assertEqual(response.json()["status"], "failure") + """Test clear_queue with malformed JSON body.""" + response = await self.client.request( + "DELETE", + "/deletequeue/sms/johndoe/", + content=b"invalid json {", + headers={"Content-Type": "application/json"}, + ) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json()["status"], "failure") + self.assertIn("message", response.json())
454-458: Fix broad exception assertion and unused variable.Static analysis correctly flags two issues:
assertRaises(Exception)is too broad and may mask other unexpected exceptions- The
responsevariable is assigned but never used✅ Suggested fix
async def test_deep_status_exception(self): """Test deep_status when queue.deep_status() raises an exception.""" with patch.object(self.queue, "deep_status", side_effect=Exception("Status check failed")): - with self.assertRaises(Exception): - response = await self.client.get("/deepstatus/") + with self.assertRaises(Exception) as context: + await self.client.get("/deepstatus/") + # Verify it's the expected exception chain + self.assertIsNotNone(context.exception.__cause__)
564-571: HandleCancelledErrorspecifically instead of silencing all exceptions.The broad
except Exception: passcould mask unexpected failures. Since you expectCancelledErrorduring shutdown, handle it explicitly.✅ Suggested fix
# Exit lifespan (shutdown) - try: - await lifespan_cm.__aexit__(None, None, None) - except Exception: - pass # May raise if task is cancelled + await lifespan_cm.__aexit__(None, None, None) # Task should be cancelled or done await asyncio.sleep(0.05) self.assertTrue(server._requeue_task.done() or server._requeue_task.cancelled())The lifespan's
__aexit__already suppressesCancelledErrorinternally (seesuppress(asyncio.CancelledError)in server.py line 96), so no try-except should be needed here.
tests/test_routes.py
Outdated
| async def test_lifespaninitializes_queue(self): | ||
| """Test that lifespan calls queue.initialize().""" | ||
| config_path = os.path.join(os.path.dirname(__file__), "test.conf") | ||
| server = setup_server(config_path) | ||
|
|
||
| with patch.object(server.queue, "initialize", new_callable=AsyncMock) as mock_init: | ||
| lifespan_cm = server._lifespan(server.app) | ||
| await lifespan_cm.__aenter__() | ||
|
|
||
| mock_init.assert_called_once() | ||
|
|
||
| # Cleanup | ||
| server._requeue_task.cancel() | ||
| try: | ||
| await lifespan_cm.__aexit__(None, None, None) | ||
| except: | ||
| pass |
There was a problem hiding this comment.
Fix function name typo and avoid bare except.
- The function name
test_lifespaninitializes_queueis missing an underscore—should betest_lifespan_initializes_queue - Lines 588-589 use a bare
except: passwhich silences all errors includingKeyboardInterruptandSystemExit
🐛 Suggested fix
- async def test_lifespaninitializes_queue(self):
+ async def test_lifespan_initializes_queue(self):
"""Test that lifespan calls queue.initialize()."""
config_path = os.path.join(os.path.dirname(__file__), "test.conf")
server = setup_server(config_path)
with patch.object(server.queue, "initialize", new_callable=AsyncMock) as mock_init:
lifespan_cm = server._lifespan(server.app)
await lifespan_cm.__aenter__()
mock_init.assert_called_once()
# Cleanup
server._requeue_task.cancel()
- try:
- await lifespan_cm.__aexit__(None, None, None)
- except:
- pass
+ await lifespan_cm.__aexit__(None, None, None)🧰 Tools
🪛 Ruff (0.14.13)
588-588: Do not use bare except
(E722)
588-589: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
In `@tests/test_routes.py` around lines 573 - 589, Rename the test function from
test_lifespaninitializes_queue to test_lifespan_initializes_queue to fix the
typo, and replace the bare "except: pass" around awaiting
lifespan_cm.__aexit__(...) with specific exception handling (e.g., catch
asyncio.CancelledError and optionally Exception as e) so you only silence
expected cancellation/errors; update references in the test to use
server._lifespan, lifespan_cm, and server._requeue_task accordingly.
There was a problem hiding this comment.
Pull request overview
This PR increases route-level test coverage for error/edge paths in the FQServer API and updates queue startup initialization to use the public initialize() method.
Changes:
- Added many new async tests covering malformed input, internal exceptions, and background requeue/lifespan behaviors.
- Updated queue initialization calls from deprecated
_initialize()toinitialize()in both server lifespan and test setup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
tests/test_routes.py |
Adds extensive new tests for exception paths/edge cases across routes plus requeue/lifespan coverage; updates test setup to call queue.initialize() |
fq_server/server.py |
Switches lifespan startup initialization from queue._initialize() to queue.initialize() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/test_routes.py
Outdated
| # Use a real redis lock that times out | ||
| requeue_task = asyncio.create_task(server.requeue_with_lock()) | ||
|
|
||
| # Let it try to acquire lock and timeout (the default behavior when another process holds it) | ||
| await asyncio.sleep(0.15) |
There was a problem hiding this comment.
This test claims to validate the LockError path, but it doesn't force lock acquisition failure (no competing lock is held and no blocking_timeout is configured). As written it will usually acquire the lock and not exercise the except LockError branch. Consider mocking redis.lock(...) to raise LockError (or acquiring the same lock first with a short blocking_timeout) and assert the loop continues/sleeps rather than crashing.
tests/test_routes.py
Outdated
| # Exit lifespan (shutdown) | ||
| try: | ||
| await lifespan_cm.__aexit__(None, None, None) | ||
| except Exception: | ||
| pass # May raise if task is cancelled |
There was a problem hiding this comment.
Swallowing all exceptions from __aexit__ makes this test pass even if shutdown fails for reasons other than cancellation (and can hide real regressions). Since _lifespan already suppresses CancelledError, __aexit__ should not normally raise—remove the broad try/except and let unexpected exceptions fail the test.
tests/test_routes.py
Outdated
|
|
||
| with patch.object(server.queue, "initialize", new_callable=AsyncMock) as mock_init: | ||
| lifespan_cm = server._lifespan(server.app) | ||
| await lifespan_cm.__aenter__() | ||
|
|
||
| mock_init.assert_called_once() | ||
|
|
||
| # Cleanup | ||
| server._requeue_task.cancel() | ||
| try: | ||
| await lifespan_cm.__aexit__(None, None, None) | ||
| except: |
There was a problem hiding this comment.
Here queue.initialize() is mocked out, but entering lifespan still starts the background requeue_with_lock() task, which may call into an uninitialized queue/redis client and fail nondeterministically (or require the later broad exception swallowing). For an isolated test of initialization, also stub server.requeue_with_lock (or patch config to disable requeue) so startup/shutdown behavior is deterministic.
| with patch.object(server.queue, "initialize", new_callable=AsyncMock) as mock_init: | |
| lifespan_cm = server._lifespan(server.app) | |
| await lifespan_cm.__aenter__() | |
| mock_init.assert_called_once() | |
| # Cleanup | |
| server._requeue_task.cancel() | |
| try: | |
| await lifespan_cm.__aexit__(None, None, None) | |
| except: | |
| # Stub out both queue.initialize and the background requeue task to make | |
| # startup/shutdown deterministic and avoid hitting an uninitialized queue. | |
| with patch.object(server.queue, "initialize", new_callable=AsyncMock) as mock_init, \ | |
| patch.object(server, "requeue_with_lock", new_callable=AsyncMock): | |
| lifespan_cm = server._lifespan(server.app) | |
| await lifespan_cm.__aenter__() | |
| mock_init.assert_called_once() | |
| # Cleanup | |
| if server._requeue_task is not None and not server._requeue_task.done(): | |
| server._requeue_task.cancel() | |
| try: | |
| await lifespan_cm.__aexit__(None, None, None) | |
| except Exception: | |
| # Ignore exceptions that may arise from task cancellation during shutdown |
tests/test_routes.py
Outdated
| except: | ||
| pass | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Avoid a bare except: here—if _lifespan.__aexit__ raises unexpectedly (e.g., background task failed with an exception), this will hide it and can produce false-green tests. Catch the specific expected exception(s) or let it fail.
| except: | |
| pass | |
| except asyncio.CancelledError: | |
| # Expected if the requeue task is cancelled during shutdown | |
| pass |
tests/test_routes.py
Outdated
| from unittest.mock import AsyncMock, MagicMock, patch | ||
| from httpx import AsyncClient, ASGITransport | ||
| from starlette.types import ASGIApp | ||
| from redis.exceptions import LockError |
There was a problem hiding this comment.
MagicMock and LockError are imported but never used in this test module. If/when linting is enabled, this will fail CI; please remove unused imports (or add assertions that actually use LockError in the requeue lock test).
| from unittest.mock import AsyncMock, MagicMock, patch | |
| from httpx import AsyncClient, ASGITransport | |
| from starlette.types import ASGIApp | |
| from redis.exceptions import LockError | |
| from unittest.mock import AsyncMock, patch | |
| from httpx import AsyncClient, ASGITransport | |
| from starlette.types import ASGIApp |
tests/test_routes.py
Outdated
| # Even if get_queue_length fails, enqueue proceeds (prints error) | ||
| # The exception is caught and printed; enqueue still attempts | ||
| # Check if response indicates the error | ||
| self.assertIn(response.status_code, [201, 400]) |
There was a problem hiding this comment.
This assertion is too permissive for the behavior being tested (it allows both success and failure). To make the test deterministic and actually validate the exception path, stub queue.enqueue to a known successful response and assert 201 (and/or assert current_queue_length is 0 when get_queue_length fails).
tests/test_routes.py
Outdated
| """Test clear_queue - testing through the server's request body parsing.""" | ||
| # Note: httpx doesn't easily let us send raw body with DELETE, | ||
| # so we test the exception path via mocking instead | ||
| with patch.object(self.queue, "clear_queue", side_effect=Exception("Clear error")): | ||
| # The server will still try to parse a body even if empty | ||
| response = await self.client.delete("/deletequeue/sms/johndoe/") | ||
| self.assertEqual(response.status_code, 400) | ||
| self.assertEqual(response.json()["status"], "failure") |
There was a problem hiding this comment.
This test is named/ documented as covering malformed JSON, but it never sends malformed JSON and instead only patches queue.clear_queue to raise. httpx can send a body with DELETE (via content=), so you can directly send invalid JSON and assert the 400 parse failure; otherwise this is redundant with test_clear_queue_exception.
| """Test clear_queue - testing through the server's request body parsing.""" | |
| # Note: httpx doesn't easily let us send raw body with DELETE, | |
| # so we test the exception path via mocking instead | |
| with patch.object(self.queue, "clear_queue", side_effect=Exception("Clear error")): | |
| # The server will still try to parse a body even if empty | |
| response = await self.client.delete("/deletequeue/sms/johndoe/") | |
| self.assertEqual(response.status_code, 400) | |
| self.assertEqual(response.json()["status"], "failure") | |
| """Test clear_queue with malformed JSON body.""" | |
| response = await self.client.delete( | |
| "/deletequeue/sms/johndoe/", | |
| content=b"invalid json", | |
| headers={"Content-Type": "application/json"}, | |
| ) | |
| self.assertEqual(response.status_code, 400) | |
| self.assertEqual(response.json()["status"], "failure") |
tests/test_routes.py
Outdated
| """Test deep_status when queue.deep_status() raises an exception.""" | ||
| with patch.object(self.queue, "deep_status", side_effect=Exception("Status check failed")): | ||
| with self.assertRaises(Exception): | ||
| response = await self.client.get("/deepstatus/") |
There was a problem hiding this comment.
Variable response is not used.
| response = await self.client.get("/deepstatus/") | |
| await self.client.get("/deepstatus/") |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
…and exception handling Co-authored-by: ochui <21917688+ochui@users.noreply.github.com>
Refactors the test to use a custom async context manager that raises the lock error, providing more accurate simulation of lock acquisition failure. Removes unnecessary assertions on mock calls for clarity.
Address code review feedback: fix test assertions and exception handling
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_routes.py`:
- Around line 501-531: In test_requeue_with_lock_lock_error update the
FailingLock and mock_redis.lock to satisfy Ruff: change the LockError
instantiation in FailingLock.__aenter__ to raise LockError() with no message,
and replace the lambda signature used for mock_redis.lock to use underscores for
unused args (e.g., lambda *_, **__: FailingLock() or similar) so there are no
unused-argument warnings; ensure references to FailingLock, mock_redis.lock and
the test_requeue_with_lock_lock_error function remain intact.
♻️ Duplicate comments (1)
tests/test_routes.py (1)
589-610: Fix typo in test name.Line 589 still misses an underscore; rename for clarity and consistency. (Duplicate of prior feedback.)
✏️ Suggested rename
- async def test_lifespaninitializes_queue(self): + async def test_lifespan_initializes_queue(self):
| async def test_requeue_with_lock_lock_error(self): | ||
| """Test requeue_with_lock when lock acquisition fails with LockError.""" | ||
| from redis.exceptions import LockError | ||
| server = self.server | ||
|
|
||
| # Create an async context manager that raises LockError on enter | ||
| class FailingLock: | ||
| async def __aenter__(self): | ||
| raise LockError("Failed to acquire lock") | ||
|
|
||
| async def __aexit__(self, *args): | ||
| pass | ||
|
|
||
| # Mock redis_client with a lock method that returns the failing lock | ||
| mock_redis = AsyncMock() | ||
| # Make lock a regular (non-async) function that returns the context manager | ||
| mock_redis.lock = lambda *args, **kwargs: FailingLock() | ||
|
|
||
| with patch.object(server.queue, "redis_client", return_value=mock_redis): | ||
| requeue_task = asyncio.create_task(server.requeue_with_lock()) | ||
|
|
||
| # Let it try to acquire lock and handle LockError (sleeps and continues) | ||
| await asyncio.sleep(0.15) | ||
|
|
||
| # Cancel it | ||
| requeue_task.cancel() | ||
|
|
||
| try: | ||
| await requeue_task | ||
| except asyncio.CancelledError: | ||
| pass # Expected - loop continues after LockError, then cancelled |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the test_routes.py file
find . -name "test_routes.py" -type fRepository: flowdacity/flowdacity-queue-server
Length of output: 100
🏁 Script executed:
# Read the specific lines mentioned in the review (501-531)
sed -n '501,531p' ./tests/test_routes.py | cat -nRepository: flowdacity/flowdacity-queue-server
Length of output: 1623
🏁 Script executed:
# Check if Ruff is available and run it on the test_routes.py file
if command -v ruff &> /dev/null; then
ruff check ./tests/test_routes.py --select=ARG,TRY --show-source 2>&1 | head -100
else
echo "Ruff not found, checking for pyproject.toml or ruff.toml for config"
ls -la | grep -E "(pyproject.toml|ruff.toml)"
fiRepository: flowdacity/flowdacity-queue-server
Length of output: 277
🏁 Script executed:
# Run Ruff check on test_routes.py without --show-source
ruff check ./tests/test_routes.py --select=ARG,TRY 2>&1Repository: flowdacity/flowdacity-queue-server
Length of output: 2823
Fix Ruff lint warnings in the lock failure mock.
Line 509's LockError message (TRY003) and line 517's unused lambda arguments (ARG005) need to be addressed. Remove the exception message and replace unused parameters with underscores.
🔧 Suggested update
class FailingLock:
async def __aenter__(self):
- raise LockError("Failed to acquire lock")
+ raise LockError()
async def __aexit__(self, *args):
pass
@@
- mock_redis.lock = lambda *args, **kwargs: FailingLock()
+ def lock(*_args, **_kwargs):
+ return FailingLock()
+ mock_redis.lock = lock🧰 Tools
🪛 Ruff (0.14.13)
509-509: Avoid specifying long messages outside the exception class
(TRY003)
517-517: Unused lambda argument: args
(ARG005)
517-517: Unused lambda argument: kwargs
(ARG005)
🤖 Prompt for AI Agents
In `@tests/test_routes.py` around lines 501 - 531, In
test_requeue_with_lock_lock_error update the FailingLock and mock_redis.lock to
satisfy Ruff: change the LockError instantiation in FailingLock.__aenter__ to
raise LockError() with no message, and replace the lambda signature used for
mock_redis.lock to use underscores for unused args (e.g., lambda *_, **__:
FailingLock() or similar) so there are no unused-argument warnings; ensure
references to FailingLock, mock_redis.lock and the
test_requeue_with_lock_lock_error function remain intact.
This pull request improves test coverage and modernizes initialization in the FQServer codebase. The most significant changes are the addition of comprehensive tests for exception handling and edge cases in the API, as well as updating the queue initialization method for consistency.
Test Coverage Improvements:
Added extensive new tests to
tests/test_routes.pyto cover exception paths and edge cases for all main API endpoints, including enqueue, dequeue, finish, interval, metrics, clear_queue, deep_status, and requeue operations. These tests ensure that the server handles malformed input, internal exceptions, and configuration scenarios gracefully.Introduced tests specifically for the server's lifespan (startup and shutdown), verifying that the queue is initialized correctly and that background tasks are managed as expected.
Queue Initialization Modernization:
_initialize()method with the publicinitialize()method for queue setup in both the server's lifespan context and the test setup, promoting better encapsulation and maintainability. [1] [2]Test Infrastructure Enhancements:
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.