Changes: Create a background task monitor_generation_time
#86
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…time` inside `app/tasks/` that periodically checks active report timers. If
WalkthroughThe pull request migrates test infrastructure from direct in-memory state management to mock-based repository interactions. Changes include adding Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/app/services/report_processor.py(1 hunks)backend/tests/test_orchestrator_integration.py(4 hunks)backend/tests/test_routes.py(0 hunks)
💤 Files with no reviewable changes (1)
- backend/tests/test_routes.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/test_orchestrator_integration.py (4)
backend/app/db/models/report_state.py (1)
ReportStatusEnum(8-20)backend/tests/test_orchestrator_updates.py (2)
orchestrator(31-34)mock_report_repository(24-28)backend/app/core/orchestrator.py (1)
create_orchestrator(102-305)backend/app/db/repositories/report_repository.py (2)
get_report_by_id(72-76)update_partial(90-100)
🔇 Additional comments (4)
backend/app/services/report_processor.py (1)
4-4: AsyncSession import is consistent with session-factory based orchestrationImporting
AsyncSessionhere is fine as preparation for Async DB session wiring and keeps this module aligned with the orchestrator/session_factory changes; no further changes needed in this file.backend/tests/test_orchestrator_integration.py (3)
92-98: Repository interaction assertions look good once mocking is fixedThe assertions on
get_report_by_idandupdate_partialcorrectly verify that the orchestrator updates the report toAGENTS_COMPLETEDwith the expected report ID. After fixing the mocking as suggested above, this block should behave as intended.Please re-run this test after applying the mocking fix to confirm the final
update_partialcall indeed carriesstatus == ReportStatusEnum.AGENTS_COMPLETED.
160-166: Timeout-path repository assertions are appropriateThese assertions correctly check that:
- The report lookup uses the right
report_id.- The final
update_partialcall setsstatustoAGENTS_FAILEDand includes anonchain_data_agententry inerrors.Once the repository methods are mocked with
AsyncMockas above, this should accurately validate timeout handling.After fixing the mocks, verify that
update_partial.call_args_list[-1].args[1]actually contains the expectederrorsstructure in practice.
227-233: Exception-path assertions correctly validate AGENTS_FAILED + errorsThese checks mirror the timeout test and look correct: they assert that the orchestrator marks the report as
AGENTS_FAILEDand includes an error entry foronchain_data_agent. With the mocking adjustments applied, this will robustly validate exception handling behavior.Please re-run the exception-handling test after updating the mocks to ensure the last
update_partialcall indeed carries the expectedstatusanderrors.
Comment on lines
+50
to
+78
| # Setup mock for ReportRepository and its interactions | ||
| from backend.app.db.models.report_state import ReportStatusEnum | ||
|
|
||
| mock_session = AsyncMock() | ||
| # Mock the async context manager for the session factory | ||
| mock_session_factory = AsyncMock(return_value=AsyncMock(__aenter__=AsyncMock(return_value=mock_session))) | ||
|
|
||
| orchestrator = await create_orchestrator(session_factory=mock_session_factory) | ||
| mock_report_repository = orchestrator.report_repository | ||
|
|
||
| # Mock initial report state (PENDING) | ||
| mock_report_state_pending = AsyncMock() | ||
| mock_report_state_pending.report_id = SAMPLE_REPORT_ID | ||
| mock_report_state_pending.status = ReportStatusEnum.PENDING | ||
| mock_report_state_pending.partial_agent_output = {} | ||
|
|
||
| # Mock final report state (COMPLETED) | ||
| mock_report_state_completed = AsyncMock() | ||
| mock_report_state_completed.report_id = SAMPLE_REPORT_ID | ||
| mock_report_state_completed.status = ReportStatusEnum.AGENTS_COMPLETED | ||
| mock_report_state_completed.partial_agent_output = {} # Will be updated | ||
|
|
||
| # Configure get_report_by_id to return pending then completed after update | ||
| mock_report_repository.get_report_by_id.side_effect = [ | ||
| mock_report_state_pending, # First call during initial status check | ||
| mock_report_state_completed # Second call after update_partial for AGENTS_COMPLETED | ||
| ] | ||
| mock_report_repository.update_partial.return_value = mock_report_state_completed # Ensure update returns something | ||
|
|
There was a problem hiding this comment.
Mocking ReportRepository methods via .side_effect/.return_value on real methods will break
Here orchestrator.report_repository is a real ReportRepository instance; its get_report_by_id / update_partial are bound async methods, not mocks. Accessing .side_effect/.return_value on them will not work and will likely raise AttributeError.
Override these attributes with AsyncMock instances instead:
@@
- orchestrator = await create_orchestrator(session_factory=mock_session_factory)
- mock_report_repository = orchestrator.report_repository
+ orchestrator = await create_orchestrator(session_factory=mock_session_factory)
+ mock_report_repository = orchestrator.report_repository
@@
- # Configure get_report_by_id to return pending then completed after update
- mock_report_repository.get_report_by_id.side_effect = [
- mock_report_state_pending, # First call during initial status check
- mock_report_state_completed # Second call after update_partial for AGENTS_COMPLETED
- ]
- mock_report_repository.update_partial.return_value = mock_report_state_completed # Ensure update returns something
+ # Configure get_report_by_id to return pending then completed after update
+ mock_report_repository.get_report_by_id = AsyncMock(side_effect=[
+ mock_report_state_pending, # First call during initial status check
+ mock_report_state_completed # Second call after update_partial for AGENTS_COMPLETED
+ ])
+ # Ensure update_partial is a mock and returns the completed state
+ mock_report_repository.update_partial = AsyncMock(return_value=mock_report_state_completed)This keeps the orchestrator instance but safely mocks just the repository calls used in the test.
📝 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.
Suggested change
| # Setup mock for ReportRepository and its interactions | |
| from backend.app.db.models.report_state import ReportStatusEnum | |
| mock_session = AsyncMock() | |
| # Mock the async context manager for the session factory | |
| mock_session_factory = AsyncMock(return_value=AsyncMock(__aenter__=AsyncMock(return_value=mock_session))) | |
| orchestrator = await create_orchestrator(session_factory=mock_session_factory) | |
| mock_report_repository = orchestrator.report_repository | |
| # Mock initial report state (PENDING) | |
| mock_report_state_pending = AsyncMock() | |
| mock_report_state_pending.report_id = SAMPLE_REPORT_ID | |
| mock_report_state_pending.status = ReportStatusEnum.PENDING | |
| mock_report_state_pending.partial_agent_output = {} | |
| # Mock final report state (COMPLETED) | |
| mock_report_state_completed = AsyncMock() | |
| mock_report_state_completed.report_id = SAMPLE_REPORT_ID | |
| mock_report_state_completed.status = ReportStatusEnum.AGENTS_COMPLETED | |
| mock_report_state_completed.partial_agent_output = {} # Will be updated | |
| # Configure get_report_by_id to return pending then completed after update | |
| mock_report_repository.get_report_by_id.side_effect = [ | |
| mock_report_state_pending, # First call during initial status check | |
| mock_report_state_completed # Second call after update_partial for AGENTS_COMPLETED | |
| ] | |
| mock_report_repository.update_partial.return_value = mock_report_state_completed # Ensure update returns something | |
| # Setup mock for ReportRepository and its interactions | |
| from backend.app.db.models.report_state import ReportStatusEnum | |
| mock_session = AsyncMock() | |
| # Mock the async context manager for the session factory | |
| mock_session_factory = AsyncMock(return_value=AsyncMock(__aenter__=AsyncMock(return_value=mock_session))) | |
| orchestrator = await create_orchestrator(session_factory=mock_session_factory) | |
| mock_report_repository = orchestrator.report_repository | |
| # Mock initial report state (PENDING) | |
| mock_report_state_pending = AsyncMock() | |
| mock_report_state_pending.report_id = SAMPLE_REPORT_ID | |
| mock_report_state_pending.status = ReportStatusEnum.PENDING | |
| mock_report_state_pending.partial_agent_output = {} | |
| # Mock final report state (COMPLETED) | |
| mock_report_state_completed = AsyncMock() | |
| mock_report_state_completed.report_id = SAMPLE_REPORT_ID | |
| mock_report_state_completed.status = ReportStatusEnum.AGENTS_COMPLETED | |
| mock_report_state_completed.partial_agent_output = {} # Will be updated | |
| # Configure get_report_by_id to return pending then completed after update | |
| mock_report_repository.get_report_by_id = AsyncMock(side_effect=[ | |
| mock_report_state_pending, # First call during initial status check | |
| mock_report_state_completed # Second call after update_partial for AGENTS_COMPLETED | |
| ]) | |
| # Ensure update_partial is a mock and returns the completed state | |
| mock_report_repository.update_partial = AsyncMock(return_value=mock_report_state_completed) |
🤖 Prompt for AI Agents
In backend/tests/test_orchestrator_integration.py around lines 50 to 78, the
test sets .side_effect and .return_value on real bound async methods of
orchestrator.report_repository which will fail; replace those bound methods with
AsyncMock instances on the repository before assigning side_effect/return_value
(e.g., orchestrator.report_repository.get_report_by_id = AsyncMock(...) and
orchestrator.report_repository.update_partial = AsyncMock(...)), then configure
their side_effect and return_value as needed so the orchestrator instance
remains but the repository calls are properly mocked.
Comment on lines
+128
to
+155
| # Setup mock for ReportRepository and its interactions | ||
| from backend.app.db.models.report_state import ReportStatusEnum | ||
| mock_session = AsyncMock() | ||
| mock_session_factory = AsyncMock(return_value=AsyncMock(__aenter__=AsyncMock(return_value=mock_session))) | ||
|
|
||
| orchestrator = await create_orchestrator(session_factory=mock_session_factory) | ||
| mock_report_repository = orchestrator.report_repository | ||
|
|
||
| # Mock initial report state (PENDING) | ||
| mock_report_state_pending = AsyncMock() | ||
| mock_report_state_pending.report_id = SAMPLE_REPORT_ID | ||
| mock_report_state_pending.status = ReportStatusEnum.PENDING | ||
| mock_report_state_pending.partial_agent_output = {} | ||
|
|
||
| # Mock failed report state | ||
| mock_report_state_failed = AsyncMock() | ||
| mock_report_state_failed.report_id = SAMPLE_REPORT_ID | ||
| mock_report_state_failed.status = ReportStatusEnum.AGENTS_FAILED | ||
| mock_report_state_failed.partial_agent_output = {} | ||
| mock_report_state_failed.errors = {"onchain_data_agent": True} | ||
|
|
||
| # Configure get_report_by_id to return pending then failed after update | ||
| mock_report_repository.get_report_by_id.side_effect = [ | ||
| mock_report_state_pending, # First call during initial status check | ||
| mock_report_state_failed # Second call after update_partial for AGENTS_FAILED | ||
| ] | ||
| mock_report_repository.update_partial.return_value = mock_report_state_failed | ||
|
|
There was a problem hiding this comment.
Same mocking issue for timeout test: methods must be replaced with AsyncMock
This block repeats the same pattern: mock_report_repository.get_report_by_id.side_effect = [...] and mock_report_repository.update_partial.return_value = ... on real async methods. Use AsyncMock assignments instead, analogous to the success test:
@@
- orchestrator = await create_orchestrator(session_factory=mock_session_factory)
- mock_report_repository = orchestrator.report_repository
+ orchestrator = await create_orchestrator(session_factory=mock_session_factory)
+ mock_report_repository = orchestrator.report_repository
@@
- # Configure get_report_by_id to return pending then failed after update
- mock_report_repository.get_report_by_id.side_effect = [
- mock_report_state_pending, # First call during initial status check
- mock_report_state_failed # Second call after update_partial for AGENTS_FAILED
- ]
- mock_report_repository.update_partial.return_value = mock_report_state_failed
+ # Configure get_report_by_id to return pending then failed after update
+ mock_report_repository.get_report_by_id = AsyncMock(side_effect=[
+ mock_report_state_pending, # First call during initial status check
+ mock_report_state_failed # Second call after update_partial for AGENTS_FAILED
+ ])
+ mock_report_repository.update_partial = AsyncMock(return_value=mock_report_state_failed)Without this, the test will not run reliably.
🤖 Prompt for AI Agents
In backend/tests/test_orchestrator_integration.py around lines 128 to 155, the
test assigns side_effect and return_value directly to coroutine methods on the
real mock_report_repository which are async and must be replaced with AsyncMock
instances; change the assignments so mock_report_repository.get_report_by_id =
AsyncMock(side_effect=[mock_report_state_pending, mock_report_state_failed]) and
mock_report_repository.update_partial =
AsyncMock(return_value=mock_report_state_failed) (and apply the same AsyncMock
replacement pattern to any other async repository methods used in this timeout
test) so the async methods behave correctly in the test.
Comment on lines
+195
to
+221
| # Setup mock for ReportRepository and its interactions | ||
| from backend.app.db.models.report_state import ReportStatusEnum | ||
| mock_session = AsyncMock() | ||
| mock_session_factory = AsyncMock(return_value=AsyncMock(__aenter__=AsyncMock(return_value=mock_session))) | ||
|
|
||
| orchestrator = await create_orchestrator(session_factory=mock_session_factory) | ||
| mock_report_repository = orchestrator.report_repository | ||
|
|
||
| # Mock initial report state (PENDING) | ||
| mock_report_state_pending = AsyncMock() | ||
| mock_report_state_pending.report_id = SAMPLE_REPORT_ID | ||
| mock_report_state_pending.status = ReportStatusEnum.PENDING | ||
| mock_report_state_pending.partial_agent_output = {} | ||
|
|
||
| # Mock failed report state | ||
| mock_report_state_failed = AsyncMock() | ||
| mock_report_state_failed.report_id = SAMPLE_REPORT_ID | ||
| mock_report_state_failed.status = ReportStatusEnum.AGENTS_FAILED | ||
| mock_report_state_failed.partial_agent_output = {} | ||
| mock_report_state_failed.errors = {"onchain_data_agent": True} | ||
|
|
||
| # Configure get_report_by_id to return pending then failed after update | ||
| mock_report_repository.get_report_by_id.side_effect = [ | ||
| mock_report_state_pending, # First call during initial status check | ||
| mock_report_state_failed # Second call after update_partial for AGENTS_FAILED | ||
| ] | ||
| mock_report_repository.update_partial.return_value = mock_report_state_failed |
There was a problem hiding this comment.
Repeat of mocking issue for exception-handling test
Same concern here: get_report_by_id and update_partial are real async methods, so setting .side_effect / .return_value on them is unsafe. Mirror the AsyncMock pattern:
@@
- orchestrator = await create_orchestrator(session_factory=mock_session_factory)
- mock_report_repository = orchestrator.report_repository
+ orchestrator = await create_orchestrator(session_factory=mock_session_factory)
+ mock_report_repository = orchestrator.report_repository
@@
- # Configure get_report_by_id to return pending then failed after update
- mock_report_repository.get_report_by_id.side_effect = [
- mock_report_state_pending, # First call during initial status check
- mock_report_state_failed # Second call after update_partial for AGENTS_FAILED
- ]
- mock_report_repository.update_partial.return_value = mock_report_state_failed
+ # Configure get_report_by_id to return pending then failed after update
+ mock_report_repository.get_report_by_id = AsyncMock(side_effect=[
+ mock_report_state_pending, # First call during initial status check
+ mock_report_state_failed # Second call after update_partial for AGENTS_FAILED
+ ])
+ mock_report_repository.update_partial = AsyncMock(return_value=mock_report_state_failed)This keeps all three tests consistent and prevents AttributeErrors when configuring mocks.
📝 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.
Suggested change
| # Setup mock for ReportRepository and its interactions | |
| from backend.app.db.models.report_state import ReportStatusEnum | |
| mock_session = AsyncMock() | |
| mock_session_factory = AsyncMock(return_value=AsyncMock(__aenter__=AsyncMock(return_value=mock_session))) | |
| orchestrator = await create_orchestrator(session_factory=mock_session_factory) | |
| mock_report_repository = orchestrator.report_repository | |
| # Mock initial report state (PENDING) | |
| mock_report_state_pending = AsyncMock() | |
| mock_report_state_pending.report_id = SAMPLE_REPORT_ID | |
| mock_report_state_pending.status = ReportStatusEnum.PENDING | |
| mock_report_state_pending.partial_agent_output = {} | |
| # Mock failed report state | |
| mock_report_state_failed = AsyncMock() | |
| mock_report_state_failed.report_id = SAMPLE_REPORT_ID | |
| mock_report_state_failed.status = ReportStatusEnum.AGENTS_FAILED | |
| mock_report_state_failed.partial_agent_output = {} | |
| mock_report_state_failed.errors = {"onchain_data_agent": True} | |
| # Configure get_report_by_id to return pending then failed after update | |
| mock_report_repository.get_report_by_id.side_effect = [ | |
| mock_report_state_pending, # First call during initial status check | |
| mock_report_state_failed # Second call after update_partial for AGENTS_FAILED | |
| ] | |
| mock_report_repository.update_partial.return_value = mock_report_state_failed | |
| # Setup mock for ReportRepository and its interactions | |
| from backend.app.db.models.report_state import ReportStatusEnum | |
| mock_session = AsyncMock() | |
| mock_session_factory = AsyncMock(return_value=AsyncMock(__aenter__=AsyncMock(return_value=mock_session))) | |
| orchestrator = await create_orchestrator(session_factory=mock_session_factory) | |
| mock_report_repository = orchestrator.report_repository | |
| # Mock initial report state (PENDING) | |
| mock_report_state_pending = AsyncMock() | |
| mock_report_state_pending.report_id = SAMPLE_REPORT_ID | |
| mock_report_state_pending.status = ReportStatusEnum.PENDING | |
| mock_report_state_pending.partial_agent_output = {} | |
| # Mock failed report state | |
| mock_report_state_failed = AsyncMock() | |
| mock_report_state_failed.report_id = SAMPLE_REPORT_ID | |
| mock_report_state_failed.status = ReportStatusEnum.AGENTS_FAILED | |
| mock_report_state_failed.partial_agent_output = {} | |
| mock_report_state_failed.errors = {"onchain_data_agent": True} | |
| # Configure get_report_by_id to return pending then failed after update | |
| mock_report_repository.get_report_by_id = AsyncMock(side_effect=[ | |
| mock_report_state_pending, # First call during initial status check | |
| mock_report_state_failed # Second call after update_partial for AGENTS_FAILED | |
| ]) | |
| mock_report_repository.update_partial = AsyncMock(return_value=mock_report_state_failed) |
🤖 Prompt for AI Agents
In backend/tests/test_orchestrator_integration.py around lines 195 to 221, the
test assigns .side_effect and .return_value directly on real async methods which
can raise AttributeErrors; change the setup so
mock_report_repository.get_report_by_id and
mock_report_repository.update_partial are AsyncMock instances (use
AsyncMock(side_effect=[...]) for get_report_by_id and
AsyncMock(return_value=...) for update_partial) mirroring the pattern used
elsewhere in the tests, and ensure the mock_session_factory/mock_session usage
remains an AsyncMock context manager as currently done.
felixjordandev
approved these changes
Dec 3, 2025
Collaborator
|
that background task for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes requested:
Create a background task
monitor_generation_timeinsideapp/tasks/that periodically checks active report timers. If a report is taking too long, log an alert and update the report state accordingly.Please review.
Summary by CodeRabbit
Tests
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.