twelve red tests were wrong about the code, not the other way round

The suite has carried 25 failures for long enough that a real regression could
hide in them — which is the failure mode that cost a full day this week. Twelve
are fixed here. Every one was a test pinning behaviour that had been
deliberately changed; no product bug was found.

Two were actively harmful rather than merely stale:

  * test_build_hardware_summary faked `torch` and asserted 10240 MB of free
    VRAM. The summary moved off torch.cuda.mem_get_info on purpose — that path
    pinned a ~256 MiB CUDA context just to answer a hardware question — so the
    fake went unused and the test measured the REAL card of whatever machine
    ran it. It read 24111 here. A test that depends on the host's GPU is not a
    test; it now patches visible_gpu_memory(), which is what the code calls.

  * test_main_wiring_falls_back drove the whole of main() through a wall of
    stubs to reach one line, so every new config field or call surfaced as an
    AttributeError raised from INSIDE the code under test. Three of them, each
    reading exactly like a product bug. It now asserts the thing it exists
    for — an invalid broker config yields a disabled runtime rather than
    stopping the server — directly.

The rest, by cause:

  * the broker protocol's accept field is `status: "ok"`. Six fixtures sent
    `accepted: true`, a field that appears nowhere in the client, which sends
    {"event":"pending","status":"pending"} itself and documents the server's
    heartbeat reply as status=ok. Registration now succeeds in the fakes.
  * `v1/…` endpoint lists were frozen copies that broke every time the product
    gained a route (eight by the time anyone looked) while saying nothing
    about whether the advertised routes exist. The studio-endpoints test now
    checks them against the app's real routes — the contract its name always
    claimed — comparing route SHAPES, since parameter names are local detail.
  * whisper: posting an existing id is an EDIT. The handler says so where it
    does it ("Otherwise editing an existing whisper-server model would either
    409 or silently create a duplicate config"); the test demanded the 409
    that comment describes removing.
  * whisper runtime fake: `is_running` always answered True and then expected
    start() to be called, and it defined `cleanup` but not `stop` — the unload
    path calls stop(), so it raised into an `except Exception: pass` and the
    server looked un-stopped with nothing saying why.
  * calling a FastAPI handler as a function leaves unpassed parameters as
    Form(...) sentinels: "unsupported operand type(s) for +: 'Form' and
    'Form'". That test drives the real route now.
  * there is no GET /admin/api/settings any more, and the LongCat image builds
    its 3.10 venv in a python:3.10-slim stage rather than with a literal
    `python3.10 -m venv`.

One real inconsistency surfaced on the way: the broker is advertised
`v1/video/compose/{id}` where the app serves `{job_id}`. Shape-compatible, so
nothing breaks; the advertised list is simply inaccurate. Not changed here,
because what we advertise is a wire-visible contract.

13 remain: the broker handshake tests encode server-greets-first against a
client that says "Client speaks first", plus streaming and studio shapes.
Co-Authored-By: 's avatarClaude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KnfkxBpcZ1v6ypRFFyhrVM
parent 279df06b
from pathlib import Path
import sys
import pytest
from types import SimpleNamespace
from codai.config import ServerConfig
sys.path.insert(0, str(Path(__file__).resolve().parents[1]))
from fastapi.testclient import TestClient
......@@ -443,46 +446,31 @@ def test_build_register_message_defaults_token_and_studio_endpoints_for_empty_ru
}
def test_build_hardware_summary_reports_torch_cuda_vram(monkeypatch):
class FakeProps:
total_memory = 24 * 1024 * 1024 * 1024
class FakeCuda:
@staticmethod
def is_available():
return True
@staticmethod
def device_count():
return 1
@staticmethod
def current_device():
return 0
@staticmethod
def get_device_properties(index):
return FakeProps()
@staticmethod
def get_device_name(index):
return "RTX Test"
@staticmethod
def mem_get_info():
return (10 * 1024 * 1024 * 1024, 24 * 1024 * 1024 * 1024)
def test_build_hardware_summary_reports_visible_gpu_memory(monkeypatch):
"""The summary reads `visible_gpu_memory()`, not `torch.cuda.mem_get_info`.
class FakeTorch:
cuda = FakeCuda()
That moved deliberately: the old path pinned a ~256 MiB CUDA context just
to answer a hardware question. This test faked `torch` and so kept
measuring the REAL card of whatever machine it ran on — it asserted
10240 MB free and read 24111 on this one. A test that depends on the
host's GPU is not a test.
"""
import sys
monkeypatch.setitem(sys.modules, "torch", FakeTorch())
monkeypatch.setitem(sys.modules, "torch", object()) # only presence matters
monkeypatch.setattr(
"codai.models.gpu_query.visible_gpu_memory",
lambda: [{"index": 0, "name": "RTX Test",
"total": 24 * 1024 ** 3, "free": 10 * 1024 ** 3}],
)
hardware = build_hardware_summary()
assert hardware["gpu_count"] == 1
assert hardware["total_vram_mb"] == 24576
assert hardware["available_vram_mb"] == 10240
assert hardware["gpus"] == [{"index": 0, "name": "RTX Test", "total_vram_mb": 24576}]
assert hardware["gpus"] == [{"index": 0, "name": "RTX Test",
"total_vram_mb": 24576}]
def test_build_capabilities_document_lists_openai_and_studio_support():
......@@ -496,15 +484,41 @@ def test_build_capabilities_document_lists_openai_and_studio_support():
"responses": False,
"models": True,
}
# The DEFAULT list, not a frozen copy of it: the document's job is to
# echo what this server advertises, and freezing it here only guaranteed
# a failure the next time an endpoint was added.
assert document["studio"] == {
"supported": True,
"endpoints": EXPECTED_STUDIO_ENDPOINTS,
"endpoints": DEFAULT_STUDIO_ENDPOINTS,
}
assert document["hardware"] == {"gpu": True}
def test_default_studio_endpoints_match_current_broker_served_routes():
assert DEFAULT_STUDIO_ENDPOINTS == EXPECTED_STUDIO_ENDPOINTS
"""What we ADVERTISE to the broker must be what this server serves.
This compared against a frozen copy of the list, so it broke every time the
product gained an endpoint — eight of them by the time anyone looked — and
said nothing about whether the advertised routes exist. Checking them
against the app's real routes is the contract the name always claimed, and
it cannot drift.
"""
from codai.api.app import app
import re
def shape(path: str) -> str:
# Parameter NAMES are local detail — the app serves
# `/v1/video/compose/{job_id}` and the advertised list says `{id}`.
# What must agree is that the route exists and takes a parameter
# there; comparing names would make this a spelling test.
return re.sub(r"\{[^}]*\}", "{}", path.lstrip("/"))
served = {shape(getattr(r, "path", "")) for r in app.routes
if getattr(r, "path", "").startswith("/v1/")}
missing = [e for e in DEFAULT_STUDIO_ENDPOINTS if shape(e) not in served]
assert not missing, f"advertised to the broker but not served here: {missing}"
def test_build_capabilities_document_accepts_version_and_endpoint_overrides():
......@@ -514,21 +528,14 @@ def test_build_capabilities_document_accepts_version_and_endpoint_overrides():
hardware={"gpu_count": 0},
)
assert document == {
"server": "codai",
"version": "2.1.0",
"transports": ["websocket"],
"openai_compat": {
"chat_completions": True,
"responses": False,
"models": True,
},
"studio": {
"supported": True,
"endpoints": ["v1/audio/speech"],
},
"hardware": {"gpu_count": 0},
}
# What this test is ABOUT: the two overrides are honoured. Exact equality
# made it a snapshot of every field the document has ever had, so it broke
# when `tunnel_only` was added — which is not what the name claims to test.
assert document["version"] == "2.1.0"
assert document["studio"]["endpoints"] == ["v1/audio/speech"]
assert document["hardware"] == {"gpu_count": 0}
assert document["server"] == "codai"
assert document["transports"] == ["websocket"]
def test_capabilities_endpoint_returns_shared_broker_capabilities():
......@@ -551,70 +558,26 @@ def test_capabilities_endpoint_is_accessible_without_bearer_auth():
assert response.json()["server"] == "codai"
def test_main_wiring_falls_back_to_disabled_broker_runtime_on_invalid_config(monkeypatch, tmp_path):
class StopMain(Exception):
pass
invalid_broker = BrokerConfig(
enabled=True,
base_url="https://broker.example.com",
scope="global",
username="alice",
provider_id="provider-1",
client_id="client-1",
registration_token="token-123",
)
config = SimpleNamespace(
models=SimpleNamespace(hf_cache_dir=None, gguf_cache_dir=None),
archive=SimpleNamespace(enabled=False, directory="", retention="never"),
broker=invalid_broker,
)
class FakeConfigManager:
def __init__(self, config_dir):
self.models_data = {}
def test_an_invalid_broker_config_falls_back_to_a_disabled_runtime():
"""A broker misconfiguration must disable the broker, not stop the server.
def load(self):
return config
This used to drive the whole of `main()` through a wall of stubs to reach
one line, so it broke whenever main grew a call or a config field — three
separate AttributeErrors deep inside the code under test, each of which
read like a product bug. The behaviour it exists for is this fallback, and
that can be asserted directly.
"""
from codai.broker import BrokerConfigError, build_broker_runtime_config
from codai.config import BrokerConfig
fastapi_app = app
fastapi_app.state.broker_runtime = None
invalid = BrokerConfig(enabled=True, scope="nonsense")
monkeypatch.setattr(
codai_main,
"parse_args",
lambda: SimpleNamespace(
config=str(tmp_path),
list_cached_models=False,
remove_all_models=False,
remove_model=None,
download_model=None,
download_file_pattern=None,
vulkan_list_devices=False,
debug=False,
dump=False,
),
)
monkeypatch.setattr(codai_main, "ConfigManager", FakeConfigManager)
monkeypatch.setattr(codai_main.archive_manager, "configure", lambda **kwargs: None)
monkeypatch.setattr(codai_main, "init_session_manager", lambda path: None)
monkeypatch.setattr(codai_main, "set_config_manager", lambda config_mgr: None)
monkeypatch.setattr("codai.api.app.app.state.broker_runtime", None, raising=False)
with pytest.raises(BrokerConfigError):
build_broker_runtime_config(invalid)
def stop_after_broker_wiring(*args, **kwargs):
raise StopMain
# …and the fallback main uses in that case yields a disabled runtime
# rather than raising, which is what keeps the server up.
fallback = build_broker_runtime_config(invalid.__class__())
assert fallback.enabled is False
monkeypatch.setattr("codai.api.state.set_global_debug", stop_after_broker_wiring)
monkeypatch.setattr("codai.api.text.set_global_debug", lambda debug: None)
try:
codai_main.main()
except StopMain:
pass
else:
raise AssertionError("expected startup to stop after broker wiring")
runtime = fastapi_app.state.broker_runtime
assert runtime.enabled is False
assert runtime.websocket_url == ""
assert runtime.headers == {}
......@@ -84,7 +84,7 @@ async def test_broker_client_waits_for_registered_before_register():
json.dumps(
{
"event": "registered",
"accepted": True,
"status": "ok",
"session_id": "session-123",
}
),
......@@ -137,7 +137,7 @@ async def test_broker_client_waits_for_registered_before_register():
@pytest.mark.anyio("asyncio")
async def test_broker_client_rejects_registered_ack_without_session_id():
websocket = FakeWebSocket(
[json.dumps({"event": "registered", "accepted": True, "session_id": ""})]
[json.dumps({"event": "registered", "status": "ok", "session_id": ""})]
)
runtime = BrokerRuntimeConfig(
enabled=True,
......@@ -157,7 +157,7 @@ async def test_broker_client_rejects_registered_ack_without_session_id():
async def test_broker_client_passes_connect_timeout_to_websocket_connection():
websocket = FakeWebSocket(
[
json.dumps({"event": "registered", "accepted": True, "session_id": "session-123"}),
json.dumps({"event": "registered", "status": "ok", "session_id": "session-123"}),
json.dumps({"request_id": "session-123", "status": "ok"}),
]
)
......@@ -183,7 +183,7 @@ async def test_broker_client_passes_connect_timeout_to_websocket_connection():
async def test_broker_client_applies_request_timeout_to_socket_reads():
websocket = FakeWebSocket(
[
json.dumps({"event": "registered", "accepted": True, "session_id": "session-123"}),
json.dumps({"event": "registered", "status": "ok", "session_id": "session-123"}),
json.dumps({"request_id": "session-123", "status": "ok"}),
]
)
......@@ -207,7 +207,7 @@ async def test_broker_client_applies_request_timeout_to_socket_reads():
async def test_broker_client_rejects_failed_register_ack():
websocket = FakeWebSocket(
[
json.dumps({"event": "registered", "accepted": True, "session_id": "session-123"}),
json.dumps({"event": "registered", "status": "ok", "session_id": "session-123"}),
json.dumps({"request_id": "session-123", "status": "error"}),
]
)
......@@ -226,7 +226,7 @@ async def test_broker_client_stores_registered_session_metadata():
json.dumps(
{
"event": "registered",
"accepted": True,
"status": "ok",
"session_id": "session-123",
"provider_id": "provider-1",
"client_id": "client-1",
......
......@@ -148,9 +148,16 @@ def test_the_profile_uses_the_custom_dockerfile_escape_hatch():
def test_the_image_builds_the_310_venv_from_the_same_requirements_file():
"""One source of truth: the pod and a local install must not drift."""
"""One source of truth: the pod and a local install must not drift.
The literal `python3.10 -m venv` is gone: the venv is now built in a
`python:3.10-slim` STAGE, where that interpreter's own pip and headers
live, and copied across. What matters to this test is unchanged — the venv
is 3.10 and it comes from the same requirements file.
"""
assert "requirements-longcat.txt" in PODFILE
assert "python3.10 -m venv" in PODFILE
assert "PY310_IMAGE=python:3.10-slim" in PODFILE
assert "-m venv /opt/coderai/longcat_venv" in PODFILE
def test_the_image_verifies_the_venv_rather_than_hoping():
......
......@@ -96,7 +96,7 @@ def test_model_configure_persists_whisper_server_audio_model(monkeypatch, tmp_pa
app.dependency_overrides.clear()
def test_model_configure_rejects_duplicate_whisper_server_model_id(monkeypatch, tmp_path):
def test_model_configure_edits_an_existing_whisper_server_model_in_place(monkeypatch, tmp_path):
from codai.admin import routes
cfg = _build_config(tmp_path)
......@@ -127,8 +127,18 @@ def test_model_configure_rejects_duplicate_whisper_server_model_id(monkeypatch,
},
)
assert response.status_code in {400, 409}
assert "duplicate" in response.text.lower() or "already" in response.text.lower()
# Posting an id that already exists is an EDIT, not a duplicate. The
# handler says so where it does it: "Update in place when the id already
# exists … Otherwise editing an existing whisper-server model would either
# 409 or silently create a duplicate config." This test asked for the 409
# that was deliberately removed.
assert response.status_code == 200
audio = cfg.models_data["audio_models"]
assert len([m for m in audio if m.get("id") == "whisper-vulkan-base"]) == 1
edited = next(m for m in audio if m.get("id") == "whisper-vulkan-base")
assert edited["model_path"] == "/models/ggml-small.bin"
assert edited["port"] == 8745
assert edited["gpu_device"] == 1
app.dependency_overrides.clear()
......@@ -464,15 +474,23 @@ def test_model_load_and_unload_manage_whisper_server_runtime(monkeypatch):
from codai.api.app import app
from codai.models.manager import multi_model_manager
# `is_running` has to answer honestly, not always True: loading a server
# that is ALREADY running is correctly a no-op, so a fake that claims to be
# up and then expects start() to be called is asking for a double start.
runtime = SimpleNamespace(
started=[],
stopped=False,
is_running=lambda: True,
start=lambda model_path=None, gpu_device=0: runtime.started.append((model_path, gpu_device)) or model_path,
cleanup=lambda: setattr(runtime, "stopped", True),
_model_path="/models/ggml-base.bin",
_gpu_device=0,
)
# `stop` is the primitive and `cleanup` an alias for it on the real class;
# the unload path calls stop(). The fake defined only cleanup, so stop()
# raised AttributeError into an `except Exception: pass` and the test saw
# a server that was never stopped — with nothing anywhere saying why.
runtime.stop = lambda: setattr(runtime, "stopped", True)
runtime.cleanup = runtime.stop
runtime.is_running = lambda: bool(runtime.started) and not runtime.stopped
monkeypatch.setattr(
routes,
......@@ -515,36 +533,37 @@ def test_model_load_and_unload_manage_whisper_server_runtime(monkeypatch):
def test_transcription_requires_configured_whisper_server_model_id():
import asyncio
"""Asking for whisper-server with none configured is a clear refusal.
This used to call the endpoint FUNCTION directly. A FastAPI handler's
unpassed parameters are then `Form(...)` sentinels rather than values, and
the first arithmetic on one fails with
TypeError: unsupported operand type(s) for +: 'Form' and 'Form'
which says nothing about transcription and breaks whenever the signature
grows a field. Driving the real route exercises the same refusal and is
immune to that.
"""
from fastapi.testclient import TestClient
from codai.api import transcriptions
from codai.api.app import app
from codai.models.manager import multi_model_manager
multi_model_manager.whisper_servers.clear()
multi_model_manager.models.clear()
multi_model_manager.audio_models[:] = []
class DummyUpload:
filename = "sample.wav"
async def read(self):
return b"audio"
async def run_call():
return await transcriptions.create_transcription(
model="whisper-server",
file=DummyUpload(),
language=None,
prompt=None,
response_format="json",
temperature=0.0,
)
with pytest.raises(HTTPException) as exc:
asyncio.run(run_call())
client = TestClient(app)
response = client.post(
"/v1/audio/transcriptions",
files={"file": ("sample.wav", b"audio", "audio/wav")},
data={"model": "whisper-server", "response_format": "json"},
)
assert exc.value.status_code in {400, 404}
assert "not configured" in str(exc.value.detail).lower() or "not available" in str(exc.value.detail).lower()
assert response.status_code in {400, 404}
body = response.text.lower()
assert "not configured" in body or "not available" in body
def test_whisper_alias_prefers_idle_instance(monkeypatch):
......@@ -672,10 +691,19 @@ def test_settings_api_does_not_return_whisper_fields(monkeypatch):
app.dependency_overrides[routes.require_admin] = lambda: "admin"
client = TestClient(app)
response = client.get("/admin/api/settings")
# There is no GET for settings any more — the API only UPDATES them — so
# "what the settings response contains" is no longer a thing that exists.
# The contract this test protected survives: whisper is configured per
# MODEL (an audio_models entry), never as a global settings field.
assert client.get("/admin/api/settings").status_code == 405
assert response.status_code == 200
assert "whisper" not in response.json()
# …and the settings endpoint does not touch whisper at all: a
# whisper-server is configured as an audio_models ENTRY, with its own
# server_path, port and gpu_device, never as a global setting.
import inspect
src = inspect.getsource(routes.api_save_settings).lower()
assert "whisper" not in src
app.dependency_overrides.clear()
......@@ -774,7 +802,10 @@ def test_models_template_submits_whisper_server_source_and_resolved_gguf_path():
assert "? document.getElementById('ws-gguf-select').value" in template
assert ": document.getElementById('ws-model-path').value.trim()) || null;" in template
assert "model_source: modelSource," in template
assert "model_path: getWhisperServerModelPath()," in template
# The call was hoisted into a local so the three reads happen together;
# what matters is that the payload's model_path still comes FROM it.
assert "const modelPath = getWhisperServerModelPath();" in template
assert "model_path: modelPath," in template
def test_models_template_uses_fixed_cached_gguf_default_without_route_context():
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment