fix(tests): BotOS integration tests use registry unregister#1669
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)
📝 WalkthroughWalkthroughAdds a module-private ChangesTest Registry Cleanup Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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:
|
Greptile SummaryThis PR fixes integration tests that broke when the bot registry was migrated from a plain
Confidence Score: 5/5Safe to merge — the change is confined to test cleanup logic, removes a dependency on a removed private API, and the new error-message regex correctly matches the live PluginRegistry output. Both the new unregister path and the updated regex are straightforward adaptations to the PluginRegistry migration. The singleton shared between register_platform and _unregister_bot_platform is the same object, so registration and cleanup operate on identical state. No production code is touched. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as Test (setup_method)
participant RP as register_platform()
participant DR as get_default_bot_registry()
participant PR as BotPlatformRegistry (singleton)
Test->>RP: register_platform("mockbot", MockPlatformAdapter)
RP->>DR: get_default_bot_registry()
DR-->>RP: BotPlatformRegistry instance
RP->>PR: registry.register("mockbot", cls)
Note over Test: teardown_method
Test->>DR: get_default_bot_registry()
DR-->>Test: same BotPlatformRegistry instance
Test->>PR: registry.unregister("mockbot")
PR-->>Test: True (removed) / False (not found)
Reviews (2): Last reviewed commit: "fix(tests): add exception handling to _u..." | Re-trigger Greptile |
|
@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 |
![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 refactors the integration tests by introducing a _unregister_bot_platform helper function to manage the removal of bot platforms from the registry, replacing direct access to internal state. It also updates the expected error message for unknown platform resolution. The reviewer suggested improving the robustness of the new helper function by wrapping the unregistration logic in a try-except block to prevent cleanup failures from masking primary test errors.
| def _unregister_bot_platform(name: str) -> None: | ||
| """Remove a runtime-registered bot platform (tests only).""" | ||
| from praisonai.bots._registry import get_default_bot_registry | ||
|
|
||
| get_default_bot_registry().unregister(name.lower()) |
There was a problem hiding this comment.
The unregister method on the registry likely raises a ValueError or KeyError if the platform name is not found (consistent with the resolve method's behavior in the same registry). Since this helper is used in teardown_method and cleanup lambdas where the platform might not have been successfully registered (e.g., if a test fails early or only registers a subset of platforms), it should be made safe to avoid secondary failures during cleanup that could mask the original test failure.
| def _unregister_bot_platform(name: str) -> None: | |
| """Remove a runtime-registered bot platform (tests only).""" | |
| from praisonai.bots._registry import get_default_bot_registry | |
| get_default_bot_registry().unregister(name.lower()) | |
| def _unregister_bot_platform(name: str) -> None: | |
| """Remove a runtime-registered bot platform (tests only).""" | |
| from praisonai.bots._registry import get_default_bot_registry | |
| try: | |
| get_default_bot_registry().unregister(name.lower()) | |
| except (KeyError, ValueError): | |
| pass |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review ✅ APPROVEDTodo List:
Architecture Compliance ✅Phase 1: AGENTS.md Review
Issue Resolution ✅Phase 2: Fixed Valid Issues
Phase 3: Final Assessment
SummaryThis PR successfully modernizes BotOS integration tests by:
The changes are minimal, focused, and maintain full backward compatibility. All valid reviewer concerns have been addressed. ✅ APPROVED - Ready to merge Modified files: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/praisonai-agents/tests/unit/test_botos_integration.py (1)
406-407: ⚡ Quick winTighten error-match regex to avoid false positives.
r"Unknown .*plugin"is permissive enough to match unrelated messages. Consider matching the unknown platform identifier too.Proposed assertion tightening
- with pytest.raises(ValueError, match=r"Unknown .*plugin"): + with pytest.raises(ValueError, match=r"Unknown .*plugin.*nonexistent_xyz"): resolve_adapter("nonexistent_xyz")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/praisonai-agents/tests/unit/test_botos_integration.py` around lines 406 - 407, The test's error regex is too permissive and can match unrelated messages; update the pytest.raises in the test that calls resolve_adapter("nonexistent_xyz") to assert the specific unknown identifier is present (e.g., match "nonexistent_xyz") or use a stricter pattern like r"Unknown .*plugin: .*nonexistent_xyz" so the failure only passes when the resolver reports that exact platform (refer to the resolve_adapter call in the test).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/praisonai-agents/tests/unit/test_botos_integration.py`:
- Around line 406-407: The test's error regex is too permissive and can match
unrelated messages; update the pytest.raises in the test that calls
resolve_adapter("nonexistent_xyz") to assert the specific unknown identifier is
present (e.g., match "nonexistent_xyz") or use a stricter pattern like r"Unknown
.*plugin: .*nonexistent_xyz" so the failure only passes when the resolver
reports that exact platform (refer to the resolve_adapter call in the test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90673625-ece1-478c-896a-2389e4c3a9d5
📒 Files selected for processing (1)
src/praisonai-agents/tests/unit/test_botos_integration.py
Prevents cleanup failures from masking primary test errors by catching KeyError/ValueError exceptions during platform unregistration. Addresses Gemini reviewer feedback. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
Replaces removed
_custom_platformsdict usage withget_default_bot_registry().unregister()after the bot registry moved toPluginRegistry. Updates unknown-platform assertion to match the current ValueError text.Made with Cursor
Summary by CodeRabbit