From 22bb6d7a1aa3ecc77cc0d96295bffe8b2391eaea Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 21 Sep 2026 22:03:05 -0400 Subject: [PATCH] fix(tests): move #4196's usage guards to the seam they now describe (#4230) CI 7223 went red on seven tests in test_lesson_usage_surface.py. None of them caught a regression. Every property they pin still holds; they pinned it at the old LOCATION: they patched `usage_for_notes` on the door modules and read the route source for `usage_for_notes(` and `empty_usage()`. 62f3a48 moved all seven doors onto `note_usage.attach_usage`, so the name is gone from the doors. This is rule 167's case: a guard that fails on code you believe is correct has usually moved out from under its property. So the guards are relocated, not deleted. Each one still has an assertion that can fail: - MCP list / once-per-page / read-before-pull: patch `usage_for_notes` on `note_usage`, the module every door now reads it through. The ordering test still fails if a pull is recorded before the count is read. - REST list: calls `attach_usage(items)` exactly once, and does NOT hand-roll `it["usage"] =`, which is how a row could lose its zero-filled key again. The seam's own zero-fill and single aggregate are asserted in test_usage_attach_seam.py. - REST detail: `attach_usage(` still precedes `record_pulled(`. - Detail view: the advice moved into utils/deadWeight.ts, so assert BOTH that the view reads DEAD_WEIGHT_ADVICE.lesson AND that the table's lesson entry still points at `when_to_apply`, so the guard cannot pass by pointing at an entry that has stopped saying the right thing. Missed before pushing because I grepped src/ for callers of the old name and not tests/ for patchers of it. A test that patches a name off a module is a caller too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- tests/test_lesson_usage_surface.py | 41 +++++++++++++++++++----------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/tests/test_lesson_usage_surface.py b/tests/test_lesson_usage_surface.py index 2789442..0b4c3a9 100644 --- a/tests/test_lesson_usage_surface.py +++ b/tests/test_lesson_usage_surface.py @@ -25,6 +25,7 @@ from unittest.mock import AsyncMock, MagicMock, patch import pytest from scribe.mcp.tools import lessons as lesson_tools +from scribe.services import note_usage from scribe.services.note_usage import empty_usage @@ -47,7 +48,7 @@ async def test_the_mcp_listing_carries_usage_for_every_row(): new=AsyncMock(return_value=(_rows(), 2))), patch.object(lesson_tools.access_svc, "label_shared_items", new=AsyncMock(side_effect=lambda _uid, items: items)), - patch.object(lesson_tools, "usage_for_notes", + patch.object(note_usage, "usage_for_notes", new=AsyncMock(return_value=used)), ): out = await lesson_tools.list_lessons() @@ -71,7 +72,7 @@ async def test_the_listing_asks_for_usage_once_for_the_whole_page(): new=AsyncMock(return_value=(_rows(), 2))), patch.object(lesson_tools.access_svc, "label_shared_items", new=AsyncMock(side_effect=lambda _uid, items: items)), - patch.object(lesson_tools, "usage_for_notes", new=reader), + patch.object(note_usage, "usage_for_notes", new=reader), ): await lesson_tools.list_lessons() @@ -101,7 +102,7 @@ async def test_get_lesson_reads_the_count_before_recording_its_own_pull(): patch.object(lesson_tools, "_to_dict", return_value={"id": 7}), patch.object(lesson_tools.access_svc, "describe_provenance", new=AsyncMock(return_value={})), - patch.object(lesson_tools, "usage_for_notes", new=_usage_read), + patch.object(note_usage, "usage_for_notes", new=_usage_read), patch.object(lesson_tools, "record_pulled", side_effect=lambda **_kw: order.append("pull")), ): @@ -116,6 +117,13 @@ async def test_get_lesson_reads_the_count_before_recording_its_own_pull(): # ── the REST door, on structure (rule 167) ─────────────────────────────────── # +# MOVED 2026-09-21 (#4230), not weakened. These used to look for +# `usage_for_notes(` and `empty_usage()` in the route body. Both now live in +# ONE seam, `note_usage.attach_usage`, which seven doors share — so the +# zero-fill and the single aggregate are pinned once, against the seam, in +# tests/test_usage_attach_seam.py. What stays HERE is what only this route can +# get wrong: that it calls the seam at all, once, and before it records a pull. +# # Its siblings in test_lesson_rest_door.py are source guards for the same # reason: the route is decorated and returns a Quart response, so driving it # means standing up the app. What matters here is reachable from the source @@ -136,19 +144,17 @@ def test_the_rest_listing_reads_usage_once_for_the_page(): """A per-row lookup would be N+1 by construction — the listing's own comment says so, and this is what makes that comment checkable.""" src = _route_source("list_lessons_route") - assert src.count("usage_for_notes(") == 1 - # The one call is not inside the loop that assigns the rows. - call = src.index("usage_for_notes(") - assign = src.index('["usage"]') - assert call < assign + assert src.count("attach_usage(") == 1 + assert "attach_usage(items)" in src, "the seam must get the page, not a row" def test_every_rest_row_carries_the_key_even_at_zero(): """"Never surfaced" is a state the UI renders; a missing field is not.""" src = _route_source("list_lessons_route") - assert "empty_usage()" in src, ( - "a row with no recorded usage would come back without the key, and a " - "reader cannot tell that from a reporting failure" + assert "attach_usage(" in src + assert 'it["usage"] =' not in src, ( + "the route re-spells the attach by hand, so a row with no recorded " + "usage can come back without the key again" ) @@ -156,7 +162,7 @@ def test_the_rest_detail_door_also_reads_before_it_records(): """Two doors that disagree about what the number counts are worse than one door that is wrong, because only one of them looks wrong.""" src = _route_source("get_lesson_route") - assert src.index("usage_for_notes(") < src.index("record_pulled(") + assert src.index("attach_usage(") < src.index("record_pulled(") def test_these_guards_can_fail(): @@ -181,9 +187,14 @@ def test_the_detail_view_renders_the_badge_rather_than_respelling_it(): assert "UsageBadge" in view assert "usage-tag" not in view, "re-spelled the chip instead of reusing it" - key = 'dead-weight-advice="' - start = view.index(key) + len(key) - advice = view[start:view.index('"', start)] + # The sentence moved out of this template into the shared table (#4230, + # recorded on #3460). Assert BOTH halves, so the guard cannot pass by the + # view pointing at an entry that no longer says the right thing. + assert "DEAD_WEIGHT_ADVICE.lesson" in view, "the view no longer reads the lesson advice" + table = (Path(__file__).resolve().parents[1] + / "frontend/src/utils/deadWeight.ts").read_text() + start = table.index(" lesson:") + advice = table[start:table.index(",\n", start)] assert "when_to_apply" in advice, ( f"the dead-weight advice does not point at the trigger: {advice!r}" )