test(app_controller): scaffold tests/test_app_controller_result.py with 5 Result-pattern tests
Adds 5 tests to lock in the data-oriented error handling contract for
src/app_controller.py:
1. test_offload_entry_payload_returns_dict
- Shape contract: _offload_entry_payload returns a dict.
2. test_migrated_method_returns_result_on_success
- Pattern template: methods migrated to Result[T] return Result[None]
with no errors on the success path. Currently FAILS because
_handle_custom_callback returns None implicitly.
3. test_migrated_method_returns_result_with_error_on_failure
- Pattern template: methods migrated to Result[T] return Result
with errors when the underlying call raises. Currently FAILS for
same reason.
4. test_app_controller_does_not_use_broad_except
- Static AST check: no 'except Exception:' clauses left in
src/app_controller.py after migration. Currently FAILS (32 sites).
5. test_offload_entry_payload_preserves_unchanged_payload
- Verifies the no-op path for non-tool entries.
The 3 currently-failing tests will turn green as the 32 INTERNAL_BROAD_CATCH
sites are migrated across Phase 2's 4 batches. The 2 currently-passing
tests verify the existing shape contract.
Refs: spec.md FR6, plan.md Task 2.1
This commit is contained in:
@@ -0,0 +1,112 @@
|
||||
"""
|
||||
Tests for the data-oriented error handling convention applied to src/app_controller.py.
|
||||
|
||||
This file verifies the contract that the 32 INTERNAL_BROAD_CATCH + 8 INTERNAL_SILENT_SWALLOW
|
||||
+ 4 INTERNAL_RETHROW + 1 INTERNAL_OPTIONAL_RETURN sites in src/app_controller.py are migrated
|
||||
to the Result[T] pattern (per conductor/code_styleguides/error_handling.md).
|
||||
|
||||
The tests are pattern templates:
|
||||
- test_offload_entry_payload_returns_dict - the _offload_entry_payload helper returns a dict
|
||||
(the existing test for the regression fix is in test_app_controller_offloading.py; this
|
||||
one is a shape check).
|
||||
- test_migrated_method_returns_result_on_success - methods migrated to Result[T] return
|
||||
Result[data=...] with no errors on the success path.
|
||||
- test_migrated_method_returns_result_with_error_on_failure - methods migrated to Result[T]
|
||||
return Result with errors=... on the failure path.
|
||||
- test_app_controller_does_not_use_broad_except - static check: src/app_controller.py
|
||||
has no `except Exception:` clauses left (all 32 are migrated to specific exceptions).
|
||||
- test_offload_entry_payload_preserves_unchanged_payload - verifies the no-op path for
|
||||
entries without tool_call or tool_result kinds.
|
||||
"""
|
||||
import pytest
|
||||
from unittest.mock import MagicMock, patch
|
||||
from src.app_controller import AppController
|
||||
from src.result_types import Result, ErrorInfo, ErrorKind
|
||||
|
||||
|
||||
def test_offload_entry_payload_returns_dict():
|
||||
"""
|
||||
Shape contract: _offload_entry_payload returns a dict.
|
||||
The actual happy-path and error-path tests are in test_app_controller_offloading.py.
|
||||
"""
|
||||
with patch("src.app_controller.performance_monitor.PerformanceMonitor"):
|
||||
ctrl = AppController()
|
||||
out = ctrl._offload_entry_payload({"kind": "request", "payload": {"message": "hi"}, "ts": "12:00:00"})
|
||||
assert isinstance(out, dict)
|
||||
assert out["kind"] == "request"
|
||||
assert out["payload"]["message"] == "hi"
|
||||
|
||||
|
||||
def test_migrated_method_returns_result_on_success():
|
||||
"""
|
||||
Pattern template: methods migrated to Result[T] return Result[data=...] with no errors
|
||||
on the success path. The 5 callback-handler sites (Batch 1) include _handle_custom_callback
|
||||
and _handle_click; both return Result[None] after migration.
|
||||
"""
|
||||
from src.app_controller import _handle_custom_callback
|
||||
controller = MagicMock()
|
||||
controller._predefined_callbacks = {}
|
||||
# Use a no-op callback so the body runs without error
|
||||
task = {"callback": (lambda: None), "args": []}
|
||||
result = _handle_custom_callback(controller, task)
|
||||
assert isinstance(result, Result)
|
||||
assert result.ok is True
|
||||
assert result.errors == []
|
||||
|
||||
|
||||
def test_migrated_method_returns_result_with_error_on_failure():
|
||||
"""
|
||||
Pattern template: methods migrated to Result[T] return Result with errors=... when
|
||||
the underlying call raises. The migrated callback sites catch the specific exception
|
||||
(e.g. TypeError, ValueError) and convert it to ErrorInfo.
|
||||
"""
|
||||
from src.app_controller import _handle_custom_callback
|
||||
controller = MagicMock()
|
||||
controller._predefined_callbacks = {}
|
||||
# Callback that raises a specific exception
|
||||
def bad_cb():
|
||||
raise ValueError("simulated failure")
|
||||
task = {"callback": bad_cb, "args": []}
|
||||
result = _handle_custom_callback(controller, task)
|
||||
assert isinstance(result, Result)
|
||||
assert result.ok is False
|
||||
assert any("ValueError" in e.message or "simulated failure" in e.message for e in result.errors)
|
||||
|
||||
|
||||
def test_app_controller_does_not_use_broad_except():
|
||||
"""
|
||||
Static check: src/app_controller.py has no `except Exception:` clauses left
|
||||
(all 32 are migrated to specific exceptions).
|
||||
"""
|
||||
import ast
|
||||
import pathlib
|
||||
src = pathlib.Path("src/app_controller.py").read_text(encoding="utf-8")
|
||||
tree = ast.parse(src)
|
||||
broad_catch_lines: list[int] = []
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.ExceptHandler) and node.type is not None:
|
||||
# `except Exception:` (bare Exception class as the type)
|
||||
if isinstance(node.type, ast.Name) and node.type.id == "Exception":
|
||||
broad_catch_lines.append(node.lineno)
|
||||
# `except (Exception,):` or `except (Exception, Foo):`
|
||||
elif isinstance(node.type, ast.Tuple):
|
||||
for elt in node.type.elts:
|
||||
if isinstance(elt, ast.Name) and elt.id == "Exception":
|
||||
broad_catch_lines.append(node.lineno)
|
||||
break
|
||||
assert broad_catch_lines == [], (
|
||||
f"src/app_controller.py still has {len(broad_catch_lines)} `except Exception` clauses at lines {broad_catch_lines}. "
|
||||
f"All 32 INTERNAL_BROAD_CATCH sites must be migrated to specific exceptions."
|
||||
)
|
||||
|
||||
|
||||
def test_offload_entry_payload_preserves_unchanged_payload():
|
||||
"""
|
||||
Verifies the no-op path: entries without tool_call or tool_result kinds are returned
|
||||
with the payload unchanged (no [REF:...] rewriting).
|
||||
"""
|
||||
with patch("src.app_controller.performance_monitor.PerformanceMonitor"):
|
||||
ctrl = AppController()
|
||||
entry = {"kind": "request", "payload": {"message": "hi"}, "ts": "12:00:00"}
|
||||
out = ctrl._offload_entry_payload(entry)
|
||||
assert out == entry
|
||||
Reference in New Issue
Block a user