alexei.dolgolyov/ledgrab
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 14 Wiki Activity

fix(setup): register scaffolded target with ProcessorManager + final-review hardening

Browse Source 
Final-review blocker: the setup scaffold created the LED output target in the
store but never registered it with the ProcessorManager, so the wizard's
"Start" step 404'd on a fresh setup (target not found) — the lights never
started despite a success screen. Now the scaffold calls
target.register_with_manager(manager) right after create (mirroring the
canonical POST /output-targets route, same ValueError guard), so
start_processing finds the target. Rollback unregisters via
manager.remove_target before deleting the store entity, so a post-registration
failure leaves no half-registered target.

Also from the final review:
- solve corner_indices elements now bounded ge=0 (clear 422 instead of silent
  modulo-wrap).
- setup-wizard.ts: reuse tutorials' suppressGettingStartedTour()/TOUR_KEY
  instead of a duplicated 'tour_completed' literal; drop a duplicate manual-form
  submit listener.

Tests: + adversarial pass over the whole feature (solver/session/scaffold edge
cases) and a scaffold->register->startable regression test. Full suite
2149 passed / 2 skipped; tsc clean; build passes; ruff clean.
This commit is contained in:
Admin
2026-06-08 16:55:36 +03:00
parent 81b18089e1
commit 6cd5e057da
7 changed files with 1730 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/tests/api/routes/test_setup_routes.py
+87 -1
View File
@@ -21,7 +21,7 @@ from __future__ import annotations
import pytest
from fastapi import FastAPI
from fastapi.testclient import TestClient
from unittest.mock import patch
from unittest.mock import MagicMock, patch
# ---------------------------------------------------------------------------
@@ -89,6 +89,14 @@ def event_log():
return log
@pytest.fixture
def mock_manager():
"""A MagicMock ProcessorManager that silently accepts all calls."""
mgr = MagicMock()
mgr.remove_target = MagicMock()
return mgr
@pytest.fixture
def setup_client(
tmp_db,
@@ -98,6 +106,7 @@ def setup_client(
css_store,
output_target_store,
event_log,
mock_manager,
):
from ledgrab.api.routes.setup import router
from ledgrab.api.auth import verify_api_key
@@ -111,6 +120,7 @@ def setup_client(
app.dependency_overrides[deps.get_picture_source_store] = lambda: picture_source_store
app.dependency_overrides[deps.get_color_strip_store] = lambda: css_store
app.dependency_overrides[deps.get_output_target_store] = lambda: output_target_store
app.dependency_overrides[deps.get_processor_manager] = lambda: mock_manager
# Capture entity events
def _fire(entity_type, action, entity_id):
@@ -512,3 +522,79 @@ class TestScaffoldCalibrationIntegration:
assert updated.calibration.leds_top == 15
assert updated.calibration.leds_right == 9
assert updated.calibration.layout == "clockwise"
# ---------------------------------------------------------------------------
# Regression: scaffold registers the output target with ProcessorManager
# ---------------------------------------------------------------------------
class TestScaffoldRegistersWithManager:
def test_scaffold_calls_add_target_on_manager(
self,
setup_client,
sample_device,
mock_manager,
):
"""Blocker regression: scaffold must register the created output target
with the ProcessorManager so that a subsequent start call can find it.
WledOutputTarget.register_with_manager calls manager.add_target(...)
— we assert that method was invoked with the scaffolded target id.
"""
resp = _scaffold(setup_client, device_id=sample_device.id)
assert resp.status_code == 201, resp.text
target_id = resp.json()["output_target_id"]
# register_with_manager → manager.add_target(target_id=..., ...)
mock_manager.add_target.assert_called_once()
call_kwargs = mock_manager.add_target.call_args
assert call_kwargs.kwargs.get("target_id") == target_id, (
f"manager.add_target was not called with target_id={target_id!r}; "
f"actual call: {call_kwargs}"
)
def test_scaffold_rollback_calls_remove_target_on_manager(
self,
setup_client,
sample_device,
output_target_store,
mock_manager,
):
"""Rollback after a post-target-creation failure must call manager.remove_target
so no half-registered target lingers in the ProcessorManager.
We inject a failure by making register_with_manager raise RuntimeError
(bypassing the ValueError-only guard), which puts the outer except branch
in play. The target IS already in created_ids at that point, so rollback
must call manager.remove_target(target_id).
"""
created_target_ids: list[str] = []
original_create = output_target_store.create_wled_target
def _spy_create(*args, **kwargs):
target = original_create(*args, **kwargs)
created_target_ids.append(target.id)
return target
output_target_store.create_wled_target = _spy_create
try:
# Patch register_with_manager on WledOutputTarget to raise RuntimeError —
# RuntimeError bypasses the ValueError guard and triggers the outer except,
# so rollback fires with the target already in created_ids.
with patch(
"ledgrab.storage.wled_output_target.WledOutputTarget.register_with_manager",
side_effect=RuntimeError("Injected registration failure for rollback test"),
):
resp = setup_client.post(
"/api/v1/setup/scaffold",
json={"device_id": sample_device.id, "display_index": 0},
)
assert resp.status_code == 500, resp.text
assert len(created_target_ids) == 1, "spy did not record a created target"
target_id = created_target_ids[0]
mock_manager.remove_target.assert_called_with(target_id)
finally:
output_target_store.create_wled_target = original_create
server/tests/api/routes/test_setup_routes_adversarial.py
+506
View File
@@ -0,0 +1,506 @@
"""Adversarial tests for the setup scaffold and onboarding preference endpoints.
Phase 2 acceptance criteria (NOT what the code happens to do):
- Rollback when the FINAL step (output target create) fails leaves ZERO orphans
AND emits ZERO "created" events.
- Reused capture template is NOT deleted on rollback.
- display_index > 63 → 422.
- Missing device_id → 422 (Pydantic validation before handler runs).
- Unknown device_id → 404.
- PUT onboarding false clears completed_at to null.
- Corrupt stored onboarding value falls back to default (onboarded=false).
These fill the gaps in the existing 22 happy-path tests.
"""
from __future__ import annotations
import pytest
from fastapi import FastAPI
from fastapi.testclient import TestClient
from unittest.mock import MagicMock, patch
# ---------------------------------------------------------------------------
# Shared fixtures (mirrors test_setup_routes.py exactly)
# ---------------------------------------------------------------------------
@pytest.fixture
def tmp_db(tmp_path):
from ledgrab.storage.database import Database
db = Database(tmp_path / "adv_setup.db")
yield db
db.close()
@pytest.fixture
def device_store(tmp_db):
from ledgrab.storage import DeviceStore
return DeviceStore(tmp_db)
@pytest.fixture
def template_store(tmp_db):
from ledgrab.storage.template_store import TemplateStore
return TemplateStore(tmp_db)
@pytest.fixture
def picture_source_store(tmp_db):
from ledgrab.storage.picture_source_store import PictureSourceStore
return PictureSourceStore(tmp_db)
@pytest.fixture
def css_store(tmp_db):
from ledgrab.storage.color_strip_store import ColorStripStore
return ColorStripStore(tmp_db)
@pytest.fixture
def output_target_store(tmp_db):
from ledgrab.storage.output_target_store import OutputTargetStore
return OutputTargetStore(tmp_db)
@pytest.fixture
def sample_device(device_store):
return device_store.create_device(
name="Test LED Strip",
url="http://192.168.1.10",
led_count=60,
)
@pytest.fixture
def event_log():
log = []
return log
@pytest.fixture
def mock_manager():
"""A MagicMock ProcessorManager that silently accepts all calls."""
mgr = MagicMock()
mgr.remove_target = MagicMock()
return mgr
@pytest.fixture
def setup_client(
tmp_db,
device_store,
template_store,
picture_source_store,
css_store,
output_target_store,
event_log,
mock_manager,
):
from ledgrab.api.routes.setup import router
from ledgrab.api.auth import verify_api_key
from ledgrab.api import dependencies as deps
app = FastAPI()
app.include_router(router)
app.dependency_overrides[verify_api_key] = lambda: "test"
app.dependency_overrides[deps.get_device_store] = lambda: device_store
app.dependency_overrides[deps.get_template_store] = lambda: template_store
app.dependency_overrides[deps.get_picture_source_store] = lambda: picture_source_store
app.dependency_overrides[deps.get_color_strip_store] = lambda: css_store
app.dependency_overrides[deps.get_output_target_store] = lambda: output_target_store
app.dependency_overrides[deps.get_processor_manager] = lambda: mock_manager
def _fire(entity_type, action, entity_id):
event_log.append((entity_type, action, entity_id))
with patch("ledgrab.api.routes.setup.fire_entity_event", side_effect=_fire):
yield TestClient(app, raise_server_exceptions=False)
def _scaffold(client, **overrides):
body = {"display_index": 0, **overrides}
return client.post("/api/v1/setup/scaffold", json=body)
# ---------------------------------------------------------------------------
# Rollback when the FINAL step (output target) fails
# Criteria: "zero orphans AND zero 'created' events"
# ---------------------------------------------------------------------------
class TestFinalStepRollback:
def test_final_step_failure_leaves_zero_orphans(
self,
setup_client,
sample_device,
picture_source_store,
css_store,
output_target_store,
):
"""output_target_store.create_wled_target failing leaves NO orphaned entities."""
original_create = output_target_store.create_wled_target
def _fail(*args, **kwargs):
raise ValueError("Injected final step failure")
output_target_store.create_wled_target = _fail
try:
resp = _scaffold(setup_client, device_id=sample_device.id)
assert resp.status_code in (
400,
500,
), f"Expected 4xx/5xx on final-step failure, got {resp.status_code}: {resp.text}"
# Zero picture sources remaining
remaining_ps = picture_source_store.get_all_streams()
assert len(remaining_ps) == 0, f"Picture sources not rolled back: {remaining_ps}"
# Zero color-strip sources remaining
remaining_css = css_store.get_all_sources()
assert len(remaining_css) == 0, f"Color-strip sources not rolled back: {remaining_css}"
# Zero output targets (trivially true since creation was never reached,
# but included for completeness)
remaining_ot = output_target_store.get_all_targets()
assert len(remaining_ot) == 0, f"Output targets not rolled back: {remaining_ot}"
finally:
output_target_store.create_wled_target = original_create
def test_final_step_failure_emits_zero_created_events(
self,
setup_client,
sample_device,
output_target_store,
event_log,
):
"""No 'created' events must be emitted when the final step fails (deferred-event contract)."""
original_create = output_target_store.create_wled_target
def _fail(*args, **kwargs):
raise ValueError("Final step injected failure")
output_target_store.create_wled_target = _fail
try:
resp = _scaffold(setup_client, device_id=sample_device.id)
assert resp.status_code in (400, 500)
created_events = [(et, act) for et, act, _ in event_log if act == "created"]
assert (
created_events == []
), f"'created' events leaked on final-step rollback: {created_events}"
finally:
output_target_store.create_wled_target = original_create
def test_final_step_failure_reused_template_survives(
self,
setup_client,
sample_device,
template_store,
output_target_store,
):
"""A reused capture template must NOT be deleted when the final step fails."""
templates_before = {t.id for t in template_store.get_all_templates()}
original_create = output_target_store.create_wled_target
def _fail(*args, **kwargs):
raise ValueError("Final step injected failure")
output_target_store.create_wled_target = _fail
try:
_scaffold(setup_client, device_id=sample_device.id)
finally:
output_target_store.create_wled_target = original_create
templates_after = {t.id for t in template_store.get_all_templates()}
assert templates_before == templates_after, (
f"Template set changed after rollback: "
f"before={templates_before} after={templates_after}"
)
def test_final_step_failure_device_not_deleted(
self,
setup_client,
sample_device,
device_store,
output_target_store,
):
"""The pre-existing device must never be touched by rollback of any step."""
original_create = output_target_store.create_wled_target
def _fail(*args, **kwargs):
raise ValueError("Final step injected failure")
output_target_store.create_wled_target = _fail
try:
_scaffold(setup_client, device_id=sample_device.id)
finally:
output_target_store.create_wled_target = original_create
device = device_store.get(sample_device.id)
assert device is not None, "Pre-existing device was deleted during rollback"
# ---------------------------------------------------------------------------
# Validation: display_index bounds
# Criteria: display_index > 63 → 422; display_index 63 → 201; negative → 422
# ---------------------------------------------------------------------------
class TestDisplayIndexBounds:
def test_display_index_64_returns_422(self, setup_client, sample_device):
"""display_index=64 (one above max) must be rejected with 422."""
resp = _scaffold(setup_client, device_id=sample_device.id, display_index=64)
assert (
resp.status_code == 422
), f"Expected 422 for display_index=64, got {resp.status_code}: {resp.text}"
def test_display_index_63_returns_201(self, setup_client, sample_device):
"""display_index=63 is the maximum valid value — must be accepted."""
resp = _scaffold(setup_client, device_id=sample_device.id, display_index=63)
assert (
resp.status_code == 201
), f"Expected 201 for display_index=63, got {resp.status_code}: {resp.text}"
def test_display_index_0_returns_201(self, setup_client, sample_device):
"""display_index=0 is the minimum valid value."""
resp = _scaffold(setup_client, device_id=sample_device.id, display_index=0)
assert (
resp.status_code == 201
), f"Expected 201 for display_index=0, got {resp.status_code}: {resp.text}"
def test_display_index_negative_1_returns_422(self, setup_client, sample_device):
"""display_index=-1 must be rejected with 422."""
resp = _scaffold(setup_client, device_id=sample_device.id, display_index=-1)
assert (
resp.status_code == 422
), f"Expected 422 for display_index=-1, got {resp.status_code}: {resp.text}"
def test_display_index_very_large_returns_422(self, setup_client, sample_device):
"""display_index=10000 must be rejected with 422."""
resp = _scaffold(setup_client, device_id=sample_device.id, display_index=10000)
assert (
resp.status_code == 422
), f"Expected 422 for display_index=10000, got {resp.status_code}: {resp.text}"
# ---------------------------------------------------------------------------
# Validation: missing / unknown device_id
# ---------------------------------------------------------------------------
class TestDeviceValidation:
def test_missing_device_id_returns_422(self, setup_client):
"""Omitting device_id entirely must yield 422 (Pydantic required field)."""
resp = setup_client.post("/api/v1/setup/scaffold", json={"display_index": 0})
assert (
resp.status_code == 422
), f"Expected 422 for missing device_id, got {resp.status_code}: {resp.text}"
def test_empty_string_device_id_handled(self, setup_client):
"""device_id='' — should yield 404 (empty string not in device store) or 422."""
resp = _scaffold(setup_client, device_id="")
assert resp.status_code in (
404,
422,
), f"Expected 404 or 422 for empty device_id, got {resp.status_code}: {resp.text}"
def test_unknown_device_id_returns_404(self, setup_client):
"""device_id that does not exist in the store must yield 404."""
resp = _scaffold(setup_client, device_id="device_definitely_does_not_exist")
assert (
resp.status_code == 404
), f"Expected 404 for unknown device_id, got {resp.status_code}: {resp.text}"
def test_none_device_id_returns_422(self, setup_client):
"""device_id=null must yield 422 (not None-convertible to str)."""
resp = setup_client.post(
"/api/v1/setup/scaffold", json={"device_id": None, "display_index": 0}
)
assert (
resp.status_code == 422
), f"Expected 422 for null device_id, got {resp.status_code}: {resp.text}"
# ---------------------------------------------------------------------------
# Rollback idempotency: calling scaffold twice with the final step failing
# must still leave exactly zero orphans (no accumulation across calls).
# ---------------------------------------------------------------------------
class TestRollbackIdempotency:
def test_two_failed_scaffolds_leave_zero_orphans(
self,
setup_client,
sample_device,
picture_source_store,
css_store,
output_target_store,
):
"""Two sequential scaffold failures must not accumulate orphans."""
original_create = output_target_store.create_wled_target
def _fail(*args, **kwargs):
raise ValueError("Always fails")
output_target_store.create_wled_target = _fail
try:
_scaffold(setup_client, device_id=sample_device.id)
_scaffold(setup_client, device_id=sample_device.id)
finally:
output_target_store.create_wled_target = original_create
assert (
len(picture_source_store.get_all_streams()) == 0
), "Picture sources accumulated across two failed scaffolds"
assert (
len(css_store.get_all_sources()) == 0
), "Color-strip sources accumulated across two failed scaffolds"
# ---------------------------------------------------------------------------
# Onboarding: adversarial cases
# ---------------------------------------------------------------------------
@pytest.fixture
def pref_client(tmp_db):
from ledgrab.api.routes.preferences import router
from ledgrab.api.auth import verify_api_key
from ledgrab.api import dependencies as deps
app = FastAPI()
app.include_router(router)
app.dependency_overrides[verify_api_key] = lambda: "test"
app.dependency_overrides[deps.get_database] = lambda: tmp_db
return TestClient(app)
class TestOnboardingAdversarial:
def test_put_false_clears_completed_at(self, pref_client):
"""PUT onboarded=false must clear completed_at to null, per criteria."""
# First mark as onboarded
r1 = pref_client.put("/api/v1/preferences/onboarding", json={"onboarded": True})
assert r1.status_code == 200
assert r1.json()["completed_at"] is not None
# Now set to false — completed_at must be cleared
r2 = pref_client.put("/api/v1/preferences/onboarding", json={"onboarded": False})
assert r2.status_code == 200
data = r2.json()
assert data["onboarded"] is False
assert (
data["completed_at"] is None
), f"completed_at should be null after PUT onboarded=false, got {data['completed_at']!r}"
def test_put_false_then_get_returns_null_completed_at(self, pref_client):
"""After PUT false, GET must also return null completed_at (persisted correctly)."""
pref_client.put("/api/v1/preferences/onboarding", json={"onboarded": True})
pref_client.put("/api/v1/preferences/onboarding", json={"onboarded": False})
resp = pref_client.get("/api/v1/preferences/onboarding")
assert resp.status_code == 200
assert (
resp.json()["completed_at"] is None
), "GET after PUT false should return null completed_at"
def test_corrupt_stored_value_falls_back_to_default(self, tmp_db, pref_client):
"""If the stored onboarding value is corrupt, GET must fall back to default.
Criteria: "corrupt stored value falls back to default".
We inject garbage into the db directly, then hit GET.
"""
# Inject a value that is syntactically valid JSON (dict) but fails
# Pydantic validation because the types are wrong.
tmp_db.set_setting("onboarded", {"onboarded": "not_a_bool", "completed_at": 12345})
resp = pref_client.get("/api/v1/preferences/onboarding")
assert (
resp.status_code == 200
), f"Expected 200 for corrupt onboarding value, got {resp.status_code}"
data = resp.json()
# Must fall back to default
assert (
data["onboarded"] is False
), f"Expected onboarded=false as default after corrupt value, got {data['onboarded']!r}"
assert (
data["completed_at"] is None
), f"Expected completed_at=null as default, got {data['completed_at']!r}"
def test_corrupt_stored_value_as_wrong_type_falls_back(self, tmp_db, pref_client):
"""Stored value is a string (not a dict) — must fall back to default."""
tmp_db.set_setting("onboarded", "this_is_not_valid")
resp = pref_client.get("/api/v1/preferences/onboarding")
assert resp.status_code == 200
data = resp.json()
assert data["onboarded"] is False
assert data["completed_at"] is None
def test_corrupt_stored_null_falls_back_to_default(self, tmp_db, pref_client):
"""Stored value is null/None — must return default (not crash)."""
tmp_db.set_setting("onboarded", None)
resp = pref_client.get("/api/v1/preferences/onboarding")
assert resp.status_code == 200
assert resp.json()["onboarded"] is False
def test_put_true_without_completed_at_stamps_timestamp(self, pref_client):
"""PUT onboarded=true without completed_at must auto-stamp a timestamp."""
resp = pref_client.put("/api/v1/preferences/onboarding", json={"onboarded": True})
assert resp.status_code == 200
data = resp.json()
assert (
data["completed_at"] is not None
), "Server must auto-stamp completed_at when onboarded=true is sent without a timestamp"
# Should be a non-empty ISO timestamp string
assert len(data["completed_at"]) > 10
def test_put_true_with_completed_at_preserves_it(self, pref_client):
"""PUT onboarded=true with explicit completed_at must preserve that value."""
ts = "2025-03-15T10:00:00+00:00"
resp = pref_client.put(
"/api/v1/preferences/onboarding",
json={"onboarded": True, "completed_at": ts},
)
assert resp.status_code == 200
assert resp.json()["completed_at"] == ts
def test_put_false_with_completed_at_clears_it(self, pref_client):
"""PUT onboarded=false even with a completed_at payload must clear it.
The criteria say: 'Setting onboarded=false clears completed_at to null.'
"""
ts = "2025-01-01T00:00:00+00:00"
resp = pref_client.put(
"/api/v1/preferences/onboarding",
json={"onboarded": False, "completed_at": ts},
)
assert resp.status_code == 200
data = resp.json()
assert data["onboarded"] is False
assert (
data["completed_at"] is None
), f"completed_at should be cleared to null when onboarded=false, got {data['completed_at']!r}"
def test_multiple_true_puts_only_stamp_once(self, pref_client):
"""Two successive PUT true calls — second call must preserve the original timestamp."""
r1 = pref_client.put("/api/v1/preferences/onboarding", json={"onboarded": True})
ts1 = r1.json()["completed_at"]
assert ts1 is not None
# Explicitly provide the original timestamp in second call
r2 = pref_client.put(
"/api/v1/preferences/onboarding",
json={"onboarded": True, "completed_at": ts1},
)
assert (
r2.json()["completed_at"] == ts1
), "Explicit completed_at should be preserved on second PUT"