refactor(tests): one definition each for the copied fixtures and fakes (#2825, milestone 296 area 1)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 24s
CI & Build / TypeScript typecheck (push) Successful in 32s
CI & Build / Python tests (push) Successful in 54s
CI & Build / Build & push image (push) Successful in 18s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / integration (push) Successful in 24s
CI & Build / TypeScript typecheck (push) Successful in 32s
CI & Build / Python tests (push) Successful in 54s
CI & Build / Build & push image (push) Successful in 18s
The shape ledger showed the same test scaffolding defined over and over: _bind_user x12 (byte-identical), _dispose_engine x10 in three wordings, _no_supersession x3, _make_mock_session x7 in three subsets, a get-or-create User helper x2 (+3 inlined), and fifteen hand-rolled MagicMock note factories each re-explaining the same "an auto-MagicMock attribute is truthy" hazard (note 2109). Now: conftest.py carries _bind_user / _dispose_engine / _no_supersession as opt-in fixtures (pytestmark = usefixtures(...) per module, so unit tests pay nothing), and tests/helpers.py carries make_mock_session(), ensure_user() and fake_note(**attrs) — the hazard documented once, real values on every attribute the product reads. Call sites were rewritten by AST so titles with dashes and commas survived; the three SimpleNamespace _note stand-ins that only feed a single function stay local. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,20 +1,16 @@
|
||||
"""Tests for fable_*_note tools."""
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from scribe.mcp._context import _user_id_ctx
|
||||
from scribe.mcp.tools.notes import (
|
||||
list_notes, get_note, create_note,
|
||||
update_note, delete_note,
|
||||
)
|
||||
from tests.helpers import fake_note
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _bind_user():
|
||||
token = _user_id_ctx.set(7)
|
||||
yield
|
||||
_user_id_ctx.reset(token)
|
||||
pytestmark = pytest.mark.usefixtures("_bind_user")
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
@@ -32,21 +28,6 @@ def _no_supersession():
|
||||
yield
|
||||
|
||||
|
||||
def _fake_note(*, user_id: int = 7, **overrides) -> MagicMock:
|
||||
note = MagicMock()
|
||||
base = {"id": 1, "title": "t", "body": "b", "tags": [], "is_task": False}
|
||||
base.update(overrides)
|
||||
note.to_dict.return_value = base
|
||||
# Real values, not auto-attributes: get_note reads deleted_at, and compares
|
||||
# user_id against the bound caller to decide whether to attach a shared/owner
|
||||
# marker. A MagicMock is truthy on both, so it would read as another user's
|
||||
# trashed note and reach for the DB (note 2109).
|
||||
note.id = base["id"]
|
||||
note.user_id = user_id
|
||||
note.deleted_at = None
|
||||
return note
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_note_blocked_by_duplicate_gate():
|
||||
from scribe.services.dedup import DuplicateMatch
|
||||
@@ -67,7 +48,7 @@ async def test_create_note_force_bypasses_duplicate_gate():
|
||||
find_mock = AsyncMock()
|
||||
with patch("scribe.mcp.tools.notes.dedup_svc.find_duplicate_note", find_mock), \
|
||||
patch("scribe.mcp.tools.notes.notes_svc.create_note",
|
||||
AsyncMock(return_value=_fake_note(id=3))):
|
||||
AsyncMock(return_value=fake_note(id=3))):
|
||||
out = await create_note(title="dup", force=True)
|
||||
assert out["id"] == 3
|
||||
find_mock.assert_not_called()
|
||||
@@ -75,7 +56,7 @@ async def test_create_note_force_bypasses_duplicate_gate():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_list_notes_repackages_tuple_into_dict():
|
||||
rows = [_fake_note(id=1), _fake_note(id=2)]
|
||||
rows = [fake_note(id=1), fake_note(id=2)]
|
||||
with patch(
|
||||
"scribe.mcp.tools.notes.notes_svc.list_notes",
|
||||
AsyncMock(return_value=(rows, 2)),
|
||||
@@ -128,7 +109,7 @@ async def test_list_notes_limit_clamped():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_get_note_returns_dict():
|
||||
fake = _fake_note(id=5, title="found")
|
||||
fake = fake_note(id=5, title="found")
|
||||
with patch(
|
||||
"scribe.mcp.tools.notes.notes_svc.get_note_for_user",
|
||||
AsyncMock(return_value=(fake, "owner")),
|
||||
@@ -153,7 +134,7 @@ async def test_get_note_warns_in_words_when_a_later_note_overtook_it():
|
||||
field to notice would be worse than not surfacing it, because the reader
|
||||
acts on it confidently either way.
|
||||
"""
|
||||
fake = _fake_note(id=5, title="June's answer")
|
||||
fake = fake_note(id=5, title="June's answer")
|
||||
with patch("scribe.mcp.tools.notes.notes_svc.get_note_for_user",
|
||||
AsyncMock(return_value=(fake, "owner"))), \
|
||||
patch("scribe.mcp.tools.notes.supersession_svc.get_relations",
|
||||
@@ -170,7 +151,7 @@ async def test_get_note_opens_a_shared_record_and_says_whose_it_is():
|
||||
that menu can list a collaborator's note reached through a shared project.
|
||||
An owner-only fetch would answer "not found" for a record the agent was just
|
||||
handed — and the web UI opens the same note fine."""
|
||||
theirs = _fake_note(id=5, title="Their note", user_id=9)
|
||||
theirs = fake_note(id=5, title="Their note", user_id=9)
|
||||
with patch(
|
||||
"scribe.mcp.tools.notes.notes_svc.get_note_for_user",
|
||||
AsyncMock(return_value=(theirs, "viewer")),
|
||||
@@ -199,7 +180,7 @@ async def test_get_note_raises_when_not_found():
|
||||
async def test_get_note_treats_a_trashed_note_as_missing():
|
||||
"""get_note_for_user resolves permission, not liveness — the trash filter is
|
||||
the caller's to apply."""
|
||||
trashed = _fake_note(id=5)
|
||||
trashed = fake_note(id=5)
|
||||
trashed.deleted_at = "2026-07-01T00:00:00Z"
|
||||
with patch(
|
||||
"scribe.mcp.tools.notes.notes_svc.get_note_for_user",
|
||||
@@ -211,7 +192,7 @@ async def test_get_note_treats_a_trashed_note_as_missing():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_note_passes_through():
|
||||
fake = _fake_note(id=10, title="new")
|
||||
fake = fake_note(id=10, title="new")
|
||||
mock = AsyncMock(return_value=fake)
|
||||
with patch("scribe.mcp.tools.notes.notes_svc.create_note", mock):
|
||||
out = await create_note(title="new", body="x", tags=["a"], project_id=5)
|
||||
@@ -223,7 +204,7 @@ async def test_create_note_passes_through():
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_note_project_zero_becomes_none():
|
||||
"""project_id=0 sentinel must become None at the service layer (orphan note)."""
|
||||
fake = _fake_note()
|
||||
fake = fake_note()
|
||||
mock = AsyncMock(return_value=fake)
|
||||
with patch("scribe.mcp.tools.notes.notes_svc.create_note", mock):
|
||||
await create_note(title="t", project_id=0)
|
||||
@@ -234,7 +215,7 @@ async def test_create_note_project_zero_becomes_none():
|
||||
async def test_update_note_only_sends_non_default_fields():
|
||||
"""Omitted (default) fields must NOT reach the service — otherwise they'd
|
||||
overwrite real data with empty strings."""
|
||||
fake = _fake_note()
|
||||
fake = fake_note()
|
||||
mock = AsyncMock(return_value=fake)
|
||||
with patch("scribe.mcp.tools.notes.notes_svc.update_note", mock):
|
||||
await update_note(note_id=1, title="new title")
|
||||
@@ -247,7 +228,7 @@ async def test_update_note_only_sends_non_default_fields():
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_note_empty_tags_clears_explicitly():
|
||||
"""tags=[] is an explicit clear, distinct from tags=None (omit)."""
|
||||
fake = _fake_note()
|
||||
fake = fake_note()
|
||||
mock = AsyncMock(return_value=fake)
|
||||
with patch("scribe.mcp.tools.notes.notes_svc.update_note", mock):
|
||||
await update_note(note_id=1, tags=[])
|
||||
@@ -256,7 +237,7 @@ async def test_update_note_empty_tags_clears_explicitly():
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_note_tags_none_means_omit():
|
||||
fake = _fake_note()
|
||||
fake = fake_note()
|
||||
mock = AsyncMock(return_value=fake)
|
||||
with patch("scribe.mcp.tools.notes.notes_svc.update_note", mock):
|
||||
await update_note(note_id=1, tags=None)
|
||||
|
||||
Reference in New Issue
Block a user