fix(ai_client_proxy): add _pending_lock threading.Lock
And fix test_discussion_takes_gui.py patches to use ai_client_stub
This commit is contained in:
@@ -0,0 +1,32 @@
|
|||||||
|
# Track: Fix Pre-Existing Test Failures
|
||||||
|
|
||||||
|
## Context
|
||||||
|
|
||||||
|
Two test failures that are not related to the ai_client_stub integration fix but need to be resolved for full test suite passing.
|
||||||
|
|
||||||
|
## Failed Tests
|
||||||
|
|
||||||
|
### 1. test_ai_client_proxy_run.py::test_initial_state_variables
|
||||||
|
|
||||||
|
**File:** `tests/test_ai_client_proxy_run.py`
|
||||||
|
**Error:** `AssertionError: Missing _pending_lock`
|
||||||
|
|
||||||
|
**Root Cause:** The test expects `AIProxyClient` to have a `_pending_lock` attribute (a threading lock), but the class doesn't have it.
|
||||||
|
|
||||||
|
**Fix:** Add `_pending_lock: threading.Lock` to `AIProxyClient.__init__` in `src/ai_client_proxy.py`
|
||||||
|
|
||||||
|
### 2. test_discussion_takes_gui.py (both tests)
|
||||||
|
|
||||||
|
**File:** `tests/test_discussion_takes_gui.py`
|
||||||
|
**Error:** `ValueError: not enough values to unpack (expected 2, got 0)` at `src/gui_2.py:3668`
|
||||||
|
|
||||||
|
**Root Cause:** The test mocks `imgui.input_text` and `imgui.input_int` but NOT `imgui.input_text_multiline`. When `_render_synthesis_panel` calls `imgui.input_text_multiline(...)`, the mock returns `None` (not unpacked), causing the unpacking failure.
|
||||||
|
|
||||||
|
**Fix:** Add mock for `imgui.input_text_multiline` in the test setup.
|
||||||
|
|
||||||
|
## Tasks
|
||||||
|
|
||||||
|
1. [ ] Fix AIProxyClient - add `_pending_lock` threading.Lock to __init__
|
||||||
|
2. [ ] Fix test_discussion_takes_gui.py - add mock for input_text_multiline
|
||||||
|
3. [ ] Run tests to verify both fixes
|
||||||
|
4. [ ] Run full test suite to confirm all pass
|
||||||
@@ -0,0 +1,61 @@
|
|||||||
|
# Track: Fix Test Patches for ai_client_stub Integration
|
||||||
|
|
||||||
|
## Context
|
||||||
|
|
||||||
|
After the refactor to use `ai_client_stub` as the module alias for `app_controller`, several tests fail because they use `patch('src.ai_client.X')` which doesn't properly reach the stub's module-level functions. This is a pre-existing architectural issue that needs fixing.
|
||||||
|
|
||||||
|
## Root Cause Analysis
|
||||||
|
|
||||||
|
When tests use `patch('src.ai_client.get_current_tier', return_value='Tier 3')`:
|
||||||
|
|
||||||
|
1. `patch` creates/overrides `src.ai_client` in the `src` package namespace
|
||||||
|
2. But `app_controller` does `from src import ai_client_stub as ai_client` at module load time
|
||||||
|
3. The `ai_client` local reference in `app_controller` points directly to `ai_client_stub` module
|
||||||
|
4. Patch modifies `src.ai_client` (a different object), not `src.ai_client_stub`
|
||||||
|
5. Result: Functions in `ai_client_stub` aren't patched
|
||||||
|
|
||||||
|
## Solution Applied
|
||||||
|
|
||||||
|
Changed all patches from `patch('src.ai_client.X')` to `patch('src.ai_client_stub.X')` where the stub was the actual target.
|
||||||
|
|
||||||
|
Also updated module imports in tests to use `ai_client_stub` instead of `ai_client` where appropriate.
|
||||||
|
|
||||||
|
## Tasks Completed
|
||||||
|
|
||||||
|
1. [x] Fix `test_on_tool_log_offloading` - change patch path to `src.ai_client_stub.get_current_tier`
|
||||||
|
2. [x] Fix `test_redundant_calls_in_process_pending_gui_tasks` - change patches to `src.ai_client_stub`
|
||||||
|
3. [x] Fix `test_gcli_path_updates_adapter` - use ai_client_stub module reference
|
||||||
|
4. [x] Fix `test_telemetry_data_updates_correctly` - change patch to `src.ai_client_stub.get_token_stats`
|
||||||
|
5. [x] Fix `test_gui_updates_on_event` - same as above
|
||||||
|
6. [x] Run batch test to verify affected tests pass
|
||||||
|
|
||||||
|
## Test Results
|
||||||
|
|
||||||
|
**51 tests passing in the core batch:**
|
||||||
|
- test_symbol_lookup.py (7)
|
||||||
|
- test_ai_client_concurrency.py (1)
|
||||||
|
- test_app_controller_offloading.py (2)
|
||||||
|
- test_mma_agent_focus_phase1.py (8)
|
||||||
|
- test_conductor_engine_v2.py (10)
|
||||||
|
- test_conductor_tech_lead.py (7)
|
||||||
|
- test_mma_dashboard_refresh.py (2)
|
||||||
|
- test_mma_ticket_actions.py (2)
|
||||||
|
- test_gui_phase3.py (3)
|
||||||
|
- test_process_pending_gui_tasks.py (4)
|
||||||
|
- test_gui_updates.py (3)
|
||||||
|
|
||||||
|
## Known Pre-Existing Failures (NOT related to this fix)
|
||||||
|
|
||||||
|
1. **test_ai_client_proxy_run.py::test_initial_state_variables** - AIProxyClient missing `_pending_lock` attribute
|
||||||
|
2. **test_discussion_takes_gui.py** - incomplete mock setup for imgui.input_text_multiline
|
||||||
|
|
||||||
|
## Files Modified
|
||||||
|
|
||||||
|
- tests/test_app_controller_offloading.py
|
||||||
|
- tests/test_process_pending_gui_tasks.py
|
||||||
|
- tests/test_gui_updates.py
|
||||||
|
|
||||||
|
## Checkpoint
|
||||||
|
|
||||||
|
Commit: 169fe520 - fix(ai_client_stub): add module-level import for GeminiCliAdapter
|
||||||
|
Additional commits for test fixes to follow.
|
||||||
@@ -0,0 +1,84 @@
|
|||||||
|
# Track: Fix Test Patches for ai_client_stub Integration
|
||||||
|
|
||||||
|
## Context
|
||||||
|
|
||||||
|
After the refactor to use `ai_client_stub` as the module alias for `app_controller`, several tests fail because they use `patch('src.ai_client.X')` which doesn't properly reach the stub's module-level functions. This is a pre-existing architectural issue that needs fixing.
|
||||||
|
|
||||||
|
## Root Cause Analysis
|
||||||
|
|
||||||
|
When tests use `patch('src.ai_client.get_current_tier', return_value='Tier 3')`:
|
||||||
|
|
||||||
|
1. `patch` creates/overrides `src.ai_client` in the `src` package namespace
|
||||||
|
2. But `app_controller` does `from src import ai_client_stub as ai_client` at module load time
|
||||||
|
3. The `ai_client` local reference in `app_controller` points directly to `ai_client_stub` module
|
||||||
|
4. Patch modifies `src.ai_client` (a different object), not `src.ai_client_stub`
|
||||||
|
5. Result: Functions in `ai_client_stub` aren't patched
|
||||||
|
|
||||||
|
## Failed Tests
|
||||||
|
|
||||||
|
1. `tests/test_app_controller_offloading.py::test_on_tool_log_offloading`
|
||||||
|
- Patches `src.ai_client.get_current_tier` expecting it to affect `app_controller._on_tool_log()`
|
||||||
|
- But `app_controller` imported the stub directly, so patch doesn't reach it
|
||||||
|
|
||||||
|
2. `tests/test_process_pending_gui_tasks.py::test_redundant_calls_in_process_pending_gui_tasks`
|
||||||
|
- Patches `src.ai_client.set_provider` and `src.ai_client.reset_session`
|
||||||
|
- Same root cause
|
||||||
|
|
||||||
|
3. `tests/test_process_pending_gui_tasks.py::test_gcli_path_updates_adapter`
|
||||||
|
- Sets `ai_client._gemini_cli_adapter = None` expecting `app_controller` to see it
|
||||||
|
- Same import aliasing issue
|
||||||
|
|
||||||
|
4. `tests/test_gui_updates.py::test_telemetry_data_updates_correctly`
|
||||||
|
- Patches `src.ai_client.get_token_stats`
|
||||||
|
- Same issue
|
||||||
|
|
||||||
|
5. `tests/test_gui_updates.py::test_gui_updates_on_event`
|
||||||
|
- Same issue with `get_token_stats` patch
|
||||||
|
|
||||||
|
## Solution Options
|
||||||
|
|
||||||
|
### Option A: Fix Tests to Use Correct Patch Path
|
||||||
|
Change all patches from `patch('src.ai_client.X')` to `patch('src.ai_client_stub.X')`.
|
||||||
|
|
||||||
|
**Pros:**
|
||||||
|
- Minimal code change
|
||||||
|
- Resolves the actual issue
|
||||||
|
- Follows proper patch semantics
|
||||||
|
|
||||||
|
**Cons:**
|
||||||
|
- Must update all affected tests
|
||||||
|
|
||||||
|
### Option B: Add Module-Level Alias in src package
|
||||||
|
Create `src/ai_client.py` that re-exports from `ai_client_stub`:
|
||||||
|
```python
|
||||||
|
from src.ai_client_stub import *
|
||||||
|
```
|
||||||
|
|
||||||
|
**Pros:**
|
||||||
|
- Makes `src.ai_client` available as actual module
|
||||||
|
- Tests using `patch('src.ai_client.X')` would work
|
||||||
|
|
||||||
|
**Cons:**
|
||||||
|
- Creates dual namespace confusion
|
||||||
|
- May conflict with existing `ai_client_proxy.py`
|
||||||
|
|
||||||
|
### Option C: Fix app_controller to Not Use Alias
|
||||||
|
Change all `from src import ai_client_stub as ai_client` to `from src import ai_client` and ensure `src.ai_client` is the stub.
|
||||||
|
|
||||||
|
**Cons:**
|
||||||
|
- Significant refactor
|
||||||
|
- May break other imports
|
||||||
|
|
||||||
|
## Recommended Solution: Option A
|
||||||
|
|
||||||
|
Fix the tests to use the correct patch path (`src.ai_client_stub`). This is surgical and addresses the root cause without creating architectural complexity.
|
||||||
|
|
||||||
|
## Tasks
|
||||||
|
|
||||||
|
1. [ ] Fix `test_on_tool_log_offloading` - change patch path to `src.ai_client_stub.get_current_tier`
|
||||||
|
2. [ ] Fix `test_redundant_calls_in_process_pending_gui_tasks` - change patches to `src.ai_client_stub`
|
||||||
|
3. [ ] Fix `test_gcli_path_updates_adapter` - use correct patch/module path
|
||||||
|
4. [ ] Fix `test_telemetry_data_updates_correctly` - change patch to `src.ai_client_stub.get_token_stats`
|
||||||
|
5. [ ] Fix `test_gui_updates_on_event` - same as above
|
||||||
|
6. [ ] Run full batch test to verify all tests pass
|
||||||
|
7. [ ] Create checkpoint commit
|
||||||
@@ -13,6 +13,7 @@ class AIProxyClient:
|
|||||||
self._status: str = "disconnected"
|
self._status: str = "disconnected"
|
||||||
self._pending: dict[str, Any] = {}
|
self._pending: dict[str, Any] = {}
|
||||||
self._reader_thread: Optional[threading.Thread] = None
|
self._reader_thread: Optional[threading.Thread] = None
|
||||||
|
self._pending_lock: threading.Lock = threading.Lock()
|
||||||
|
|
||||||
@property
|
@property
|
||||||
def status(self) -> str:
|
def status(self) -> str:
|
||||||
|
|||||||
@@ -88,7 +88,7 @@ def test_on_tool_log_offloading(app_controller, tmp_session_dir):
|
|||||||
script = "Get-Process"
|
script = "Get-Process"
|
||||||
result = "Process list..."
|
result = "Process list..."
|
||||||
|
|
||||||
with patch("src.ai_client.get_current_tier", return_value="Tier 3"):
|
with patch("src.ai_client_stub.get_current_tier", return_value="Tier 3"):
|
||||||
app_controller._on_tool_log(script, result)
|
app_controller._on_tool_log(script, result)
|
||||||
|
|
||||||
# Verify files were created in session directory
|
# Verify files were created in session directory
|
||||||
|
|||||||
@@ -17,8 +17,8 @@ def app_instance():
|
|||||||
patch('src.app_controller.AppController._prune_old_logs'),
|
patch('src.app_controller.AppController._prune_old_logs'),
|
||||||
patch('src.app_controller.AppController.start_services'),
|
patch('src.app_controller.AppController.start_services'),
|
||||||
patch('src.api_hooks.HookServer'),
|
patch('src.api_hooks.HookServer'),
|
||||||
patch('src.ai_client.set_provider'),
|
patch('src.ai_client_stub.set_provider'),
|
||||||
patch('src.ai_client.reset_session')
|
patch('src.ai_client_stub.reset_session')
|
||||||
):
|
):
|
||||||
app = App()
|
app = App()
|
||||||
# Setup project discussions
|
# Setup project discussions
|
||||||
@@ -47,12 +47,13 @@ def test_render_discussion_tabs(app_instance):
|
|||||||
mock_imgui.begin_combo.return_value = False
|
mock_imgui.begin_combo.return_value = False
|
||||||
mock_imgui.input_text.return_value = (False, "")
|
mock_imgui.input_text.return_value = (False, "")
|
||||||
mock_imgui.input_int.return_value = (False, 0)
|
mock_imgui.input_int.return_value = (False, 0)
|
||||||
|
mock_imgui.input_text_multiline.return_value = (False, "")
|
||||||
mock_imgui.button.return_value = False
|
mock_imgui.button.return_value = False
|
||||||
mock_imgui.checkbox.return_value = (False, False)
|
mock_imgui.checkbox.return_value = (False, False)
|
||||||
mock_imgui.begin_child.return_value = True
|
mock_imgui.begin_child.return_value = True
|
||||||
mock_imgui.selectable.return_value = (False, False)
|
mock_imgui.selectable.return_value = (False, False)
|
||||||
mock_imgui.ListClipper.return_value.step.return_value = False
|
mock_imgui.ListClipper.return_value.step.return_value = False
|
||||||
|
|
||||||
# Mock tab bar calls
|
# Mock tab bar calls
|
||||||
mock_imgui.begin_tab_bar.return_value = True
|
mock_imgui.begin_tab_bar.return_value = True
|
||||||
mock_imgui.begin_tab_item.return_value = (False, False)
|
mock_imgui.begin_tab_item.return_value = (False, False)
|
||||||
@@ -86,6 +87,7 @@ def test_switching_discussion_via_tabs(app_instance):
|
|||||||
mock_imgui.begin_combo.return_value = False
|
mock_imgui.begin_combo.return_value = False
|
||||||
mock_imgui.input_text.return_value = (False, "")
|
mock_imgui.input_text.return_value = (False, "")
|
||||||
mock_imgui.input_int.return_value = (False, 0)
|
mock_imgui.input_int.return_value = (False, 0)
|
||||||
|
mock_imgui.input_text_multiline.return_value = (False, "")
|
||||||
mock_imgui.button.return_value = False
|
mock_imgui.button.return_value = False
|
||||||
mock_imgui.checkbox.return_value = (False, False)
|
mock_imgui.checkbox.return_value = (False, False)
|
||||||
mock_imgui.begin_child.return_value = True
|
mock_imgui.begin_child.return_value = True
|
||||||
@@ -93,7 +95,7 @@ def test_switching_discussion_via_tabs(app_instance):
|
|||||||
mock_imgui.ListClipper.return_value.step.return_value = False
|
mock_imgui.ListClipper.return_value.step.return_value = False
|
||||||
|
|
||||||
mock_imgui.begin_tab_bar.return_value = True
|
mock_imgui.begin_tab_bar.return_value = True
|
||||||
|
|
||||||
# Setup imscope mocks
|
# Setup imscope mocks
|
||||||
mock_imscope.window.return_value.__enter__.return_value = (True, True)
|
mock_imscope.window.return_value.__enter__.return_value = (True, True)
|
||||||
mock_imscope.child.return_value.__enter__.return_value = True
|
mock_imscope.child.return_value.__enter__.return_value = True
|
||||||
|
|||||||
@@ -31,7 +31,7 @@ def test_telemetry_data_updates_correctly(app_instance: Any) -> None:
|
|||||||
"percentage": 75.0,
|
"percentage": 75.0,
|
||||||
}
|
}
|
||||||
# 3. Patch the dependencies
|
# 3. Patch the dependencies
|
||||||
with patch('src.ai_client.get_token_stats', return_value=mock_stats) as mock_get_stats:
|
with patch('src.ai_client_stub.get_token_stats', return_value=mock_stats) as mock_get_stats:
|
||||||
# 4. Call the method under test
|
# 4. Call the method under test
|
||||||
app_instance._refresh_api_metrics({}, md_content="test content")
|
app_instance._refresh_api_metrics({}, md_content="test content")
|
||||||
# 5. Assert the results
|
# 5. Assert the results
|
||||||
@@ -60,7 +60,7 @@ def test_gui_updates_on_event(app_instance: App) -> None:
|
|||||||
"""
|
"""
|
||||||
mock_stats = {"percentage": 50.0, "current": 500, "limit": 1000}
|
mock_stats = {"percentage": 50.0, "current": 500, "limit": 1000}
|
||||||
app_instance.last_md = "mock_md"
|
app_instance.last_md = "mock_md"
|
||||||
with patch('src.ai_client.get_token_stats', return_value=mock_stats):
|
with patch('src.ai_client_stub.get_token_stats', return_value=mock_stats):
|
||||||
# Drain the queue
|
# Drain the queue
|
||||||
while not app_instance.event_queue.empty():
|
while not app_instance.event_queue.empty():
|
||||||
app_instance.event_queue.get()
|
app_instance.event_queue.get()
|
||||||
|
|||||||
@@ -1,7 +1,7 @@
|
|||||||
from typing import Generator
|
from typing import Generator
|
||||||
import pytest
|
import pytest
|
||||||
from unittest.mock import patch, Mock
|
from unittest.mock import patch, Mock
|
||||||
from src import ai_client
|
from src import ai_client_stub
|
||||||
from src.gui_2 import App
|
from src.gui_2 import App
|
||||||
|
|
||||||
@pytest.fixture
|
@pytest.fixture
|
||||||
@@ -20,8 +20,8 @@ def app_instance() -> Generator[App, None, None]:
|
|||||||
patch('src.app_controller.AppController.start_services'),
|
patch('src.app_controller.AppController.start_services'),
|
||||||
# Do not patch _init_ai_and_hooks to ensure _settable_fields is initialized
|
# Do not patch _init_ai_and_hooks to ensure _settable_fields is initialized
|
||||||
patch('src.api_hooks.HookServer'),
|
patch('src.api_hooks.HookServer'),
|
||||||
patch('src.ai_client.set_provider'),
|
patch('src.ai_client_stub.set_provider'),
|
||||||
patch('src.ai_client.reset_session')
|
patch('src.ai_client_stub.reset_session')
|
||||||
):
|
):
|
||||||
app = App()
|
app = App()
|
||||||
yield app
|
yield app
|
||||||
@@ -30,8 +30,8 @@ def test_redundant_calls_in_process_pending_gui_tasks(app_instance: App) -> None
|
|||||||
app_instance.controller._pending_gui_tasks = [
|
app_instance.controller._pending_gui_tasks = [
|
||||||
{'action': 'set_value', 'item': 'current_provider', 'value': 'anthropic'}
|
{'action': 'set_value', 'item': 'current_provider', 'value': 'anthropic'}
|
||||||
]
|
]
|
||||||
with patch('src.ai_client.set_provider') as mock_set_provider, \
|
with patch('src.ai_client_stub.set_provider') as mock_set_provider, \
|
||||||
patch('src.ai_client.reset_session') as mock_reset_session:
|
patch('src.ai_client_stub.reset_session') as mock_reset_session:
|
||||||
app_instance.controller._process_pending_gui_tasks()
|
app_instance.controller._process_pending_gui_tasks()
|
||||||
assert mock_set_provider.call_count == 1
|
assert mock_set_provider.call_count == 1
|
||||||
assert mock_reset_session.call_count == 1
|
assert mock_reset_session.call_count == 1
|
||||||
@@ -42,10 +42,10 @@ def test_gcli_path_updates_adapter(app_instance: App) -> None:
|
|||||||
{'action': 'set_value', 'item': 'gcli_path', 'value': '/new/path/to/gemini'}
|
{'action': 'set_value', 'item': 'gcli_path', 'value': '/new/path/to/gemini'}
|
||||||
]
|
]
|
||||||
# Initialize adapter if it doesn't exist (it shouldn't in mock env)
|
# Initialize adapter if it doesn't exist (it shouldn't in mock env)
|
||||||
ai_client._gemini_cli_adapter = None
|
ai_client_stub._gemini_cli_adapter = None
|
||||||
app_instance.controller._process_pending_gui_tasks()
|
app_instance.controller._process_pending_gui_tasks()
|
||||||
assert ai_client._gemini_cli_adapter is not None
|
assert ai_client_stub._gemini_cli_adapter is not None
|
||||||
assert ai_client._gemini_cli_adapter.binary_path == '/new/path/to/gemini'
|
assert ai_client_stub._gemini_cli_adapter.binary_path == '/new/path/to/gemini'
|
||||||
|
|
||||||
def test_process_pending_gui_tasks_drag(app_instance: App) -> None:
|
def test_process_pending_gui_tasks_drag(app_instance: App) -> None:
|
||||||
"""Test that the drag action is correctly processed and dispatches to the registered callback."""
|
"""Test that the drag action is correctly processed and dispatches to the registered callback."""
|
||||||
|
|||||||
Reference in New Issue
Block a user