Vineeth/dev 1027#177
Conversation
WalkthroughThis update introduces Sentry tracing decorators to key asynchronous methods in the deriver system, enhancing observability. It also adds a comprehensive suite of new asynchronous tests and fixtures for deriver, queue operations, and CRUD/session logic, as well as documentation for deriver tests. Additional tests for error handling in message and peer routes are included, and LLM call mocking is globally enabled for tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Fixture as Pytest Fixture
participant Deriver as Deriver (Mocked)
participant LLM as LLM Call (Mocked)
participant DB as Test Database
Test->>Fixture: Request sample data/mocks
Fixture->>DB: Setup sample sessions, peers, messages, queue items
Fixture->>Deriver: Provide mock deriver/critical analysis
Fixture->>LLM: Patch LLM calls to return mock responses
Test->>Deriver: Call deriver processing methods (mocked)
Deriver->>LLM: (Mocked) LLM call returns dummy response
Test->>DB: Validate results, assert behaviors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (7)
tests/deriver/__init__.py (1)
1-2: LGTM! Consider removing the trailing blank line.The
__init__.pyfile appropriately marks this as a test package for the deriver system.# Tests for the deriver system. -tests/deriver/README.md (1)
1-73: Excellent documentation for the deriver test suite!The README provides comprehensive information about the test structure, fixtures, and patterns. The code examples are particularly helpful.
Consider these minor formatting improvements:
# Deriver Testing This directory contains tests for the deriver system, which handles background processing of messages to extract insights and update working representations. ## Structure - `conftest.py` - Shared fixtures for deriver testing - `test_queue_operations.py` - Tests for basic queue operations - `test_deriver_processing.py` - Tests for deriver processing logic - `test_queue_processing.py` - Tests for queue manager and work unit processing ## Key Fixtures ### Database Fixtures - `sample_session_with_peers` - Creates a session with multiple peers having different observation configurations - `sample_messages` - Creates sample messages for testing -- `sample_queue_items` - Creates queue items with various payload types (representation, summary) +- `sample_queue_items` - Creates queue items with various payload types (representation, summary) ### Queue Fixtures - `create_queue_payload` - Helper to create queue payloads for testing - `add_queue_items` - Helper to add queue items to the database - `create_active_queue_session` - Helper to create active queue sessions for work unit tracking ### Mocking Fixtures - `mock_deriver_process` - Mocks the deriver process_message method - `mock_critical_analysis_call` - Mocks the critical analysis LLM call - `mock_queue_manager` - Mocks the queue manager for testing - `mock_embedding_store` - Mocks the embedding store operationstests/crud/test_session.py (1)
1-134: Address static analysis issues for consistency.The static analysis correctly identifies missing return type annotations and trailing commas. While these don't affect functionality, addressing them improves code consistency.
Apply this diff for consistent formatting:
- async def test_get_session_peer_configuration( + async def test_get_session_peer_configuration( - self, - db_session: AsyncSession, - sample_data: tuple[models.Workspace, models.Peer], - ): + self, + db_session: AsyncSession, + sample_data: tuple[models.Workspace, models.Peer], + ) -> None: """Test retrieving peer configuration data from session""" # Create another peer peer2 = models.Peer( - name=str(generate_nanoid()), workspace_name=test_workspace.name + name=str(generate_nanoid()), workspace_name=test_workspace.name, ) # Create session test_session = models.Session( - name=str(generate_nanoid()), workspace_name=test_workspace.name + name=str(generate_nanoid()), workspace_name=test_workspace.name, ) peer_configs = { test_peer.name: schemas.SessionPeerConfig( - observe_others=True, observe_me=False + observe_others=True, observe_me=False, ), peer2.name: schemas.SessionPeerConfig( - observe_others=False, observe_me=True + observe_others=False, observe_me=True, ), } - async def test_get_session_not_found(self, db_session: AsyncSession): + async def test_get_session_not_found(self, db_session: AsyncSession) -> None: - async def test_get_peer_config_not_found( + async def test_get_peer_config_not_found( - self, - db_session: AsyncSession, - sample_data: tuple[models.Workspace, models.Peer], - ): + self, + db_session: AsyncSession, + sample_data: tuple[models.Workspace, models.Peer], + ) -> None: # Create session test_session = models.Session( - name=str(generate_nanoid()), workspace_name=test_workspace.name + name=str(generate_nanoid()), workspace_name=test_workspace.name, ) with pytest.raises(ResourceNotFoundException): await crud.get_peer_config( - db_session, test_workspace.name, test_session.name, "nonexistent_peer" + db_session, test_workspace.name, test_session.name, "nonexistent_peer", ) - async def test_clone_session_not_found(self, db_session: AsyncSession): + async def test_clone_session_not_found(self, db_session: AsyncSession) -> None: - async def test_clone_session_invalid_cutoff_message( + async def test_clone_session_invalid_cutoff_message( - self, - db_session: AsyncSession, - sample_data: tuple[models.Workspace, models.Peer], - ): + self, + db_session: AsyncSession, + sample_data: tuple[models.Workspace, models.Peer], + ) -> None: # Create session test_session = models.Session( - name=str(generate_nanoid()), workspace_name=test_workspace.name + name=str(generate_nanoid()), workspace_name=test_workspace.name, ) # Try to clone with invalid cutoff message ID with pytest.raises( ValueError, match="Message not found or doesn't belong to the specified session", ): await crud.clone_session( - db_session, test_workspace.name, test_session.name, "invalid_message_id" + db_session, test_workspace.name, test_session.name, "invalid_message_id", )tests/deriver/test_queue_processing.py (1)
15-153: Add return type annotations for consistency.While not critical for functionality, adding return type annotations improves code consistency with the rest of the codebase.
Add
-> Nonereturn type annotations to all test methods for consistency with coding standards.tests/deriver/test_queue_operations.py (1)
13-86: Add return type annotations for consistency.The static analysis correctly identifies missing return type annotations. Adding them improves code consistency.
Apply this diff to add return type annotations:
- async def test_sample_queue_items_created( + async def test_sample_queue_items_created( self, sample_queue_items: list[models.QueueItem], - ): + ) -> None: - async def test_sample_session_with_peers( + async def test_sample_session_with_peers( self, sample_session_with_peers: tuple[models.Session, list[models.Peer]], - ): + ) -> None: - async def test_sample_messages( + async def test_sample_messages( self, sample_messages: list[models.Message], - ): + ) -> None: - async def test_create_active_queue_session( + async def test_create_active_queue_session( self, create_active_queue_session: Callable[..., Any], sample_session_with_peers: tuple[models.Session, list[models.Peer]], - ): + ) -> None:tests/deriver/test_deriver_processing.py (1)
15-114: Add return type annotations for consistency.Adding return type annotations improves code consistency with the codebase standards.
Add
-> Nonereturn type annotations to all test methods for consistency.tests/routes/test_messages.py (1)
889-924: LGTM! Address static analysis formatting issues.The test correctly verifies that file size limits are enforced, returning a 413 status code for files exceeding the configured maximum size. The approach of using an in-memory file with mocked settings is appropriate for testing this scenario.
Apply this diff to address static analysis issues:
@pytest.mark.asyncio -async def test_create_messages_with_file_too_large( - client: TestClient, db_session: AsyncSession, sample_data: tuple[Workspace, Peer] -): +async def test_create_messages_with_file_too_large( + client: TestClient, + db_session: AsyncSession, + sample_data: tuple[Workspace, Peer], +) -> None: """Test that FileTooLargeError is properly handled in create_messages_with_file""" test_workspace, test_peer = sample_data # Create a test session test_session = models.Session( - workspace_name=test_workspace.name, name=str(generate_nanoid()) + workspace_name=test_workspace.name, + name=str(generate_nanoid()), ) # Mock the settings to make the test deterministic with patch( - "src.routers.messages.settings.MAX_FILE_SIZE", 5 * 1024 * 1024 + "src.routers.messages.settings.MAX_FILE_SIZE", + 5 * 1024 * 1024, ): # 5MB limit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (12)
src/deriver/deriver.py(8 hunks)tests/conftest.py(1 hunks)tests/crud/test_session.py(1 hunks)tests/deriver/README.md(1 hunks)tests/deriver/__init__.py(1 hunks)tests/deriver/conftest.py(1 hunks)tests/deriver/test_deriver_processing.py(1 hunks)tests/deriver/test_queue_operations.py(1 hunks)tests/deriver/test_queue_processing.py(1 hunks)tests/routes/test_messages.py(2 hunks)tests/routes/test_peers.py(1 hunks)tests/test_llm_mock.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Tests in pytest with fixtures in tests/conftest.py
Files:
tests/deriver/__init__.pytests/routes/test_peers.pytests/test_llm_mock.pytests/deriver/test_queue_processing.pytests/conftest.pytests/deriver/conftest.pytests/deriver/test_deriver_processing.pytests/deriver/test_queue_operations.pytests/routes/test_messages.pytests/crud/test_session.py
src/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.py: Follow isort conventions with absolute imports preferred
snake_case for variables/functions; PascalCase for classes
Line length: 88 chars (Black compatible)
Explicit error handling with appropriate exception types
Docstrings: Use Google style docstrings
Use environment variables via python-dotenv (.env)
Use specific exception types (ResourceNotFoundException, ValidationException, etc.)
Proper logging with context instead of print statements
Files:
src/deriver/deriver.py
🧠 Learnings (8)
📚 Learning: the `get_or_create_session` function in this codebase is designed to handle both session creation an...
Learnt from: dr-frmr
PR: plastic-labs/honcho#131
File: src/routers/sessions.py:206-213
Timestamp: 2025-06-18T20:42:06.458Z
Learning: The `get_or_create_session` function in this codebase is designed to handle both session creation and adding peers to existing sessions. When called with peers, it will add those peers to an existing session rather than creating a duplicate session.
Applied to files:
tests/routes/test_peers.pytests/crud/test_session.py
📚 Learning: applies to tests/**/*.py : tests in pytest with fixtures in tests/conftest.py...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to tests/**/*.py : Tests in pytest with fixtures in tests/conftest.py
Applied to files:
tests/deriver/README.mdtests/conftest.pytests/deriver/conftest.pytests/deriver/test_deriver_processing.pytests/deriver/test_queue_operations.py
📚 Learning: the queue and active_queue_sessions tables in the migration script `migrations/versions/d429de0e5338...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: migrations/versions/d429de0e5338_adopt_peer_paradigm.py:918-942
Timestamp: 2025-06-19T14:32:02.934Z
Learning: The queue and active_queue_sessions tables in the migration script `migrations/versions/d429de0e5338_adopt_peer_paradigm.py` are expected to remain small, so per-row updates are acceptable and performance optimizations like set-based updates are not necessary.
Applied to files:
tests/deriver/test_queue_processing.pytests/deriver/conftest.pytests/deriver/test_queue_operations.pytests/crud/test_session.py
📚 Learning: in fastapi tests, it's common to use @pytest.mark.asyncio with a synchronous testclient. the async m...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use @pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
Applied to files:
tests/deriver/test_queue_processing.pytests/conftest.pytests/deriver/test_deriver_processing.pytests/deriver/test_queue_operations.pytests/routes/test_messages.pytests/crud/test_session.py
📚 Learning: applies to src/routers/messages.py : batch message creation supports up to 100 messages...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/routers/messages.py : Batch message creation supports up to 100 messages
Applied to files:
tests/routes/test_messages.py
📚 Learning: the codebase uses a layered error handling approach where the crud layer raises valueerror for busin...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: src/crud.py:374-377
Timestamp: 2025-06-18T21:24:55.907Z
Learning: The codebase uses a layered error handling approach where the CRUD layer raises ValueError for business rule violations (like peer limits), and the API layer catches these ValueError exceptions and translates them to appropriate domain-specific exceptions (ValidationException for validation errors, ResourceNotFoundException for missing resources). This provides clean separation of concerns between business logic and HTTP semantics.
Applied to files:
tests/routes/test_messages.py
📚 Learning: in the codebase, the api layer catches valueerror exceptions from the crud layer and converts them t...
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: src/crud.py:374-377
Timestamp: 2025-06-18T21:24:55.907Z
Learning: In the codebase, the API layer catches ValueError exceptions from the CRUD layer and converts them to ValidationException. This represents a layered error handling approach where business logic violations in the CRUD layer use standard Python exceptions, while the API layer handles the translation to domain-specific exceptions and appropriate HTTP responses.
Applied to files:
tests/routes/test_messages.py
📚 Learning: applies to src/models.py : feature flags on workspace, peer, and session levels...
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to src/models.py : Feature flags on workspace, peer, and session levels
Applied to files:
src/deriver/deriver.py
🧬 Code Graph Analysis (4)
tests/routes/test_peers.py (3)
tests/conftest.py (1)
sample_data(235-253)src/schemas.py (2)
Workspace(41-51)Peer(78-89)src/models.py (2)
Workspace(81-99)Peer(103-134)
tests/test_llm_mock.py (1)
src/deriver/deriver.py (1)
critical_analysis_call(60-73)
tests/deriver/test_queue_operations.py (2)
tests/deriver/conftest.py (4)
sample_queue_items(197-244)sample_session_with_peers(53-81)sample_messages(85-131)create_active_queue_session(248-269)src/models.py (1)
QueueItem(362-371)
tests/routes/test_messages.py (2)
tests/conftest.py (3)
client(168-189)db_session(159-164)sample_data(235-253)src/models.py (4)
Workspace(81-99)Peer(103-134)Session(138-168)Message(172-229)
🪛 Ruff (0.12.2)
tests/routes/test_peers.py
511-511: Missing return type annotation for public function test_get_peers_with_complex_filter
Add return type annotation: None
(ANN201)
512-512: Trailing comma missing
Add trailing comma
(COM812)
535-535: Trailing comma missing
Add trailing comma
(COM812)
536-536: Trailing comma missing
Add trailing comma
(COM812)
544-544: Missing return type annotation for public function test_update_peer_all_fields
Add return type annotation: None
(ANN201)
545-545: Trailing comma missing
Add trailing comma
(COM812)
tests/test_llm_mock.py
11-11: Missing return type annotation for public function test_generic_honcho_llm_call_mock
Add return type annotation: None
(ANN201)
34-34: Missing return type annotation for public function test_summarizer_decorated_functions_with_mock
Add return type annotation: None
(ANN201)
47-47: Trailing comma missing
Add trailing comma
(COM812)
51-51: Trailing comma missing
Add trailing comma
(COM812)
tests/deriver/test_queue_processing.py
15-15: Missing return type annotation for public function test_get_available_work_units
Add return type annotation: None
(ANN201)
44-44: Missing return type annotation for public function test_work_unit_claiming
Add return type annotation: None
(ANN201)
47-47: Unused method argument: sample_queue_items
(ARG002)
47-47: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
78-78: Missing return type annotation for public function test_stale_work_unit_cleanup
Add return type annotation: None
(ANN201)
101-101: Missing return type annotation for public function test_process_item_with_mocked_deriver
Add return type annotation: None
(ANN201)
103-103: Unused method argument: mock_deriver_process
(ARG002)
103-103: Dynamically typed expressions (typing.Any) are disallowed in mock_deriver_process
(ANN401)
103-103: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
117-117: Missing return type annotation for public function test_work_unit_string_representation
Add return type annotation: None
(ANN201)
118-118: Trailing comma missing
Add trailing comma
(COM812)
tests/conftest.py
381-381: mock_honcho_llm_call is too complex (17 > 10)
(C901)
381-381: Missing return type annotation for public function mock_honcho_llm_call
(ANN201)
393-393: Dynamically typed expressions (typing.Any) are disallowed in response_model
(ANN401)
394-394: Boolean-typed positional argument in function definition
(FBT001)
394-394: Boolean default positional argument in function definition
(FBT002)
395-395: Boolean-typed positional argument in function definition
(FBT001)
395-395: Boolean default positional argument in function definition
(FBT002)
396-396: Dynamically typed expressions (typing.Any) are disallowed in create_mock_response
(ANN401)
403-403: Unnecessary elif after return statement
Remove unnecessary elif
(RET505)
412-412: Trailing comma missing
Add trailing comma
(COM812)
418-418: Unnecessary elif after return statement
Remove unnecessary elif
(RET505)
429-429: Trailing comma missing
Add trailing comma
(COM812)
459-459: Unused lambda argument: args
(ARG005)
459-459: Unused lambda argument: kwargs
(ARG005)
463-463: Unused function argument: args
(ARG001)
463-463: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
463-463: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
467-467: Unused function argument: func_args
(ARG001)
467-467: Dynamically typed expressions (typing.Any) are disallowed in *func_args
(ANN401)
467-467: Unused function argument: func_kwargs
(ARG001)
467-467: Dynamically typed expressions (typing.Any) are disallowed in **func_kwargs
(ANN401)
467-467: Dynamically typed expressions (typing.Any) are disallowed in async_wrapper
(ANN401)
475-475: Unused function argument: func_args
(ARG001)
475-475: Dynamically typed expressions (typing.Any) are disallowed in *func_args
(ANN401)
475-475: Unused function argument: func_kwargs
(ARG001)
475-475: Dynamically typed expressions (typing.Any) are disallowed in **func_kwargs
(ANN401)
475-475: Dynamically typed expressions (typing.Any) are disallowed in sync_wrapper
(ANN401)
488-488: Unnecessary else after return statement
Remove unnecessary else
(RET505)
tests/deriver/conftest.py
21-21: Dynamically typed expressions (typing.Any) are disallowed in _self
(ANN401)
35-35: Dynamically typed expressions (typing.Any) are disallowed in *_args
(ANN401)
35-35: Dynamically typed expressions (typing.Any) are disallowed in **_kwargs
(ANN401)
47-47: Trailing comma missing
Add trailing comma
(COM812)
127-127: Trailing comma missing
Add trailing comma
(COM812)
131-131: Unnecessary assignment to messages before return statement
Remove unnecessary assignment
(RET504)
172-172: Trailing comma missing
Add trailing comma
(COM812)
244-244: Unnecessary assignment to queue_items before return statement
Remove unnecessary assignment
(RET504)
273-273: Unused function argument: monkeypatch
(ARG001)
301-301: Unused function argument: monkeypatch
(ARG001)
tests/deriver/test_deriver_processing.py
15-15: Missing return type annotation for public function test_mock_deriver_process
Add return type annotation: None
(ANN201)
17-17: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
18-18: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
31-31: Missing return type annotation for public function test_mock_critical_analysis_call
Add return type annotation: None
(ANN201)
43-43: Missing return type annotation for public function test_work_unit_creation
Add return type annotation: None
(ANN201)
77-77: Missing return type annotation for public function test_mock_queue_manager
Add return type annotation: None
(ANN201)
79-79: Dynamically typed expressions (typing.Any) are disallowed in mock_queue_manager
(ANN401)
101-101: Missing return type annotation for public function test_mock_embedding_store
Add return type annotation: None
(ANN201)
103-103: Dynamically typed expressions (typing.Any) are disallowed in mock_embedding_store
(ANN401)
tests/deriver/test_queue_operations.py
13-13: Missing return type annotation for public function test_sample_queue_items_created
Add return type annotation: None
(ANN201)
39-39: Missing return type annotation for public function test_sample_session_with_peers
Add return type annotation: None
(ANN201)
51-51: Missing return type annotation for public function test_sample_messages
Add return type annotation: None
(ANN201)
64-64: Missing return type annotation for public function test_create_active_queue_session
Add return type annotation: None
(ANN201)
tests/routes/test_messages.py
801-801: Missing return type annotation for public function test_get_messages_handles_crud_value_error
Add return type annotation: None
(ANN201)
802-802: Trailing comma missing
Add trailing comma
(COM812)
809-809: Trailing comma missing
Add trailing comma
(COM812)
828-828: Missing return type annotation for public function test_get_message_handles_not_found
Add return type annotation: None
(ANN201)
829-829: Trailing comma missing
Add trailing comma
(COM812)
836-836: Trailing comma missing
Add trailing comma
(COM812)
846-846: Trailing comma missing
Add trailing comma
(COM812)
854-854: Missing return type annotation for public function test_update_message_handles_crud_value_error
Add return type annotation: None
(ANN201)
855-855: Trailing comma missing
Add trailing comma
(COM812)
862-862: Trailing comma missing
Add trailing comma
(COM812)
890-890: Missing return type annotation for public function test_create_messages_with_file_too_large
Add return type annotation: None
(ANN201)
891-891: Trailing comma missing
Add trailing comma
(COM812)
898-898: Trailing comma missing
Add trailing comma
(COM812)
911-911: Trailing comma missing
Add trailing comma
(COM812)
tests/crud/test_session.py
13-13: Missing return type annotation for public function test_get_session_peer_configuration
Add return type annotation: None
(ANN201)
23-23: Trailing comma missing
Add trailing comma
(COM812)
30-30: Trailing comma missing
Add trailing comma
(COM812)
38-38: Trailing comma missing
Add trailing comma
(COM812)
41-41: Trailing comma missing
Add trailing comma
(COM812)
78-78: Missing return type annotation for public function test_get_session_not_found
Add return type annotation: None
(ANN201)
84-84: Missing return type annotation for public function test_get_peer_config_not_found
Add return type annotation: None
(ANN201)
94-94: Trailing comma missing
Add trailing comma
(COM812)
101-101: Trailing comma missing
Add trailing comma
(COM812)
105-105: Missing return type annotation for public function test_clone_session_not_found
Add return type annotation: None
(ANN201)
111-111: Missing return type annotation for public function test_clone_session_invalid_cutoff_message
Add return type annotation: None
(ANN201)
121-121: Trailing comma missing
Add trailing comma
(COM812)
132-132: Trailing comma missing
Add trailing comma
(COM812)
🪛 LanguageTool
tests/deriver/README.md
[grammar] ~1-~1: Ensure spelling is correct
Context: # Deriver Testing This directory contains tests for the ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~3-~3: Use correct spacing
Context: ... to extract insights and update working representations. ## Structure - conftest.py - Shared fixt...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~7-~7: Use correct spacing
Context: ...ntations. ## Structure - conftest.py - Shared fixtures for deriver testing - `...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~7-~7: There might be a problem here.
Context: ... - conftest.py - Shared fixtures for deriver testing - test_queue_operations.py - Tests for basic queue operations - `tes...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~8-~8: Use correct spacing
Context: ..._operations.py- Tests for basic queue operations -test_deriver_processing.py- Tests for deriver processing logic -t...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~9-~9: Ensure spelling is correct
Context: ...test_deriver_processing.py- Tests for deriver processing logic -test_queue_processi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~9-~9: Use correct spacing
Context: ...sing.py- Tests for deriver processing logic -test_queue_processing.py` - Tests for queue manager and work unit p...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~10-~10: Use correct spacing
Context: ...- Tests for queue manager and work unit processing ## Key Fixtures ### Database Fixtures - `...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~12-~12: Use correct spacing
Context: ...anager and work unit processing ## Key Fixtures ### Database Fixtures - `sample_session_wit...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~14-~14: Use correct spacing
Context: ...ocessing ## Key Fixtures ### Database Fixtures - sample_session_with_peers - Creates a session with multiple peers...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~16-~16: Use hyphens correctly
Context: ...iple peers having different observation configurations - sample_messages - Creates sample messages for testing - ...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~17-~17: Use hyphens correctly
Context: ...messages- Creates sample messages for testing -sample_queue_items` - Creates queue items with various paylo...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~18-~18: There might be a mistake here.
Context: ... various payload types (representation, summary) ### Queue Fixtures - create_queue_payload...
(QB_NEW_EN_OTHER)
[grammar] ~20-~20: Use correct spacing
Context: ...es (representation, summary) ### Queue Fixtures - create_queue_payload - Helper to create queue payloads for t...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~22-~22: Use correct spacing
Context: ...- Helper to create queue payloads for testing -add_queue_items` - Helper to add queue items to the databa...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~23-~23: Use correct spacing
Context: ...ems- Helper to add queue items to the database -create_active_queue_session` - Helper to create active queue sessions ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~24-~24: Use correct spacing
Context: ...ate active queue sessions for work unit tracking ### Mocking Fixtures - `mock_deriver_proces...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~26-~26: Use correct spacing
Context: ...ons for work unit tracking ### Mocking Fixtures - mock_deriver_process - Mocks the deriver process_message met...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~28-~28: Use correct spacing
Context: ...ss- Mocks the deriver process_message method -mock_critical_analysis_call` - Mocks the critical analysis LLM call - ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~29-~29: Use correct spacing
Context: ...call- Mocks the critical analysis LLM call -mock_queue_manager- Mocks the queue manager for testing -...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~30-~30: Use correct spacing
Context: ..._manager- Mocks the queue manager for testing -mock_embedding_store` - Mocks the embedding store operations #...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~31-~31: Use correct spacing
Context: ...ding_store` - Mocks the embedding store operations ## Testing Patterns ### Creating Queue Ite...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~33-~33: Use correct spacing
Context: ... embedding store operations ## Testing Patterns ### Creating Queue Items ```python # Create...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~35-~35: Use correct spacing
Context: ...## Testing Patterns ### Creating Queue Items python # Create representation payloads payload = create_queue_payload( message=message, task_type="representation", sender_name=message.peer_name, target_name=observer_peer.name, ) # Add to queue queue_items = await add_queue_items([payload], session.id) ### Testing Work Units ```python # Create a...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~50-~50: Use correct spacing
Context: ...oad], session.id) ### Testing Work Units python # Create a work unit work_unit = WorkUnit( session_id=session.id, sender_name=sender.name, target_name=target.name, task_type="representation", ) # Test string representation assert str(work_unit) == f"({session.id}, {sender.name}, {target.name}, representation)" ### Mocking Deriver Processing python # ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~65-~65: Ensure spelling is correct
Context: ...ame}, representation)" ### Mocking Deriver Processing python # Use the mock_deriver_proce...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (32)
tests/conftest.py (1)
459-459: Lambda arguments are correctly used for decorator signature.The static analysis warning about unused lambda arguments can be ignored. These arguments are necessary to maintain the correct decorator signature even though they're not used in the lambda body.
tests/crud/test_session.py (4)
13-76: LGTM! Comprehensive test coverage for peer configuration retrieval.The test correctly creates a complex scenario with multiple peers and different configurations, then validates the returned data structure and content. The async/await patterns are properly implemented.
78-82: LGTM! Proper exception testing for non-existent resources.The test correctly verifies that
ResourceNotFoundExceptionis raised when attempting to retrieve a non-existent session.
84-103: LGTM! Well-structured test for peer config not found scenario.The test properly sets up a session and then verifies the exception is raised for a non-existent peer, following good testing practices.
105-134: LGTM! Thorough testing of clone session error scenarios.Both tests properly validate exception handling for non-existent sessions and invalid cutoff message IDs, with appropriate error message matching.
tests/deriver/test_queue_processing.py (5)
15-43: LGTM! Well-structured test for work unit availability.The test properly validates the queue manager's ability to identify available work units and verifies the structure of returned WorkUnit objects.
44-77: LGTM! Comprehensive test for work unit claiming mechanism.The test effectively demonstrates the claiming process by creating an ActiveQueueSession and verifying that claimed work units are no longer available to other workers.
78-100: LGTM! Good test structure for cleanup validation.While the test doesn't create actual stale entries, it ensures the cleanup logic in
get_available_work_unitsdoesn't break the system.
101-116: LGTM! Proper integration test with mocked deriver.The test validates that
process_itemworks correctly with the mocked deriver, ensuring no actual LLM calls are made during testing.
117-153: LGTM! Thorough testing of WorkUnit string representation.The test covers both representation and summary work unit types, ensuring the string representation contains expected information for debugging purposes.
tests/deriver/test_queue_operations.py (4)
13-38: LGTM! Excellent validation of queue item fixture correctness.The test thoroughly validates the expected structure and counts of sample queue items, ensuring the test infrastructure produces consistent data for other tests.
39-50: LGTM! Proper validation of session and peer fixtures.The test ensures the session and peer fixtures are correctly created with the expected relationships.
51-63: LGTM! Good validation of message fixtures.The test confirms that sample messages are created with the required content and peer associations.
64-86: LGTM! Comprehensive test of active queue session creation.The test validates the helper fixture for creating active queue sessions, ensuring all attributes are correctly assigned.
tests/deriver/test_deriver_processing.py (6)
15-30: LGTM! Good validation of deriver process mocking.The test ensures the mock is properly configured and that sample data is available for processing tests.
31-42: LGTM! Proper validation of critical analysis call mocking.The test confirms the critical analysis mock is in place to prevent actual LLM calls during testing.
43-76: LGTM! Comprehensive WorkUnit creation testing.The test thoroughly validates WorkUnit objects for both representation and summary task types, ensuring all attributes are correctly assigned.
77-100: LGTM! Thorough validation of queue manager mocking.The test ensures the mock queue manager has all expected attributes and methods, and validates that mocked methods can be called and track invocations.
101-114: LGTM! Good validation of embedding store mocking.The test confirms the embedding store mock is properly configured and methods can be invoked without making actual embedding operations.
17-18: Fix incorrect noqa directives.The parameters are used as pytest fixtures, so the noqa directive should use ARG002 for unused method arguments, not ARG001.
Apply this diff to fix the noqa directives:
- mock_deriver_process: Callable[..., Any], # noqa: ARG001 - sample_queue_items: list[models.QueueItem], # noqa: ARG001 + mock_deriver_process: Callable[..., Any], # noqa: ARG002 + sample_queue_items: list[models.QueueItem], # noqa: ARG002⛔ Skipped due to learnings
Learnt from: dr-frmr PR: plastic-labs/honcho#142 File: tests/sdk/conftest.py:17-18 Timestamp: 2025-06-26T20:57:43.067Z Learning: In test configuration files, when sys.path modifications are required to make modules discoverable (like adding SDK source paths), the subsequent imports must use # noqa: E402 directives to suppress "module level import not at top of file" warnings. This is a necessary pattern when imports depend on runtime path modifications.tests/deriver/conftest.py (10)
16-29: LGTM! Well-designed mock for deriver processing.The fixture properly mocks the Deriver class's process_message method to prevent actual LLM calls during testing, and returns the mock for further configuration if needed.
31-50: LGTM! Comprehensive mock for critical analysis calls.The fixture creates a properly structured mock response that matches the expected interface, ensuring tests don't make actual LLM calls while maintaining realistic response structure.
52-82: LGTM! Excellent sample session and peer creation.The fixture creates a realistic test scenario with multiple peers and different observation configurations, properly using CRUD operations and async database patterns.
84-132: LGTM! Well-structured sample message creation.The fixture creates diverse message content from different peers and properly handles database commits and queries to ensure consistent test data.
134-163: LGTM! Good helper for queue payload creation.The fixture provides a flexible helper for creating different types of queue payloads with proper validation and structure.
165-194: LGTM! Robust helper for adding queue items.The fixture properly adds queue items to the database with appropriate commits and refreshes to ensure IDs are available for testing.
196-245: LGTM! Comprehensive sample queue item creation.The fixture creates a realistic mix of representation and summary queue items, properly exercising different payload types and peer relationships.
247-270: LGTM! Useful helper for active queue session creation.The fixture provides a convenient way to create active queue sessions for testing work unit tracking functionality.
272-298: LGTM! Comprehensive queue manager mocking.The fixture creates a thorough mock of the QueueManager with all necessary attributes and methods, ensuring tests can run without actual queue processing.
300-310: LGTM! Proper embedding store mocking.The fixture provides appropriate mocking for embedding operations, preventing actual embedding calls during testing.
src/deriver/deriver.py (2)
6-6: LGTM!The Sentry SDK import is properly added to enable tracing decorators for observability enhancements.
80-80: LGTM! Excellent observability enhancement.The addition of
@sentry_sdk.tracedecorators to key asynchronous methods in the deriver processing pipeline will provide valuable performance monitoring and error tracking capabilities. The decorators are consistently placed and cover the critical processing methods:
process_message- Main entry point for message processingprocess_summary_task- Summary generation processingprocess_representation_task- Representation extraction processingderive_new_insights- Critical analysis and insight derivationreason- Core reasoning functionality_save_new_observations- Observation persistencesave_working_representation_to_peer- Working representation persistenceThese changes enhance observability without affecting the functional behavior of the deriver system.
Also applies to: 104-104, 122-122, 341-341, 438-438, 496-496, 569-569
| @pytest.fixture(autouse=True) | ||
| def mock_honcho_llm_call(): | ||
| """Generic mock for the honcho_llm_call decorator to avoid actual LLM calls during tests""" | ||
| from unittest.mock import AsyncMock, MagicMock | ||
|
|
||
| from src.utils.shared_models import ( | ||
| DeductiveObservation, | ||
| ReasoningResponse, | ||
| ReasoningResponseWithThinking, | ||
| SemanticQueries, | ||
| ) | ||
|
|
||
| def create_mock_response( | ||
| response_model: Any = None, | ||
| stream: bool = False, | ||
| return_call_response: bool = False, | ||
| ) -> Any: | ||
| """Create a mock response based on the expected return type""" | ||
| if stream: | ||
| # For streaming responses, return an async mock | ||
| mock_stream = AsyncMock() | ||
| mock_stream.__aiter__.return_value = iter([]) | ||
| return mock_stream | ||
| elif response_model: | ||
| # For structured responses, create appropriate mock objects | ||
| if getattr(response_model, "__name__", "") == "ReasoningResponse": | ||
| mock_response = MagicMock(spec=ReasoningResponse) | ||
| mock_response.explicit = ["Test explicit observation"] | ||
| mock_response.deductive = [ | ||
| DeductiveObservation( | ||
| conclusion="Test deductive conclusion", | ||
| premises=["Test premise 1", "Test premise 2"], | ||
| ) | ||
| ] | ||
| # Add the _response attribute that contains thinking (used in the actual code) | ||
| mock_response._response = MagicMock() | ||
| mock_response._response.thinking = "Test thinking content" | ||
| return mock_response | ||
| elif ( | ||
| getattr(response_model, "__name__", "") | ||
| == "ReasoningResponseWithThinking" | ||
| ): | ||
| mock_response = MagicMock(spec=ReasoningResponseWithThinking) | ||
| mock_response.thinking = "Test thinking content" | ||
| mock_response.explicit = ["Test explicit observation"] | ||
| mock_response.deductive = [ | ||
| DeductiveObservation( | ||
| conclusion="Test deductive conclusion", | ||
| premises=["Test premise 1", "Test premise 2"], | ||
| ) | ||
| ] | ||
| return mock_response | ||
| elif getattr(response_model, "__name__", "") == "SemanticQueries": | ||
| return SemanticQueries(queries=["test query 1", "test query 2"]) | ||
| else: | ||
| # Generic response model mock | ||
| mock_response = MagicMock(spec=response_model) | ||
| # Set some default attributes for common use cases | ||
| if hasattr(mock_response, "content"): | ||
| mock_response.content = "Test response content" | ||
| return mock_response | ||
| elif return_call_response: | ||
| # For CallResponse objects, create a mock with content and usage | ||
| mock_response = MagicMock() | ||
| mock_response.content = "Test response content" | ||
| mock_response.usage = MagicMock() | ||
| mock_response.usage.input_tokens = 100 | ||
| mock_response.usage.output_tokens = 50 | ||
| return mock_response | ||
| else: | ||
| # For string responses, return a simple string | ||
| return "Test response content" | ||
|
|
||
| # Patch the honcho_llm_call decorator to prevent actual LLM calls at module level | ||
| original_decorator = None | ||
| try: | ||
| import src.utils.clients | ||
|
|
||
| original_decorator = src.utils.clients.honcho_llm_call | ||
| src.utils.clients.honcho_llm_call = lambda *args, **kwargs: lambda func: func # pyright: ignore[reportUnknownLambdaType] | ||
| except ImportError: | ||
| pass | ||
|
|
||
| def decorator_factory(*args: Any, **kwargs: Any) -> Callable[..., Any]: # pyright: ignore[reportUnusedParameter] | ||
| """Factory function that creates the mock decorator""" | ||
|
|
||
| def mock_llm_decorator(func: Callable[..., Any]) -> Callable[..., Any]: | ||
| async def async_wrapper(*func_args: Any, **func_kwargs: Any) -> Any: # pyright: ignore[reportUnusedParameter] | ||
| # Create and return appropriate mock response | ||
| return create_mock_response( | ||
| response_model=kwargs.get("response_model"), | ||
| stream=kwargs.get("stream", False), | ||
| return_call_response=kwargs.get("return_call_response", False), | ||
| ) | ||
|
|
||
| def sync_wrapper(*func_args: Any, **func_kwargs: Any) -> Any: # pyright: ignore[reportUnusedParameter] | ||
| # Create and return appropriate mock response | ||
| return create_mock_response( | ||
| response_model=kwargs.get("response_model"), | ||
| stream=kwargs.get("stream", False), | ||
| return_call_response=kwargs.get("return_call_response", False), | ||
| ) | ||
|
|
||
| # Check if the original function is async | ||
| import inspect | ||
|
|
||
| if inspect.iscoroutinefunction(func): | ||
| return async_wrapper | ||
| else: | ||
| return sync_wrapper | ||
|
|
||
| return mock_llm_decorator | ||
|
|
||
| with patch("src.utils.clients.honcho_llm_call", side_effect=decorator_factory): | ||
| yield decorator_factory | ||
|
|
||
| # Restore the original decorator | ||
| if original_decorator: | ||
| try: | ||
| import src.utils.clients | ||
|
|
||
| src.utils.clients.honcho_llm_call = original_decorator | ||
| except ImportError: | ||
| pass | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider reducing complexity by extracting response creation logic.
The fixture has high cyclomatic complexity (17). Consider extracting the response creation logic into separate helper functions to improve maintainability.
+def _create_reasoning_response() -> MagicMock:
+ """Create a mock ReasoningResponse object."""
+ from src.utils.shared_models import DeductiveObservation, ReasoningResponse
+
+ mock_response = MagicMock(spec=ReasoningResponse)
+ mock_response.explicit = ["Test explicit observation"]
+ mock_response.deductive = [
+ DeductiveObservation(
+ conclusion="Test deductive conclusion",
+ premises=["Test premise 1", "Test premise 2"],
+ )
+ ]
+ mock_response._response = MagicMock()
+ mock_response._response.thinking = "Test thinking content"
+ return mock_response
+
+def _create_reasoning_response_with_thinking() -> MagicMock:
+ """Create a mock ReasoningResponseWithThinking object."""
+ from src.utils.shared_models import DeductiveObservation, ReasoningResponseWithThinking
+
+ mock_response = MagicMock(spec=ReasoningResponseWithThinking)
+ mock_response.thinking = "Test thinking content"
+ mock_response.explicit = ["Test explicit observation"]
+ mock_response.deductive = [
+ DeductiveObservation(
+ conclusion="Test deductive conclusion",
+ premises=["Test premise 1", "Test premise 2"],
+ )
+ ]
+ return mock_response
+
@pytest.fixture(autouse=True)
def mock_honcho_llm_call():
"""Generic mock for the honcho_llm_call decorator to avoid actual LLM calls during tests"""
- from unittest.mock import AsyncMock, MagicMock
-
from src.utils.shared_models import (
- DeductiveObservation,
- ReasoningResponse,
- ReasoningResponseWithThinking,
SemanticQueries,
)
def create_mock_response(
response_model: Any = None,
stream: bool = False,
return_call_response: bool = False,
) -> Any:
"""Create a mock response based on the expected return type"""
if stream:
# For streaming responses, return an async mock
mock_stream = AsyncMock()
mock_stream.__aiter__.return_value = iter([])
return mock_stream
elif response_model:
# For structured responses, create appropriate mock objects
if getattr(response_model, "__name__", "") == "ReasoningResponse":
- mock_response = MagicMock(spec=ReasoningResponse)
- mock_response.explicit = ["Test explicit observation"]
- mock_response.deductive = [
- DeductiveObservation(
- conclusion="Test deductive conclusion",
- premises=["Test premise 1", "Test premise 2"],
- )
- ]
- # Add the _response attribute that contains thinking (used in the actual code)
- mock_response._response = MagicMock()
- mock_response._response.thinking = "Test thinking content"
- return mock_response
+ return _create_reasoning_response()
elif (
getattr(response_model, "__name__", "")
== "ReasoningResponseWithThinking"
):
- mock_response = MagicMock(spec=ReasoningResponseWithThinking)
- mock_response.thinking = "Test thinking content"
- mock_response.explicit = ["Test explicit observation"]
- mock_response.deductive = [
- DeductiveObservation(
- conclusion="Test deductive conclusion",
- premises=["Test premise 1", "Test premise 2"],
- )
- ]
- return mock_response
+ return _create_reasoning_response_with_thinking()
elif getattr(response_model, "__name__", "") == "SemanticQueries":
return SemanticQueries(queries=["test query 1", "test query 2"])Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.12.2)
381-381: mock_honcho_llm_call is too complex (17 > 10)
(C901)
381-381: Missing return type annotation for public function mock_honcho_llm_call
(ANN201)
393-393: Dynamically typed expressions (typing.Any) are disallowed in response_model
(ANN401)
394-394: Boolean-typed positional argument in function definition
(FBT001)
394-394: Boolean default positional argument in function definition
(FBT002)
395-395: Boolean-typed positional argument in function definition
(FBT001)
395-395: Boolean default positional argument in function definition
(FBT002)
396-396: Dynamically typed expressions (typing.Any) are disallowed in create_mock_response
(ANN401)
403-403: Unnecessary elif after return statement
Remove unnecessary elif
(RET505)
412-412: Trailing comma missing
Add trailing comma
(COM812)
418-418: Unnecessary elif after return statement
Remove unnecessary elif
(RET505)
429-429: Trailing comma missing
Add trailing comma
(COM812)
459-459: Unused lambda argument: args
(ARG005)
459-459: Unused lambda argument: kwargs
(ARG005)
463-463: Unused function argument: args
(ARG001)
463-463: Dynamically typed expressions (typing.Any) are disallowed in *args
(ANN401)
463-463: Dynamically typed expressions (typing.Any) are disallowed in **kwargs
(ANN401)
467-467: Unused function argument: func_args
(ARG001)
467-467: Dynamically typed expressions (typing.Any) are disallowed in *func_args
(ANN401)
467-467: Unused function argument: func_kwargs
(ARG001)
467-467: Dynamically typed expressions (typing.Any) are disallowed in **func_kwargs
(ANN401)
467-467: Dynamically typed expressions (typing.Any) are disallowed in async_wrapper
(ANN401)
475-475: Unused function argument: func_args
(ARG001)
475-475: Dynamically typed expressions (typing.Any) are disallowed in *func_args
(ANN401)
475-475: Unused function argument: func_kwargs
(ARG001)
475-475: Dynamically typed expressions (typing.Any) are disallowed in **func_kwargs
(ANN401)
475-475: Dynamically typed expressions (typing.Any) are disallowed in sync_wrapper
(ANN401)
488-488: Unnecessary else after return statement
Remove unnecessary else
(RET505)
🤖 Prompt for AI Agents
In tests/conftest.py between lines 380 and 504, the mock_honcho_llm_call fixture
has high cyclomatic complexity due to the large create_mock_response function
handling multiple response types. To fix this, refactor by extracting the
response creation logic for each response model type into separate helper
functions. Then, call these helpers from create_mock_response to simplify its
structure and reduce complexity, improving readability and maintainability.
| from unittest.mock import AsyncMock, MagicMock | ||
|
|
There was a problem hiding this comment.
Remove duplicate imports.
AsyncMock and MagicMock are already imported at the top of the file (line 4).
- from unittest.mock import AsyncMock, MagicMock
-📝 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.
| from unittest.mock import AsyncMock, MagicMock |
🤖 Prompt for AI Agents
In tests/conftest.py around lines 383 to 384, remove the duplicate import
statement for AsyncMock and MagicMock since they are already imported at the top
of the file on line 4. This will clean up the imports and avoid redundancy.
There was a problem hiding this comment.
@dr-frmr Thank you for confirming! You're absolutely right - those duplicate imports should be removed since AsyncMock and MagicMock are already imported at the top of the file on line 4.
The lines 383-384 can be safely deleted:
from unittest.mock import AsyncMock, MagicMockThis will clean up the code and eliminate the redundancy without affecting functionality.
🐰
^_^
( )
) (
/ \
🧠 Learnings used
Learnt from: dr-frmr
PR: plastic-labs/honcho#142
File: tests/sdk/conftest.py:17-18
Timestamp: 2025-06-26T20:57:43.067Z
Learning: In test configuration files, when sys.path modifications are required to make modules discoverable (like adding SDK source paths), the subsequent imports must use # noqa: E402 directives to suppress "module level import not at top of file" warnings. This is a necessary pattern when imports depend on runtime path modifications.
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to tests/**/*.py : Tests in pytest with fixtures in tests/conftest.py
| ) | ||
| messages = list(result.scalars().all()) | ||
|
|
||
| return messages |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: Simplify return statements.
The static analysis identifies unnecessary assignments before return statements. These can be simplified, though the current form is also acceptable for readability.
Apply this diff to simplify return statements (optional):
- messages = list(result.scalars().all())
-
- return messages
+ return list(result.scalars().all())
- queue_items = await add_queue_items(payloads, session.id)
-
- return queue_items
+ return await add_queue_items(payloads, session.id)Also applies to: 244-244
🧰 Tools
🪛 Ruff (0.12.2)
131-131: Unnecessary assignment to messages before return statement
Remove unnecessary assignment
(RET504)
🤖 Prompt for AI Agents
In tests/deriver/conftest.py at lines 131 and 244, simplify the return
statements by removing any unnecessary intermediate variable assignments and
returning the value directly. This reduces redundant code while maintaining
readability.
There was a problem hiding this comment.
@dr-frmr Thank you for the approval! The suggested simplifications will help reduce redundant code while maintaining readability.
🧠 Learnings used
Learnt from: Rajat-Ahuja1997
PR: plastic-labs/honcho#131
File: tests/routes/test_messages.py:7-39
Timestamp: 2025-06-18T15:47:44.327Z
Learning: In FastAPI tests, it's common to use pytest.mark.asyncio with a synchronous TestClient. The async marker is used when tests contain async database operations (like await db_session.commit()), but the HTTP client calls (client.post(), client.get()) should NOT be awaited since TestClient is synchronous. Only the actual async operations like database commits need awaiting.
Learnt from: CR
PR: plastic-labs/honcho#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-21T14:51:09.913Z
Learning: Applies to tests/**/*.py : Tests in pytest with fixtures in tests/conftest.py
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_queue_manager(monkeypatch: pytest.MonkeyPatch) -> AsyncMock: # pyright: ignore[reportUnusedParameter] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused monkeypatch parameters.
The monkeypatch parameters are not used in these fixtures. They can be removed to address the static analysis warnings.
Apply this diff to remove unused parameters:
-def mock_queue_manager(monkeypatch: pytest.MonkeyPatch) -> AsyncMock: # pyright: ignore[reportUnusedParameter]
+def mock_queue_manager() -> AsyncMock:
-def mock_embedding_store(monkeypatch: pytest.MonkeyPatch) -> AsyncMock: # pyright: ignore[reportUnusedParameter]
+def mock_embedding_store() -> AsyncMock:Also applies to: 301-301
🧰 Tools
🪛 Ruff (0.12.2)
273-273: Unused function argument: monkeypatch
(ARG001)
🤖 Prompt for AI Agents
In tests/deriver/conftest.py at lines 273 and 301, the monkeypatch parameter in
the mock_queue_manager fixture is unused and causes static analysis warnings.
Remove the monkeypatch parameter from the function signature in both places to
fix the issue.
| async def test_work_unit_claiming( | ||
| self, | ||
| db_session: AsyncSession, | ||
| sample_queue_items: list[models.QueueItem], # noqa: ARG001 # pyright: ignore[reportUnusedParameter] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix incorrect noqa directives.
The static analysis correctly identifies that the noqa directives are using the wrong error codes. The parameters are actually used as pytest fixtures.
Apply this diff to fix the noqa directives:
- sample_queue_items: list[models.QueueItem], # noqa: ARG001 # pyright: ignore[reportUnusedParameter]
+ sample_queue_items: list[models.QueueItem], # noqa: ARG002 # pyright: ignore[reportUnusedParameter]
- mock_deriver_process: Any, # noqa: ARG001 # pyright: ignore[reportUnusedParameter]
+ mock_deriver_process: Any, # noqa: ARG002 # pyright: ignore[reportUnusedParameter]Also applies to: 103-103
🧰 Tools
🪛 Ruff (0.12.2)
47-47: Unused method argument: sample_queue_items
(ARG002)
47-47: Unused noqa directive (unused: ARG001)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In tests/deriver/test_queue_processing.py at lines 47 and 103, the noqa
directives on the parameters are incorrect because these parameters are used as
pytest fixtures and should not be ignored for unused argument errors. Remove or
correct the noqa directives to reflect that these parameters are intentionally
used, ensuring static analysis tools do not mistakenly suppress relevant
warnings.
| def test_update_peer_all_fields( | ||
| client: TestClient, sample_data: tuple[Workspace, Peer] | ||
| ): | ||
| """Test peer update with all possible fields""" | ||
| test_workspace, test_peer = sample_data | ||
|
|
||
| # Test updating both metadata and configuration | ||
| metadata = {"updated": True, "version": 2} | ||
| configuration = {"features": {"new_feature": True}} | ||
|
|
||
| response = client.put( | ||
| f"/v2/workspaces/{test_workspace.name}/peers/{test_peer.name}", | ||
| json={"metadata": metadata, "configuration": configuration}, | ||
| ) | ||
| assert response.status_code == 200 | ||
| data = response.json() | ||
| assert data["metadata"] == metadata | ||
| assert data["configuration"] == configuration |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Good test coverage for updating multiple fields. Add return type annotation.
The test correctly validates that both metadata and configuration can be updated in a single request.
def test_update_peer_all_fields(
client: TestClient, sample_data: tuple[Workspace, Peer]
-):
+) -> None:📝 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.
| def test_update_peer_all_fields( | |
| client: TestClient, sample_data: tuple[Workspace, Peer] | |
| ): | |
| """Test peer update with all possible fields""" | |
| test_workspace, test_peer = sample_data | |
| # Test updating both metadata and configuration | |
| metadata = {"updated": True, "version": 2} | |
| configuration = {"features": {"new_feature": True}} | |
| response = client.put( | |
| f"/v2/workspaces/{test_workspace.name}/peers/{test_peer.name}", | |
| json={"metadata": metadata, "configuration": configuration}, | |
| ) | |
| assert response.status_code == 200 | |
| data = response.json() | |
| assert data["metadata"] == metadata | |
| assert data["configuration"] == configuration | |
| def test_update_peer_all_fields( | |
| client: TestClient, sample_data: tuple[Workspace, Peer] | |
| ) -> None: | |
| """Test peer update with all possible fields""" | |
| test_workspace, test_peer = sample_data | |
| # Test updating both metadata and configuration | |
| metadata = {"updated": True, "version": 2} | |
| configuration = {"features": {"new_feature": True}} | |
| response = client.put( | |
| f"/v2/workspaces/{test_workspace.name}/peers/{test_peer.name}", | |
| json={"metadata": metadata, "configuration": configuration}, | |
| ) | |
| assert response.status_code == 200 | |
| data = response.json() | |
| assert data["metadata"] == metadata | |
| assert data["configuration"] == configuration |
🧰 Tools
🪛 Ruff (0.12.2)
544-544: Missing return type annotation for public function test_update_peer_all_fields
Add return type annotation: None
(ANN201)
545-545: Trailing comma missing
Add trailing comma
(COM812)
🤖 Prompt for AI Agents
In tests/routes/test_peers.py around lines 544 to 561, the
test_update_peer_all_fields function lacks a return type annotation. Add the
return type annotation -> None to the function definition to explicitly indicate
it does not return a value.
dr-frmr
left a comment
There was a problem hiding this comment.
Approved but 1 or 2 of the coderabbit suggestions are good
* fix (deriver): Add Sentry decorators and some additional tests * feat (test): Deriver Fixtures for Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation