fix(iorails): Fix failing tests due to ModelManager refactor#1791
fix(iorails): Fix failing tests due to ModelManager refactor#1791tgasser-nv merged 1 commit intodevelopfrom
Conversation
Greptile SummaryThis PR updates
|
| Filename | Overview |
|---|---|
| tests/guardrails/test_iorails.py | Updated test suite to replace all model_manager/ModelManager references with engine_registry/EngineRegistry after the ModelManager refactor. Two style issues: variable shadowing in the engine-error propagation test and use of deprecated asyncio.get_event_loop() in the sync-from-async test. |
Sequence Diagram
sequenceDiagram
participant Test
participant IORails
participant EngineRegistry
participant RailsManager
Test->>IORails: generate_async(messages)
IORails->>IORails: start() [idempotent]
IORails->>EngineRegistry: start()
IORails->>RailsManager: is_input_safe(messages)
RailsManager-->>IORails: RailResult(is_safe)
alt input safe
IORails->>EngineRegistry: model_call("main", messages)
EngineRegistry-->>IORails: response_text
IORails->>RailsManager: is_output_safe(messages, response_text)
RailsManager-->>IORails: RailResult(is_safe)
alt output safe
IORails-->>Test: "{role: assistant, content: response_text}"
else output unsafe
IORails-->>Test: "{role: assistant, content: REFUSAL_MESSAGE}"
end
else input unsafe
IORails-->>Test: "{role: assistant, content: REFUSAL_MESSAGE}"
end
Comments Outside Diff (2)
-
tests/guardrails/test_iorails.py, line 246-253 (link)Variable shadowing loop variable
engineThe loop variable
engineon line 250 silently overwrites thecontent_safetyengine captured on line 246. After the loop,enginepoints to the last non-content_safetyengine. The mock forcontent_safetyis already set before the loop, so the test still passes, but the leftoverenginereference is misleading and could trip up a future maintainer who tries to re-use it after the loop.Prompt To Fix With AI
This is a comment left during a code review. Path: tests/guardrails/test_iorails.py Line: 246-253 Comment: **Variable shadowing loop variable `engine`** The loop variable `engine` on line 250 silently overwrites the `content_safety` engine captured on line 246. After the loop, `engine` points to the last non-`content_safety` engine. The mock for `content_safety` is already set before the loop, so the test still passes, but the leftover `engine` reference is misleading and could trip up a future maintainer who tries to re-use it after the loop. How can I resolve this? If you propose a fix, please make it concise.
-
tests/guardrails/test_iorails.py, line 456 (link)asyncio.get_event_loop()is deprecatedasyncio.get_event_loop()emits aDeprecationWarningin Python 3.10+ when there is no current running loop, and raisesRuntimeErrorin 3.12+ in the same case. The test should useasyncio.run(), which is both the modern API and the same callgenerate()itself uses internally — making the conflict explicit.Prompt To Fix With AI
This is a comment left during a code review. Path: tests/guardrails/test_iorails.py Line: 456 Comment: **`asyncio.get_event_loop()` is deprecated** `asyncio.get_event_loop()` emits a `DeprecationWarning` in Python 3.10+ when there is no current running loop, and raises `RuntimeError` in 3.12+ in the same case. The test should use `asyncio.run()`, which is both the modern API and the same call `generate()` itself uses internally — making the conflict explicit. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: tests/guardrails/test_iorails.py
Line: 246-253
Comment:
**Variable shadowing loop variable `engine`**
The loop variable `engine` on line 250 silently overwrites the `content_safety` engine captured on line 246. After the loop, `engine` points to the last non-`content_safety` engine. The mock for `content_safety` is already set before the loop, so the test still passes, but the leftover `engine` reference is misleading and could trip up a future maintainer who tries to re-use it after the loop.
```suggestion
cs_engine = iorails.engine_registry._get_engine("content_safety", ModelEngine)
cs_engine.start = AsyncMock(side_effect=RuntimeError("NIM endpoint unreachable"))
# Mock the other engines so they don't make real HTTP connections
for engine_type, eng in iorails.engine_registry._engines.items():
if engine_type != "content_safety":
eng.start = AsyncMock()
eng.stop = AsyncMock()
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: tests/guardrails/test_iorails.py
Line: 456
Comment:
**`asyncio.get_event_loop()` is deprecated**
`asyncio.get_event_loop()` emits a `DeprecationWarning` in Python 3.10+ when there is no current running loop, and raises `RuntimeError` in 3.12+ in the same case. The test should use `asyncio.run()`, which is both the modern API and the same call `generate()` itself uses internally — making the conflict explicit.
```suggestion
with pytest.raises(RuntimeError):
asyncio.run(call_generate())
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Fix failing tests on develop after model..." | Re-trigger Greptile
📝 WalkthroughWalkthroughTest mocks in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Description
The
test_generate_async_calls_start,test_generate_async_start_is_idempotent,test_stream_async_calls_startandtest_stream_async_propagates_start_failuretests weren't updated to match the ModelManager to EngineRegistry refactoring. This broke unit-tests on the develop branch.Test Plan
Checklist