Test/backend core coverage #260
Conversation
📝 WalkthroughWalkthroughThis PR establishes database connection infrastructure with async SQLAlchemy and asyncpg, adding configuration support for DATABASE_URL, a database core module with connection pooling and session management, handler registry enum normalization, and comprehensive test coverage for new components and utilities. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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 |
![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 `@docs/DATABASE_CONNECTION.md`:
- Around line 19-25: Update the module path in the documentation so it matches
the repository layout: change references to app/database/core.py to
backend/app/database/core.py in the "Engine & Pooling" section (and any other
occurrences), ensuring the doc points to the actual module location used by the
codebase.
🧹 Nitpick comments (9)
pyproject.toml (1)
43-44: Keep dependency constraints in sync with backend/requirements.txt.
SQLAlchemyis pinned to a specific version in backend/requirements.txt, while pyproject.toml allows a range. If both are used in different environments, this can cause drift. Consider aligning them to a single source of truth.tests/test_agent_state.py (1)
6-25: Consider centralizing sys.path manipulation in a shared test hook.Per-test sys.path edits can be order-dependent. A conftest.py or pytest.ini pythonpath setting would keep test imports consistent and reduce duplication.
tests/test_db_pool.py (1)
59-64: Rename unused async-loop variables to satisfy Ruff B007.Use
_sessionto avoid lint warnings in the async generator loops.Proposed diff
- async for session in mock_db_module.get_db(): + async for _session in mock_db_module.get_db(): # Simulate some work await asyncio.sleep(0.01) return True- async for session in mock_db_module.get_db(): + async for _session in mock_db_module.get_db(): raise ValueError("Simulated Error")Also applies to: 90-91
tests/test_handler_registry.py (1)
19-34: Rename unused handler parameters to avoid Ruff ARG002.Using
_eventmakes the unused argument explicit and keeps lint clean.Proposed diff
class MockHandler(BaseHandler): """Concrete handler for testing.""" - async def handle(self, event: BaseEvent): + async def handle(self, _event: BaseEvent): return {"handled": True, "handler": "MockHandler"} class AnotherMockHandler(BaseHandler): """Another concrete handler for testing.""" - async def handle(self, event: BaseEvent): + async def handle(self, _event: BaseEvent): return {"handled": True, "handler": "AnotherMockHandler"} class DiscordSpecificHandler(BaseHandler): """Platform-specific handler for Discord.""" - async def handle(self, event: BaseEvent): + async def handle(self, _event: BaseEvent): return {"handled": True, "handler": "DiscordSpecificHandler"}tests/test_message_handler.py (1)
58-61: Silence the unused tuple element fromis_faq.
faq_responseisn’t used; renaming avoids lint noise and clarifies intent.♻️ Suggested tweak
- is_faq, faq_response = await self.faq_handler.is_faq(content) + is_faq, _faq_response = await self.faq_handler.is_faq(content)tests/test_faq_handler.py (4)
13-13: Remove unused imports.
ABCandabstractmethodare imported but not used in this file.-from abc import ABC, abstractmethod
72-77: Avoid silent exception swallowing.The bare
except Exception: passsilently swallows all errors, making debugging difficult. Even in a test double, consider logging or at least catching a more specific exception type.🛡️ Suggested improvement
try: channel = self.bot.get_channel(int(channel_id)) if channel: await channel.send(response) - except Exception: - pass + except (ValueError, AttributeError): + # ValueError: invalid channel_id conversion + # AttributeError: bot/channel API issues + pass
26-31: Consider annotating mutable class attribute withClassVar.Static analysis (RUF012) flags that mutable class attributes should be annotated. Since this dict is intended as a constant lookup table, using
ClassVarmakes the intent explicit.♻️ Suggested fix
+from typing import ClassVar + class FAQHandlerTestDouble: """ Test double mirroring the production FAQHandler implementation. This avoids circular import issues while testing the FAQ pattern. """ - FAQ_RESPONSES = { + FAQ_RESPONSES: ClassVar[dict[str, str]] = { "what is devr.ai?": "Devr.AI is an AI-powered Developer Relations assistant.",
115-141: Replace deprecatedasyncio.get_event_loop().run_until_complete()withpytest.mark.asyncio.The
get_event_loop()pattern is deprecated since Python 3.10. Since pytest 8.x and pytest-asyncio are available, use native async test functions instead.♻️ Example refactor for one test
+@pytest.mark.asyncio -def test_is_faq_returns_true_for_known_question(self, handler): +async def test_is_faq_returns_true_for_known_question(self, handler): """is_faq returns (True, response) for known FAQ.""" - result = asyncio.get_event_loop().run_until_complete(handler.is_faq("what is devr.ai?")) + result = await handler.is_faq("what is devr.ai?") assert result[0] is True assert result[1] is not None assert "AI-powered" in result[1]Apply this pattern to all async test methods in this file.
| ### 1. Engine & Pooling | ||
| Located in `app/database/core.py`. | ||
| - **Pool Size**: 20 connections maintained open. | ||
| - **Max Overflow**: 10 temporary connections allowed during high load. | ||
| - **Pool Timeout**: 30 seconds wait time before raising an error. | ||
| - **Pre-Ping**: Checked before checkout to ensure connection health. | ||
|
|
There was a problem hiding this comment.
Clarify the module path to match the repo layout.
The file lives under backend/app/... in this repo; adjusting the path avoids confusion for contributors.
✏️ Suggested doc tweak
-Located in `app/database/core.py`.
+Located in `backend/app/database/core.py`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### 1. Engine & Pooling | |
| Located in `app/database/core.py`. | |
| - **Pool Size**: 20 connections maintained open. | |
| - **Max Overflow**: 10 temporary connections allowed during high load. | |
| - **Pool Timeout**: 30 seconds wait time before raising an error. | |
| - **Pre-Ping**: Checked before checkout to ensure connection health. | |
| ### 1. Engine & Pooling | |
| Located in `backend/app/database/core.py`. | |
| - **Pool Size**: 20 connections maintained open. | |
| - **Max Overflow**: 10 temporary connections allowed during high load. | |
| - **Pool Timeout**: 30 seconds wait time before raising an error. | |
| - **Pre-Ping**: Checked before checkout to ensure connection health. |
🤖 Prompt for AI Agents
In `@docs/DATABASE_CONNECTION.md` around lines 19 - 25, Update the module path in
the documentation so it matches the repository layout: change references to
app/database/core.py to backend/app/database/core.py in the "Engine & Pooling"
section (and any other occurrences), ensuring the doc points to the actual
module location used by the codebase.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/test_db_pool.py`:
- Around line 59-63: Rename the unused loop variable in the async generators to
silence Ruff B007: in the async def task() where you iterate "async for session
in mock_db_module.get_db():" (and the other occurrence later in the file),
change the loop variable name to "_session" so the variable is clearly marked as
intentionally unused; update both places that consume mock_db_module.get_db() to
use "_session" instead of "session".
🧹 Nitpick comments (4)
tests/test_faq_handler.py (4)
13-13: Unused importABC.The
ABCimport fromabcmodule is not used in this file. Consider removing it.-from abc import ABC, abstractmethod +from unittest.mock import MagicMock, AsyncMockWait,
abstractmethodis also unused. Both can be removed:-from abc import ABC, abstractmethod
26-31: Consider annotating mutable class attribute withClassVar.Static analysis flagged that mutable class attributes should be annotated with
typing.ClassVarto indicate they belong to the class, not instances.✨ Suggested fix
+from typing import ClassVar + class FAQHandlerTestDouble: """ Test double mirroring the production FAQHandler implementation. This avoids circular import issues while testing the FAQ pattern. """ - FAQ_RESPONSES = { + FAQ_RESPONSES: ClassVar[dict[str, str]] = { "what is devr.ai?": "Devr.AI is an AI-powered Developer Relations assistant.",
72-77: Silent exception swallowing reduces test fidelity.The bare
except Exception: passsilently swallows all errors, including unexpected ones likeTypeErrororAttributeError. If the productionFAQHandlerlogs errors, this test double should mirror that behavior. At minimum, consider catching more specific exceptions.🛡️ Proposed improvement
+import logging + +logger = logging.getLogger(__name__) + async def _send_discord_response(self, channel_id: str, response: str): """Send a response to Discord channel.""" if self.bot is None: return try: channel = self.bot.get_channel(int(channel_id)) if channel: await channel.send(response) - except Exception: - pass + except (ValueError, AttributeError) as e: + logger.debug("Failed to send Discord response: %s", e)
115-122: Usepytest.mark.asyncioinstead of deprecated event loop pattern.The
asyncio.get_event_loop().run_until_complete()pattern is deprecated since Python 3.10 and emitsDeprecationWarning. Since pytest-asyncio is already a dependency, convert these to native async tests for cleaner, more idiomatic code.This applies to all async test methods in this file (11 tests total).
♻️ Example conversion for this test
+ `@pytest.mark.asyncio` - def test_is_faq_returns_true_for_known_question(self, handler): + async def test_is_faq_returns_true_for_known_question(self, handler): """is_faq returns (True, response) for known FAQ.""" - result = asyncio.get_event_loop().run_until_complete(handler.is_faq("what is devr.ai?")) + result = await handler.is_faq("what is devr.ai?") assert result[0] is True assert result[1] is not None assert "AI-powered" in result[1]Apply the same pattern to:
test_is_faq_returns_false_for_unknown_questiontest_is_faq_case_insensitivetest_is_faq_how_do_i_contributetest_handle_faq_requested_eventtest_handle_knowledge_updated_eventtest_handle_unsupported_event_typetest_send_discord_response_with_bottest_send_discord_response_without_bottest_send_discord_response_channel_not_found
| async def task(): | ||
| # Use the get_db from the RELOADED module | ||
| async for session in mock_db_module.get_db(): | ||
| # Simulate some work | ||
| await asyncio.sleep(0.01) |
There was a problem hiding this comment.
Silence unused loop variable to satisfy Ruff B007.
Line 61 and Line 90 don’t use the loop variable. Rename to _session to avoid lint noise/failures.
🔧 Proposed fix
- async for session in mock_db_module.get_db():
+ async for _session in mock_db_module.get_db():
# Simulate some work
await asyncio.sleep(0.01)
return True
@@
- async for session in mock_db_module.get_db():
+ async for _session in mock_db_module.get_db():
raise ValueError("Simulated Error")Also applies to: 90-91
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 61-61: Loop control variable session not used within loop body
Rename unused session to _session
(B007)
🤖 Prompt for AI Agents
In `@tests/test_db_pool.py` around lines 59 - 63, Rename the unused loop variable
in the async generators to silence Ruff B007: in the async def task() where you
iterate "async for session in mock_db_module.get_db():" (and the other
occurrence later in the file), change the loop variable name to "_session" so
the variable is clearly marked as intentionally unused; update both places that
consume mock_db_module.get_db() to use "_session" instead of "session".
1 participant
📝 Description
This PR significantly improves backend reliability by adding comprehensive unit test coverage for core handler, event, and agent state modules. While writing tests, a production bug was discovered and fixed in
HandlerRegistry.get_handler()related to incorrect assumptions about event value types.The changes are intentionally scoped to core backend logic and do not introduce new features or alter public APIs.
🔧 Changes Made
HandlerRegistry.get_handler():event.platformandevent.event_typewere enums with.value🐞 Bug Fix Details
Issue
HandlerRegistry.get_handler()assumedevent.platformandevent.event_typewere enum objects, butBaseEventstores these fields as strings in some cases. This caused handler resolution failures at runtime.Fix
The handler key resolution now gracefully handles both enum and string inputs.
🧪 Verification
All scoped backend tests pass locally: