Add registry regression test for duplicate tool names across modules#1141
Conversation
Greptile SummaryThis PR adds a regression test ( Confidence Score: 5/5Safe to merge; the one finding is a minor robustness suggestion on the caplog assertion that does not affect correctness. All findings are P2. The test logic is sound, integrates correctly with the autouse cache-reset fixture, and the monkeypatching approach mirrors the established pattern in the file. No production code is changed. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Test as test_registry_regression…
participant Registry as registry._load_registry_snapshot
participant M1 as module1 (first_module)
participant M2 as module2 (second_module)
participant Log as logger (WARNING)
Test->>Registry: get_registered_tools()
Registry->>M1: _import_tool_module("first_module")
M1-->>Registry: [shared_tool_name → {module: first}]
Registry->>Registry: tools_by_name["shared_tool_name"] = first
Registry->>M2: _import_tool_module("second_module")
M2-->>Registry: [shared_tool_name → {module: second}]
Registry->>Log: WARNING "Duplicate tool name 'shared_tool_name' across modules"
Registry->>Registry: continue (skip second tool)
Registry-->>Test: [RegisteredTool(shared_tool_name, first)]
Test->>Registry: get_registered_tool_map()["shared_tool_name"].run()
Registry-->>Test: {"module": "first"} ✓
Test->>Log: assert WARNING record captured ✓
Reviews (1): Last reviewed commit: "Add registry regression test for duplica..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
VaibhavUpreti
left a comment
VaibhavUpreti
left a comment
There was a problem hiding this comment.
LGTM, awesome @Aaryan-549 !
|
🏄 Some PRs rot in review for six weeks. @Aaryan-549's said "not today" and merged like it owned the place. 🌊
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
2 participants
Fixes #1116
Add registry regression test for duplicate tool names across modules
Describe the changes you have made in this PR
Added a comprehensive regression test
test_registry_regression_duplicate_tool_names_across_modulesto ensure the tool registry correctlyhandles duplicate tool names when they appear across different modules.
What was added:
Test validates:
Demo/Screenshot for feature changes and bug fixes
Test execution with broken duplicate handling (simulating bug):
FAILED tests/tools/test_registry.py::test_registry_regression_duplicate_tool_names_across_modules
AssertionError: assert {'module': 'second'} == {'module': 'first'}
Test execution with fix in place:
tests/tools/test_registry.py::test_registry_regression_duplicate_tool_names_across_modules PASSED
All 13 registry tests passing after change.
Code Understanding and AI Usage
Did you use AI assistance (ChatGPT, Claude, Copilot, etc.) to write any part of this code?
If you used AI assistance:
Explain your implementation approach:
The test follows the existing pattern in
test_registry.py:ModuleTypeinstances to simulate tool modules@tooldecorator to register themmonkeypatchto mock the module discovery mechanismThe test proves the fix works by deliberately breaking the
continuestatement inregistry.pyline 146 and verifying the test fails(second module's tool overwrites first). With the fix restored, the test passes.
Why this approach:
Checklist before requesting a review