fix(tests): align framework and datastore mocks with current APIs#1665
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTwo unit-test modules change their mocking targets: session datastore tests patch ChangesTest Mocking Strategy Refactors
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates unit tests for the AI UI datastore and framework validators. In test_aiui_datastore.py, the patch target for the session store was modified and the implementation class is now reset during the test. In test_framework_validators.py, the tests were refactored to patch get_default_registry directly instead of using FrameworkAdapterRegistry.get_instance. Feedback was provided regarding the risk of modifying global state in tests and a suggestion was made to use patch.object for safer state management, along with a reminder to verify the correct patch target location.
| with patch("praisonaiagents.session.get_hierarchical_session_store") as mock_get_store: | ||
| mock_store = Mock() | ||
| mock_get_store.return_value = mock_store | ||
|
|
||
| _ds._impl_cls = None |
There was a problem hiding this comment.
Directly setting _ds._impl_cls = None modifies the module's global state, which can lead to test interference and flakiness. It is better to use patch.object to ensure the state is automatically restored after the test block.
Also, verify the patch target "praisonaiagents.session.get_hierarchical_session_store". If the module under test (_ds) uses from praisonaiagents.session import get_hierarchical_session_store, patching the source module will not affect the reference already imported into _ds. In that case, you should patch the usage point: "praisonai.ui._aiui_datastore.get_hierarchical_session_store".
| with patch("praisonaiagents.session.get_hierarchical_session_store") as mock_get_store: | |
| mock_store = Mock() | |
| mock_get_store.return_value = mock_store | |
| _ds._impl_cls = None | |
| with patch("praisonaiagents.session.get_hierarchical_session_store") as mock_get_store, \ | |
| patch.object(_ds, "_impl_cls", None): | |
| mock_store = Mock() | |
| mock_get_store.return_value = mock_store |
Agent-Logs-Url: https://github.com/MervinPraison/PraisonAI/sessions/2a49bd60-577c-45b9-8236-3ee18f853a01 Co-authored-by: MervinPraison <454862+MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates unit tests to match the current registry and session-store APIs after upstream refactors (removal of FrameworkAdapterRegistry.get_instance and changes to how the default hierarchical session store is accessed).
Changes:
- Update framework validator tests to patch
get_default_registry()instead of the removedFrameworkAdapterRegistry.get_instance(). - Update aiui datastore adapter tests to patch
praisonaiagents.session.get_hierarchical_session_storeand reset the cached datastore implementation class to exercise the default-store code path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/praisonai/tests/unit/test_framework_validators.py | Replaces registry mocking approach to align with get_default_registry() usage in validators. |
| src/praisonai/tests/unit/test_aiui_datastore.py | Adjusts datastore mocking to target the current session-store getter and forces rebuilding the cached impl class for the test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import praisonai.ui._aiui_datastore as _ds | ||
|
|
||
| with patch("praisonaiagents.session.get_hierarchical_session_store") as mock_get_store, \ | ||
| patch.object(_ds, "_impl_cls", None): | ||
| mock_store = Mock() | ||
| mock_get_store.return_value = mock_store | ||
|
|
||
| adapter = PraisonAISessionDataStore() |
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
Completed the review and applied the only actionable finding from existing bot feedback in commit
Review results:
with patch("praisonaiagents.session.get_hierarchical_session_store") as mock_get_store, \
patch.object(_ds, "_impl_cls", None):
...I also checked Actions runs for this branch; many push workflows are marked failure with no jobs created, so there were no job logs to analyze for test failures. |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅Phase 1: Architecture Review per AGENTS.md
Phase 2: Fix Valid Issues
Phase 3: Final Verdict
Architecture Review Results ✅1. Protocol-Driven Design Compliance ✅
2. Backward Compatibility ✅
3. Performance Impact ✅
Issue Resolution ✅Previous Reviewer Issues Addressed:
Test Changes Validation ✅File:
File:
Final Verdict: APPROVED ✅This PR successfully aligns test mocks with current API implementations:
Recommendation: MERGE this PR. All changes are technically sound and improve test reliability. |
Greptile SummaryThis PR fixes two unit test files to align their mocking strategies with the current production API. The old tests patched a removed
Confidence Score: 5/5Test-only changes that correct previously broken mock targets; no production code is modified. Both changes are narrowly scoped to unit test files. The new mocking approach correctly patches get_default_registry where the source code calls it, and resets _impl_cls to ensure _build_impl_cls() re-executes and captures the mock via the from X import Y closure — which is the right technique for this pattern. No edge cases or regressions are introduced. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test
participant DS as _aiui_datastore module
participant PA as praisonaiagents.session
Note over T: test_init_with_default_store
T->>DS: patch.object(_ds, "_impl_cls", None)
T->>PA: patch("...get_hierarchical_session_store") → mock_store
T->>DS: PraisonAISessionDataStore()
DS->>DS: __new__ → _build_impl_cls()
DS->>DS: _impl_cls is None → proceed
DS->>PA: from praisonaiagents.session import get_hierarchical_session_store
PA-->>DS: returns mock (captured in closure)
DS->>DS: create _PraisonAISessionDataStoreImpl class
DS->>DS: "_impl_cls = new class"
DS->>DS: "impl_cls(store=None)"
DS->>PA: get_hierarchical_session_store() → mock_store
DS-->>T: "adapter._store == mock_store ✓"
Note over T,DS: patch.object restores original _impl_cls on exit
Note over T: test_framework_validators
T->>T: patch("...validators.get_default_registry") → mock_registry
T->>T: assert_framework_available("crewai")
T->>T: "registry = get_default_registry() → mock_registry"
T->>T: registry.is_available("crewai") → False
T->>T: raises ImportError ✓
Reviews (1): Last reviewed commit: "test: scope datastore impl cache reset v..." | Re-trigger Greptile |
MervinPraison
deleted the
fix/tests-stale-mock-targets-framework-and-datastore
branch
get_default_registryinstead of removedFrameworkAdapterRegistry.get_instance.\n- Patchpraisonaiagents.session.get_hierarchical_session_storeand reset_impl_clsso the default-store path is exercised correctly.Made with Cursor
Summary by CodeRabbit