kai/smart-support
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: engineering improvements -- API versioning, structured logging, Alembic, error standardization, test coverage

Browse Source 
- API versioning: all REST endpoints prefixed with /api/v1/
- Structured logging: replaced stdlib logging with structlog (console/JSON modes)
- Alembic migrations: versioned DB schema with initial migration
- Error standardization: global exception handlers for consistent envelope format
- Interrupt cleanup: asyncio background task for expired interrupt removal
- Integration tests: +30 tests (analytics, replay, openapi, error, session APIs)
- Frontend tests: +57 tests (all components, pages, useWebSocket hook)
- Backend: 557 tests, 89.75% coverage | Frontend: 80 tests, 16 test files
This commit is contained in:
Yaojia Wang
2026-04-06 23:19:29 +02:00
parent af53111928
commit f0699436c5
59 changed files with 2846 additions and 149 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
backend/tests/unit/analytics/test_api.py
View File

@@ -44,7 +44,7 @@ def _make_analytics_result() -> object:
)
def _get_analytics(app: FastAPI, path: str = "/api/analytics", **patch_kwargs: object) -> object:
def _get_analytics(app: FastAPI, path: str = "/api/v1/analytics", **patch_kwargs: object) -> object:
"""Helper: patch get_analytics, make request, return (response, mock)."""
analytics_result = _make_analytics_result()
with (
@@ -84,7 +84,7 @@ class TestAnalyticsEndpoint:
def test_custom_range_7d(self) -> None:
app = _build_app()
app.state.pool = _make_mock_pool()
resp, mock_ga = _get_analytics(app, "/api/analytics?range=7d")
resp, mock_ga = _get_analytics(app, "/api/v1/analytics?range=7d")
assert resp.status_code == 200
mock_ga.assert_called_once()
@@ -94,7 +94,7 @@ class TestAnalyticsEndpoint:
def test_custom_range_30d(self) -> None:
app = _build_app()
app.state.pool = _make_mock_pool()
resp, mock_ga = _get_analytics(app, "/api/analytics?range=30d")
resp, mock_ga = _get_analytics(app, "/api/v1/analytics?range=30d")
assert resp.status_code == 200
call_kwargs = mock_ga.call_args
@@ -107,7 +107,7 @@ class TestAnalyticsEndpoint:
app.state.pool = _make_mock_pool()
with TestClient(app) as client:
resp = client.get("/api/analytics?range=invalid")
resp = client.get("/api/v1/analytics?range=invalid")
assert resp.status_code == 400
@@ -116,7 +116,7 @@ class TestAnalyticsEndpoint:
app.state.pool = _make_mock_pool()
with TestClient(app) as client:
resp = client.get("/api/analytics?range=7")
resp = client.get("/api/v1/analytics?range=7")
assert resp.status_code == 400

52
backend/tests/unit/openapi/test_review_api.py
View File

@@ -28,7 +28,7 @@ def client():
@pytest.fixture
def job_id(client):
"""Create a job and return its ID."""
response = client.post("/api/openapi/import", json={"url": _SAMPLE_URL})
response = client.post("/api/v1/openapi/import", json={"url": _SAMPLE_URL})
assert response.status_code == 202
return response.json()["job_id"]
@@ -61,11 +61,11 @@ def job_with_classifications(client, job_id):
class TestImportEndpoint:
"""Tests for POST /api/openapi/import."""
"""Tests for POST /api/v1/openapi/import."""
def test_post_import_returns_job_id(self, client) -> None:
"""POST /import returns 202 with a job_id."""
response = client.post("/api/openapi/import", json={"url": _SAMPLE_URL})
response = client.post("/api/v1/openapi/import", json={"url": _SAMPLE_URL})
assert response.status_code == 202
data = response.json()
assert "job_id" in data
@@ -73,38 +73,38 @@ class TestImportEndpoint:
def test_post_import_empty_url_returns_422(self, client) -> None:
"""POST /import with empty URL returns 422 validation error."""
response = client.post("/api/openapi/import", json={"url": ""})
response = client.post("/api/v1/openapi/import", json={"url": ""})
assert response.status_code == 422
def test_post_import_missing_url_returns_422(self, client) -> None:
"""POST /import with missing URL field returns 422."""
response = client.post("/api/openapi/import", json={})
response = client.post("/api/v1/openapi/import", json={})
assert response.status_code == 422
def test_post_import_invalid_scheme_returns_422(self, client) -> None:
"""POST /import with non-http URL returns 422."""
response = client.post("/api/openapi/import", json={"url": "ftp://evil.com/spec"})
response = client.post("/api/v1/openapi/import", json={"url": "ftp://evil.com/spec"})
assert response.status_code == 422
def test_post_import_returns_pending_status(self, client) -> None:
"""Newly created job has pending status."""
response = client.post("/api/openapi/import", json={"url": _SAMPLE_URL})
response = client.post("/api/v1/openapi/import", json={"url": _SAMPLE_URL})
data = response.json()
assert data["status"] == "pending"
def test_post_import_returns_spec_url(self, client) -> None:
"""Response includes the original spec URL."""
response = client.post("/api/openapi/import", json={"url": _SAMPLE_URL})
response = client.post("/api/v1/openapi/import", json={"url": _SAMPLE_URL})
data = response.json()
assert data["spec_url"] == _SAMPLE_URL
class TestGetJobEndpoint:
"""Tests for GET /api/openapi/jobs/{job_id}."""
"""Tests for GET /api/v1/openapi/jobs/{job_id}."""
def test_get_job_returns_status(self, client, job_id) -> None:
"""GET /jobs/{id} returns job status."""
response = client.get(f"/api/openapi/jobs/{job_id}")
response = client.get(f"/api/v1/openapi/jobs/{job_id}")
assert response.status_code == 200
data = response.json()
assert "status" in data
@@ -112,23 +112,23 @@ class TestGetJobEndpoint:
def test_get_unknown_job_returns_404(self, client) -> None:
"""GET /jobs/nonexistent returns 404."""
response = client.get("/api/openapi/jobs/nonexistent-id")
response = client.get("/api/v1/openapi/jobs/nonexistent-id")
assert response.status_code == 404
def test_get_job_includes_spec_url(self, client, job_id) -> None:
"""Job response includes the spec URL."""
response = client.get(f"/api/openapi/jobs/{job_id}")
response = client.get(f"/api/v1/openapi/jobs/{job_id}")
data = response.json()
assert data["spec_url"] == _SAMPLE_URL
class TestGetClassificationsEndpoint:
"""Tests for GET /api/openapi/jobs/{job_id}/classifications."""
"""Tests for GET /api/v1/openapi/jobs/{job_id}/classifications."""
def test_get_classifications_returns_list(self, client, job_with_classifications) -> None:
"""GET /classifications returns a list."""
response = client.get(
f"/api/openapi/jobs/{job_with_classifications}/classifications"
f"/api/v1/openapi/jobs/{job_with_classifications}/classifications"
)
assert response.status_code == 200
data = response.json()
@@ -137,13 +137,13 @@ class TestGetClassificationsEndpoint:
def test_get_classifications_unknown_job_returns_404(self, client) -> None:
"""GET /classifications for unknown job returns 404."""
response = client.get("/api/openapi/jobs/unknown/classifications")
response = client.get("/api/v1/openapi/jobs/unknown/classifications")
assert response.status_code == 404
def test_classification_has_expected_fields(self, client, job_with_classifications) -> None:
"""Each classification item has access_type and endpoint fields."""
response = client.get(
f"/api/openapi/jobs/{job_with_classifications}/classifications"
f"/api/v1/openapi/jobs/{job_with_classifications}/classifications"
)
item = response.json()[0]
assert "access_type" in item
@@ -152,12 +152,12 @@ class TestGetClassificationsEndpoint:
class TestUpdateClassificationEndpoint:
"""Tests for PUT /api/openapi/jobs/{job_id}/classifications/{idx}."""
"""Tests for PUT /api/v1/openapi/jobs/{job_id}/classifications/{idx}."""
def test_update_classification_succeeds(self, client, job_with_classifications) -> None:
"""PUT /classifications/0 updates the classification."""
response = client.put(
f"/api/openapi/jobs/{job_with_classifications}/classifications/0",
f"/api/v1/openapi/jobs/{job_with_classifications}/classifications/0",
json={"access_type": "write", "needs_interrupt": True, "agent_group": "write_agent"},
)
assert response.status_code == 200
@@ -165,7 +165,7 @@ class TestUpdateClassificationEndpoint:
def test_update_unknown_job_returns_404(self, client) -> None:
"""PUT /classifications/0 for unknown job returns 404."""
response = client.put(
"/api/openapi/jobs/unknown/classifications/0",
"/api/v1/openapi/jobs/unknown/classifications/0",
json={"access_type": "write", "needs_interrupt": True, "agent_group": "write_agent"},
)
assert response.status_code == 404
@@ -173,7 +173,7 @@ class TestUpdateClassificationEndpoint:
def test_update_invalid_access_type_returns_422(self, client, job_with_classifications) -> None:
"""PUT /classifications/0 with invalid access_type returns 422."""
response = client.put(
f"/api/openapi/jobs/{job_with_classifications}/classifications/0",
f"/api/v1/openapi/jobs/{job_with_classifications}/classifications/0",
json={"access_type": "admin", "needs_interrupt": True, "agent_group": "x"},
)
assert response.status_code == 422
@@ -181,7 +181,7 @@ class TestUpdateClassificationEndpoint:
def test_update_invalid_agent_group_returns_422(self, client, job_with_classifications) -> None:
"""PUT /classifications/0 with invalid agent_group returns 422."""
response = client.put(
f"/api/openapi/jobs/{job_with_classifications}/classifications/0",
f"/api/v1/openapi/jobs/{job_with_classifications}/classifications/0",
json={"access_type": "read", "needs_interrupt": False, "agent_group": "evil group!"},
)
assert response.status_code == 422
@@ -189,31 +189,31 @@ class TestUpdateClassificationEndpoint:
def test_update_out_of_range_index_returns_404(self, client, job_with_classifications) -> None:
"""PUT /classifications/999 returns 404 for out-of-range index."""
response = client.put(
f"/api/openapi/jobs/{job_with_classifications}/classifications/999",
f"/api/v1/openapi/jobs/{job_with_classifications}/classifications/999",
json={"access_type": "read", "needs_interrupt": False, "agent_group": "read_agent"},
)
assert response.status_code == 404
class TestApproveEndpoint:
"""Tests for POST /api/openapi/jobs/{job_id}/approve."""
"""Tests for POST /api/v1/openapi/jobs/{job_id}/approve."""
def test_approve_job_succeeds(self, client, job_with_classifications) -> None:
"""POST /approve transitions job to approved status."""
response = client.post(
f"/api/openapi/jobs/{job_with_classifications}/approve"
f"/api/v1/openapi/jobs/{job_with_classifications}/approve"
)
assert response.status_code == 200
def test_approve_unknown_job_returns_404(self, client) -> None:
"""POST /approve for unknown job returns 404."""
response = client.post("/api/openapi/jobs/unknown/approve")
response = client.post("/api/v1/openapi/jobs/unknown/approve")
assert response.status_code == 404
def test_approve_returns_job_status(self, client, job_with_classifications) -> None:
"""POST /approve returns updated job status."""
response = client.post(
f"/api/openapi/jobs/{job_with_classifications}/approve"
f"/api/v1/openapi/jobs/{job_with_classifications}/approve"
)
data = response.json()
assert "status" in data

40
backend/tests/unit/replay/test_api.py
View File

@@ -5,9 +5,12 @@ from __future__ import annotations
from unittest.mock import AsyncMock, MagicMock
import pytest
from fastapi import FastAPI
from fastapi import FastAPI, HTTPException
from fastapi.responses import JSONResponse
from fastapi.testclient import TestClient
from app.api_utils import envelope
pytestmark = pytest.mark.unit
@@ -16,6 +19,14 @@ def _build_app() -> FastAPI:
app = FastAPI()
app.include_router(router)
@app.exception_handler(HTTPException)
async def _http_exc(request, exc): # type: ignore[no-untyped-def]
return JSONResponse(
status_code=exc.status_code,
content=envelope(None, success=False, error=exc.detail),
)
return app
@@ -64,7 +75,7 @@ class TestListConversations:
app.state.pool = _make_mock_pool([], count=0)
with TestClient(app) as client:
resp = client.get("/api/conversations")
resp = client.get("/api/v1/conversations")
assert resp.status_code == 200
body = resp.json()
assert body["success"] is True
@@ -89,7 +100,7 @@ class TestListConversations:
app.state.pool = _make_mock_pool(mock_rows, count=1)
with TestClient(app) as client:
resp = client.get("/api/conversations")
resp = client.get("/api/v1/conversations")
body = resp.json()
assert resp.status_code == 200
data = body["data"]
@@ -102,7 +113,7 @@ class TestListConversations:
app.state.pool = _make_mock_pool([], count=0)
with TestClient(app) as client:
resp = client.get("/api/conversations")
resp = client.get("/api/v1/conversations")
assert resp.status_code == 200
def test_pagination_custom_params(self) -> None:
@@ -110,7 +121,7 @@ class TestListConversations:
app.state.pool = _make_mock_pool([], count=0)
with TestClient(app) as client:
resp = client.get("/api/conversations?page=2&per_page=10")
resp = client.get("/api/v1/conversations?page=2&per_page=10")
assert resp.status_code == 200
def test_per_page_max_capped_at_100(self) -> None:
@@ -118,7 +129,7 @@ class TestListConversations:
app.state.pool = _make_mock_pool([], count=0)
with TestClient(app) as client:
resp = client.get("/api/conversations?per_page=200")
resp = client.get("/api/v1/conversations?per_page=200")
# FastAPI Query(le=100) rejects values > 100
assert resp.status_code == 422
@@ -129,7 +140,7 @@ class TestGetReplay:
app.state.pool = _make_mock_pool([])
with TestClient(app) as client:
resp = client.get("/api/replay/nonexistent-thread")
resp = client.get("/api/v1/replay/nonexistent-thread")
assert resp.status_code == 404
def test_returns_replay_page_for_existing_thread(self) -> None:
@@ -149,7 +160,7 @@ class TestGetReplay:
app.state.pool = _make_mock_pool(mock_rows)
with TestClient(app) as client:
resp = client.get("/api/replay/thread-123")
resp = client.get("/api/v1/replay/thread-123")
assert resp.status_code == 200
body = resp.json()
assert body["success"] is True
@@ -174,7 +185,7 @@ class TestGetReplay:
app.state.pool = _make_mock_pool(mock_rows)
with TestClient(app) as client:
resp = client.get("/api/replay/t1?page=1&per_page=5")
resp = client.get("/api/v1/replay/t1?page=1&per_page=5")
assert resp.status_code == 200
def test_error_response_has_envelope(self) -> None:
@@ -182,16 +193,19 @@ class TestGetReplay:
app.state.pool = _make_mock_pool([])
with TestClient(app) as client:
resp = client.get("/api/replay/missing")
resp = client.get("/api/v1/replay/missing")
assert resp.status_code == 404
assert "detail" in resp.json()
body = resp.json()
assert body["success"] is False
assert body["data"] is None
assert body["error"] is not None
def test_invalid_thread_id_returns_400(self) -> None:
app = _build_app()
app.state.pool = _make_mock_pool([])
with TestClient(app) as client:
resp = client.get("/api/replay/id%20with%20spaces")
resp = client.get("/api/v1/replay/id%20with%20spaces")
assert resp.status_code == 400
def test_thread_id_special_chars_returns_400(self) -> None:
@@ -199,5 +213,5 @@ class TestGetReplay:
app.state.pool = _make_mock_pool([])
with TestClient(app) as client:
resp = client.get("/api/replay/id;DROP TABLE")
resp = client.get("/api/v1/replay/id;DROP TABLE")
assert resp.status_code == 400

142
backend/tests/unit/test_error_responses.py Normal file
View File

@@ -0,0 +1,142 @@
"""Tests for standardized error response envelope format."""
from __future__ import annotations
import pytest
from fastapi import FastAPI, HTTPException
from fastapi.exceptions import RequestValidationError
from fastapi.responses import JSONResponse
from fastapi.testclient import TestClient
from pydantic import BaseModel, Field
from app.api_utils import envelope
pytestmark = pytest.mark.unit
def _build_test_app() -> FastAPI:
"""Build a minimal FastAPI app with the standard exception handlers."""
app = FastAPI()
@app.exception_handler(HTTPException)
async def http_exception_handler(request, exc): # type: ignore[no-untyped-def]
return JSONResponse(
status_code=exc.status_code,
content=envelope(None, success=False, error=exc.detail),
)
@app.exception_handler(RequestValidationError)
async def validation_exception_handler(request, exc): # type: ignore[no-untyped-def]
return JSONResponse(
status_code=422,
content=envelope(None, success=False, error=str(exc)),
)
@app.exception_handler(Exception)
async def general_exception_handler(request, exc): # type: ignore[no-untyped-def]
return JSONResponse(
status_code=500,
content=envelope(None, success=False, error="Internal server error"),
)
class ItemRequest(BaseModel):
name: str = Field(..., min_length=1)
count: int = Field(..., gt=0)
@app.get("/items/{item_id}")
def get_item(item_id: int) -> dict:
if item_id == 0:
raise HTTPException(status_code=400, detail="Invalid item ID")
if item_id == 999:
raise HTTPException(status_code=404, detail="Item not found")
if item_id == 401:
raise HTTPException(status_code=401, detail="Not authenticated")
return envelope({"id": item_id, "name": "test"})
@app.post("/items")
def create_item(item: ItemRequest) -> dict:
return envelope({"id": 1, "name": item.name})
@app.get("/crash")
def crash() -> dict:
msg = "unexpected failure"
raise RuntimeError(msg)
return app
class TestHttpExceptionEnvelope:
"""HTTPException responses use the standard envelope format."""
def test_400_returns_envelope(self) -> None:
app = _build_test_app()
with TestClient(app, raise_server_exceptions=False) as client:
resp = client.get("/items/0")
assert resp.status_code == 400
body = resp.json()
assert body["success"] is False
assert body["data"] is None
assert body["error"] == "Invalid item ID"
def test_404_returns_envelope(self) -> None:
app = _build_test_app()
with TestClient(app, raise_server_exceptions=False) as client:
resp = client.get("/items/999")
assert resp.status_code == 404
body = resp.json()
assert body["success"] is False
assert body["data"] is None
assert body["error"] == "Item not found"
def test_401_returns_envelope(self) -> None:
app = _build_test_app()
with TestClient(app, raise_server_exceptions=False) as client:
resp = client.get("/items/401")
assert resp.status_code == 401
body = resp.json()
assert body["success"] is False
assert body["data"] is None
assert body["error"] == "Not authenticated"
class TestValidationErrorEnvelope:
"""Validation errors return 422 with envelope format."""
def test_validation_error_returns_envelope(self) -> None:
app = _build_test_app()
with TestClient(app, raise_server_exceptions=False) as client:
resp = client.post("/items", json={"name": "", "count": -1})
assert resp.status_code == 422
body = resp.json()
assert body["success"] is False
assert body["data"] is None
assert isinstance(body["error"], str)
assert len(body["error"]) > 0
class TestGeneralExceptionEnvelope:
"""Unhandled exceptions return 500 with safe envelope."""
def test_unhandled_exception_returns_500_envelope(self) -> None:
app = _build_test_app()
with TestClient(app, raise_server_exceptions=False) as client:
resp = client.get("/crash")
assert resp.status_code == 500
body = resp.json()
assert body["success"] is False
assert body["data"] is None
assert body["error"] == "Internal server error"
class TestSuccessResponseUnchanged:
"""Success responses still work normally."""
def test_success_returns_envelope(self) -> None:
app = _build_test_app()
with TestClient(app) as client:
resp = client.get("/items/42")
assert resp.status_code == 200
body = resp.json()
assert body["success"] is True
assert body["data"]["id"] == 42
assert body["error"] is None

86
backend/tests/unit/test_interrupt_cleanup.py Normal file
View File

@@ -0,0 +1,86 @@
"""Tests for the interrupt cleanup background loop in main.py."""
from __future__ import annotations
import asyncio
import logging
from unittest.mock import MagicMock, patch
import pytest
from app.main import _interrupt_cleanup_loop
@pytest.mark.unit
@pytest.mark.asyncio
async def test_cleanup_loop_calls_cleanup_expired() -> None:
"""The loop should call cleanup_expired after each sleep interval."""
manager = MagicMock()
manager.cleanup_expired.return_value = ()
call_count = 0
original_sleep = asyncio.sleep
async def _fake_sleep(seconds: float) -> None:
nonlocal call_count
call_count += 1
if call_count >= 2:
raise asyncio.CancelledError
await original_sleep(0)
with patch("app.main.asyncio.sleep", side_effect=_fake_sleep):
with pytest.raises(asyncio.CancelledError):
await _interrupt_cleanup_loop(manager, interval=60)
assert manager.cleanup_expired.call_count >= 1
@pytest.mark.unit
@pytest.mark.asyncio
async def test_cleanup_loop_survives_exceptions() -> None:
"""The loop should not die when cleanup_expired raises an exception."""
manager = MagicMock()
manager.cleanup_expired.side_effect = [RuntimeError("db gone"), ()]
call_count = 0
original_sleep = asyncio.sleep
async def _fake_sleep(seconds: float) -> None:
nonlocal call_count
call_count += 1
if call_count >= 3:
raise asyncio.CancelledError
await original_sleep(0)
with patch("app.main.asyncio.sleep", side_effect=_fake_sleep):
with pytest.raises(asyncio.CancelledError):
await _interrupt_cleanup_loop(manager, interval=60)
# Should have been called twice: once raising, once returning ()
assert manager.cleanup_expired.call_count == 2
@pytest.mark.unit
@pytest.mark.asyncio
async def test_cleanup_loop_logs_expired_count(capsys: pytest.CaptureFixture[str]) -> None:
"""The loop should log when expired interrupts are found."""
fake_record = MagicMock()
manager = MagicMock()
manager.cleanup_expired.return_value = (fake_record, fake_record)
call_count = 0
original_sleep = asyncio.sleep
async def _fake_sleep(seconds: float) -> None:
nonlocal call_count
call_count += 1
if call_count >= 2:
raise asyncio.CancelledError
await original_sleep(0)
with patch("app.main.asyncio.sleep", side_effect=_fake_sleep):
with pytest.raises(asyncio.CancelledError):
await _interrupt_cleanup_loop(manager, interval=60)
captured = capsys.readouterr()
assert "2 expired interrupt" in captured.out

20
backend/tests/unit/test_logging_config.py Normal file
View File

@@ -0,0 +1,20 @@
"""Tests for structured logging configuration."""
from __future__ import annotations
import pytest
from app.logging_config import configure_logging
pytestmark = pytest.mark.unit
def test_configure_logging_console_mode() -> None:
"""Console mode configures without error."""
configure_logging("console")
def test_configure_logging_json_mode() -> None:
"""JSON mode configures without error."""
configure_logging("json")

2
backend/tests/unit/test_main.py
View File

@@ -36,7 +36,7 @@ class TestMainModule:
def test_health_route_registered(self) -> None:
routes = [r.path for r in app.routes if hasattr(r, "path")]
assert "/api/health" in routes
assert "/api/v1/health" in routes
def test_app_version_is_0_5_0(self) -> None:
assert app.version == "0.6.0"