feat: realtime voice — tests + top-level exports (follow-up to PR #6)#11
Conversation
- Add comprehensive tests/unit/test_realtime.py covering: * Protocol conformance verification (RealtimeProtocol ABC) * Session lifecycle with mocked OpenAI SDK * Manager singleton round-trip (get/set_realtime_manager) * Health endpoint behavior with/without OpenAI * Lazy-import invariant validation - Export realtime classes from src/praisonaiui/__init__.py: * RealtimeProtocol, OpenAIRealtimeManager * set_realtime (alias), get_realtime_manager, set_realtime_manager * Following existing lazy-import pattern with __getattr__ Fixes acceptance criteria from #1: ✅ Unit tests for protocol conformance + session lifecycle ✅ aiui.set_realtime(OpenAIRealtimeManager()) top-level API Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
Hold on merge — 3 test failures. Root cause is a test-authoring bug, not a production bug. Fix is small (~10 lines). FailuresRoot cause
with patch('praisonaiui.features.realtime.openai', None):But This is actually correct behaviour on the production side — lazy imports are the whole point (AGENTS.md §4.2). The test is wrong. Fix — patch
|
|
Claude finished @MervinPraison's task —— View job Task Complete ✅Fixed the 3 failing tests in Todo List
Changes MadeRoot Cause: Tests were trying to patch Fix Applied: Replaced 3 failing patch calls with
This preserves the lazy-import invariant while properly mocking the Results
Commit: 2bf0ea3 |
![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 introduces realtime voice feature support to the praisonaiui package, including the RealtimeProtocol and OpenAIRealtimeManager. The implementation utilizes lazy loading to ensure the openai SDK is only imported when needed. Feedback on the new unit tests highlights issues with the current mocking strategy; specifically, patching builtins.import and module attributes is unreliable for local imports. It is recommended to use patch.dict on sys.modules to correctly simulate missing packages or inject mocks. Additionally, the lazy import tests should ensure praisonaiui is cleared from sys.modules to maintain test isolation.
| with patch('praisonaiui.features.realtime.openai', None): | ||
| # Mock import to raise ImportError | ||
| with patch('builtins.__import__', side_effect=ImportError("No module named 'openai'")): | ||
| session_info = await manager.create_session() | ||
|
|
||
| assert session_info["type"] == "error" | ||
| assert "openai package not installed" in session_info["error"] |
There was a problem hiding this comment.
The current mocking strategy for simulating a missing openai package is problematic. Patching builtins.__import__ can interfere with other imports (like uuid used in the same method) or even internal pytest operations, which is likely why this test is failing. A more robust and standard way to simulate a missing package is to patch sys.modules by setting the module name to None.
# Mock ImportError for openai package
with patch.dict("sys.modules", {"openai": None}):
session_info = await manager.create_session()
assert session_info["type"] == "error"
assert "openai package not installed" in session_info["error"]| mock_client.realtime.sessions.create = AsyncMock(return_value=mock_response) | ||
| mock_openai.OpenAI.return_value = mock_client | ||
|
|
||
| with patch('praisonaiui.features.realtime.openai', mock_openai): |
There was a problem hiding this comment.
Patching praisonaiui.features.realtime.openai does not affect the import openai statement inside the create_session method because it is a local import. Local imports look up sys.modules and bind to the local scope, ignoring the module's global namespace. To mock a locally imported module, you should patch sys.modules instead.
| with patch('praisonaiui.features.realtime.openai', mock_openai): | |
| with patch.dict("sys.modules", {"openai": mock_openai}): |
| manager._sessions["session_2"] = {"id": "session_2"} | ||
|
|
||
| mock_openai = mock.MagicMock() | ||
| with patch('praisonaiui.features.realtime.openai', mock_openai): |
There was a problem hiding this comment.
Similar to the session lifecycle test, patching the module attribute is ineffective for local imports inside the health method. Use patch.dict on sys.modules to ensure the mock is correctly injected.
| with patch('praisonaiui.features.realtime.openai', mock_openai): | |
| with patch.dict("sys.modules", {"openai": mock_openai}): |
| with patch('builtins.__import__', side_effect=ImportError("No module named 'openai'")): | ||
| health = manager.health() |
There was a problem hiding this comment.
While this test might pass because health() has fewer side effects than create_session(), patching builtins.__import__ remains a brittle practice. It is recommended to use patch.dict on sys.modules for consistency and reliability.
| with patch('builtins.__import__', side_effect=ImportError("No module named 'openai'")): | |
| health = manager.health() | |
| with patch.dict("sys.modules", {"openai": None}): | |
| health = manager.health() |
| # Remove openai from modules if present | ||
| openai_modules = [mod for mod in sys.modules.keys() if mod.startswith('openai')] | ||
| for mod in openai_modules: | ||
| if mod in sys.modules: | ||
| del sys.modules[mod] |
There was a problem hiding this comment.
The test for lazy imports may be unreliable if praisonaiui has already been imported in the current test process (e.g., by other tests in the suite). To ensure a fresh import and accurately verify the lazy loading logic, praisonaiui and its submodules should also be removed from sys.modules before the test import.
| # Remove openai from modules if present | |
| openai_modules = [mod for mod in sys.modules.keys() if mod.startswith('openai')] | |
| for mod in openai_modules: | |
| if mod in sys.modules: | |
| del sys.modules[mod] | |
| # Remove praisonaiui and openai from modules if present | |
| for mod in list(sys.modules.keys()): | |
| if mod.startswith(("praisonaiui", "openai")): | |
| del sys.modules[mod] |
…ime tests
- Replace patch('praisonaiui.features.realtime.openai') with patch.dict(sys.modules, {'openai': ...})
- Fixes 3 failing tests: test_session_lifecycle_without_openai, test_session_lifecycle_with_mocked_openai, test_health_with_openai
- Preserves lazy-import invariant by not adding module-level openai import
Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Follow-up to PR #6 to complete Issue #1’s public API surface by exposing realtime voice primitives at the package top level and adding a dedicated unit test module for protocol + manager behavior.
Changes:
- Added lazy top-level exports for
RealtimeProtocol,OpenAIRealtimeManager, and realtime manager setters/getters (includingset_realtimealias). - Added
tests/unit/test_realtime.pycovering protocol conformance, singleton lifecycle, session lifecycle, health, and lazy-import expectations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/praisonaiui/__init__.py |
Adds lazy __getattr__ routing + __all__ entries for realtime voice public exports. |
tests/unit/test_realtime.py |
New unit test module for realtime protocol/manager behavior and lazy import invariants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class TestManagerSingleton: | ||
| """Test get/set realtime manager lifecycle.""" | ||
|
|
||
| def test_manager_round_trip(self): | ||
| """Test set_realtime_manager / get_realtime_manager round-trip.""" | ||
| # Create custom manager | ||
| custom_manager = OpenAIRealtimeManager() | ||
|
|
||
| # Set it | ||
| set_realtime_manager(custom_manager) | ||
|
|
||
| # Get it back | ||
| retrieved = get_realtime_manager() | ||
|
|
||
| # Should be the same instance | ||
| assert retrieved is custom_manager | ||
|
|
||
| def test_default_manager_is_openai(self): | ||
| """Test default manager is OpenAIRealtimeManager instance.""" | ||
| # Reset manager | ||
| set_realtime_manager(None) | ||
|
|
||
| # Get default | ||
| default = get_realtime_manager() | ||
|
|
||
| assert isinstance(default, OpenAIRealtimeManager) | ||
|
|
There was a problem hiding this comment.
These tests mutate the global realtime manager singleton (set_realtime_manager(...)) but don’t guarantee cleanup if test order changes or if additional tests are added later. Add a fixture (preferably autouse=True) that saves the prior manager and restores it (or resets to None) after each test to avoid state leakage across the test module / suite.
| # Access manager functions (should not import openai yet) | ||
| manager = get_realtime_manager() | ||
| assert isinstance(manager, OpenAIRealtimeManager) | ||
|
|
There was a problem hiding this comment.
The lazy-import coverage here calls get_realtime_manager() directly from praisonaiui.features.realtime, so it doesn’t exercise the new top-level exports added in praisonaiui.__init__ (RealtimeProtocol, OpenAIRealtimeManager, set_realtime, etc.). Consider importing praisonaiui and accessing these attributes via the package (praisonaiui.get_realtime_manager(), praisonaiui.set_realtime(...)) to ensure the __getattr__ routing + the set_realtime alias behave as intended.
| # Access manager functions (should not import openai yet) | |
| manager = get_realtime_manager() | |
| assert isinstance(manager, OpenAIRealtimeManager) | |
| # Access realtime exports via the package to exercise __getattr__ routing | |
| import praisonaiui | |
| manager = praisonaiui.get_realtime_manager() | |
| assert isinstance(manager, praisonaiui.OpenAIRealtimeManager) | |
| # Exercise the package-level setter alias without changing behavior | |
| praisonaiui.set_realtime(manager) | |
| assert praisonaiui.get_realtime_manager() is manager | |
| # Force `import openai` inside the function body to fail | ||
| with patch.dict(sys.modules, {"openai": None}): | ||
| session_info = await manager.create_session() | ||
|
|
There was a problem hiding this comment.
patch('praisonaiui.features.realtime.openai', None) will raise AttributeError because openai is not a module-level attribute in praisonaiui.features.realtime (it’s imported inside create_session()/health()). Patch the import path in a way that actually affects import openai (e.g., patch.dict(sys.modules, {'openai': None}) + assert the returned error type, or patch builtins.__import__ with a wrapper that only raises for 'openai'). If you keep patch(...), it needs create=True but that still won’t intercept the local import openai.
| with patch.dict(sys.modules, {"openai": mock_openai}): | ||
| # Create session | ||
| session_info = await manager.create_session(model="gpt-4o-realtime-preview") | ||
|
|
There was a problem hiding this comment.
This mock doesn’t affect import openai inside OpenAIRealtimeManager.create_session() because openai isn’t referenced as praisonaiui.features.realtime.openai. Use patch.dict(sys.modules, {'openai': mock_openai}) (or a targeted __import__ wrapper) so the in-function import resolves to the mock; otherwise the test may import the real SDK (or fail due to the missing attribute being patched).
|
|
||
| mock_openai = mock.MagicMock() | ||
| with patch.dict(sys.modules, {"openai": mock_openai}): | ||
| health = manager.health() |
There was a problem hiding this comment.
patch('praisonaiui.features.realtime.openai', mock_openai) will fail for the same reason as above: openai is imported locally inside health(), not stored as a module attribute. Patch sys.modules['openai'] (or use a targeted import wrapper) so import openai inside health() resolves to the mock, and the test remains stable regardless of whether the real OpenAI SDK is installed in the test environment.
|
|
||
| def test_import_praisonaiui_no_openai_import(self): | ||
| """Verify importing praisonaiui package doesn't import openai.""" | ||
| # This test ensures lazy imports work correctly | ||
| import sys | ||
|
|
||
| # Remove openai from modules if present | ||
| openai_modules = [mod for mod in sys.modules.keys() if mod.startswith('openai')] | ||
| for mod in openai_modules: | ||
| if mod in sys.modules: | ||
| del sys.modules[mod] | ||
|
|
||
| # Import praisonaiui | ||
| import praisonaiui # noqa: F401 |
There was a problem hiding this comment.
The “lazy import” tests don’t actually validate import praisonaiui behavior because this module already imports praisonaiui (via from praisonaiui.features.realtime import ...) at import time. By the time test_import_praisonaiui_no_openai_import runs, praisonaiui is already in sys.modules, so import praisonaiui is a no-op. To make this test meaningful, move the praisonaiui.features.realtime imports inside the specific tests (or purge praisonaiui + its submodules from sys.modules before importing, or run the invariant check in a subprocess).
Overview
Addresses gaps on issue #1 identified in the follow-up review of merged PR #6. Adds the missing
aiui.set_realtime(...)/OpenAIRealtimeManager/RealtimeProtocolexports and a dedicated test module.Work by
@claudeon branchclaude/issue-1-20260418-0609(opened here because the Claude Assistant completed but did not open the PR).Closes #1
Files changed
src/praisonaiui/__init__.pyRealtimeProtocol,OpenAIRealtimeManager,set_realtime,get_realtime_manager,set_realtime_managervia lazy__getattr__tests/unit/test_realtime.pyLocal validation
✅ This PR is now ready for merge — all tests pass.
Scope boundary (what this PR does NOT do)
@claudedid not auto-open the PR. Update on force-push.src/praisonaiui/features/realtime.py— only adds exports and tests.