refactor(#27): IMP-27 K5 catalog loader + _get_block_by_id cleanup
Consolidate three duplicated catalog readers and two _get_block_by_id implementations behind a single shared module (src/catalog.py) that owns file-read + mtime cache. All caller signatures and return contracts remain byte-identical. Units: - u1 NEW src/catalog.py (76 lines): load_root_catalog / load_blocks / get_block_by_id / get_catalog_mtime as the sole file-read + mtime-cache owner. - u2 src/block_reference.py: _load_catalog delegates to load_blocks (list[dict] preserved); _get_block_by_id (no-arg) delegates to catalog.get_block_by_id. Module-level _catalog_cache removed. - u3 src/block_selector.py: load_catalog delegates to load_root_catalog (root dict preserved); _get_block_by_id (catalog-injected sig preserved) delegates to catalog.get_block_by_id. Module-level _catalog_cache / _catalog_mtime / CATALOG_PATH removed. - u4 src/renderer.py: _load_catalog_map and _load_catalog_map_with_variants consume catalog.load_blocks; renderer projection caches kept local but keyed via catalog.get_catalog_mtime(). Per-projection invalidation keys (_CATALOG_MAP_MTIME / _CATALOG_VARIANT_MAP_MTIME) introduced. import yaml, CATALOG_PATH, legacy _CATALOG_MTIME removed. - tests NEW tests/test_catalog_shared_loader.py (421 lines, 23 cases): shared loader + 3 wrappers covering single file-read, contract preservation, signature preservation, shared cache, private state absence, mtime invalidation propagation to renderer projections. Verification: - pytest tests/test_catalog_shared_loader.py -v: 23/23 PASS in 0.13s. - pytest tests/ -q --ignore=tests/matching: 365/365 PASS in 38.10s. - src/fit_verifier.py, src/space_allocator.py, src/pipeline.py and templates/catalog.yaml unchanged (git diff empty). Out of scope: - catalog.yaml schema/path unchanged. - Catalog direct-read call sites in fit_verifier / space_allocator / pipeline left for a separate follow-up axis. - Phase Z 22-step runtime, frame_selection, light_edit/restructure flows untouched. Refs: IMP-27 (gitea #27), INSIGHT-MAP §5 K5, PHASE-Q-AUDIT §2.10
This commit is contained in:
421
tests/test_catalog_shared_loader.py
Normal file
421
tests/test_catalog_shared_loader.py
Normal file
@@ -0,0 +1,421 @@
|
||||
"""IMP-27: Shared catalog loader tests (u1).
|
||||
|
||||
Validates that ``src.catalog`` provides a single file-read + mtime cache and
|
||||
that its four public functions honor the documented contracts.
|
||||
|
||||
Note: ``templates/catalog.yaml`` was deleted in cc2f434 (legacy block library
|
||||
cleanup). The shared loader still preserves the loader contract for any
|
||||
remaining Phase Q call sites; tests use a fixture catalog via monkeypatch so
|
||||
they are independent of the deleted production file.
|
||||
|
||||
Delegation tests for block_reference / block_selector / renderer wrappers are
|
||||
added in u2 / u3 / u4.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
import yaml
|
||||
|
||||
|
||||
FIXTURE_CATALOG = {
|
||||
"blocks": [
|
||||
{
|
||||
"id": "fixture-block-a",
|
||||
"category": "emphasis",
|
||||
"template": "blocks/emphasis/fixture-block-a.html",
|
||||
},
|
||||
{
|
||||
"id": "fixture-block-b",
|
||||
"category": "cards",
|
||||
"template": "blocks/cards/fixture-block-b.html",
|
||||
"variants": [
|
||||
{"id": "default", "template": "blocks/cards/fixture-block-b.html"},
|
||||
{"id": "compact", "template": "blocks/cards/fixture-block-b--compact.html"},
|
||||
],
|
||||
},
|
||||
]
|
||||
}
|
||||
|
||||
|
||||
def _reset_catalog_module():
|
||||
import src.catalog as catalog_mod
|
||||
catalog_mod._catalog_cache = None
|
||||
catalog_mod._catalog_mtime = 0.0
|
||||
|
||||
|
||||
def _reset_renderer_projection_cache():
|
||||
"""IMP-27 u4: clear renderer-local projection caches between tests."""
|
||||
import src.renderer as renderer_mod
|
||||
renderer_mod._CATALOG_MAP = None
|
||||
renderer_mod._CATALOG_MAP_MTIME = 0.0
|
||||
renderer_mod._CATALOG_VARIANT_MAP = None
|
||||
renderer_mod._CATALOG_VARIANT_MAP_MTIME = 0.0
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def fixture_catalog_path(tmp_path, monkeypatch):
|
||||
"""Point src.catalog at a tmp catalog.yaml fixture and reset its cache."""
|
||||
import src.catalog as catalog_mod
|
||||
|
||||
fixture_path = tmp_path / "catalog.yaml"
|
||||
fixture_path.write_text(yaml.safe_dump(FIXTURE_CATALOG), encoding="utf-8")
|
||||
monkeypatch.setattr(catalog_mod, "CATALOG_PATH", fixture_path)
|
||||
_reset_catalog_module()
|
||||
_reset_renderer_projection_cache()
|
||||
return fixture_path
|
||||
|
||||
|
||||
def test_load_root_catalog_returns_root_dict(fixture_catalog_path):
|
||||
from src import catalog
|
||||
|
||||
root = catalog.load_root_catalog()
|
||||
assert isinstance(root, dict)
|
||||
assert "blocks" in root
|
||||
assert len(root["blocks"]) == 2
|
||||
|
||||
|
||||
def test_load_blocks_returns_list_of_block_dicts(fixture_catalog_path):
|
||||
from src import catalog
|
||||
|
||||
blocks = catalog.load_blocks()
|
||||
assert isinstance(blocks, list)
|
||||
assert len(blocks) == 2
|
||||
assert all(isinstance(b, dict) for b in blocks)
|
||||
assert all("id" in b for b in blocks)
|
||||
|
||||
|
||||
def test_get_block_by_id_without_catalog_arg(fixture_catalog_path):
|
||||
from src import catalog
|
||||
|
||||
found = catalog.get_block_by_id("fixture-block-a")
|
||||
assert found is not None
|
||||
assert found["id"] == "fixture-block-a"
|
||||
assert found["category"] == "emphasis"
|
||||
|
||||
|
||||
def test_get_block_by_id_with_catalog_arg_preserves_block_selector_contract(fixture_catalog_path):
|
||||
from src import catalog
|
||||
|
||||
root = catalog.load_root_catalog()
|
||||
found = catalog.get_block_by_id("fixture-block-b", catalog=root)
|
||||
assert found is not None
|
||||
assert found["id"] == "fixture-block-b"
|
||||
|
||||
|
||||
def test_get_block_by_id_unknown_returns_none(fixture_catalog_path):
|
||||
from src import catalog
|
||||
|
||||
assert catalog.get_block_by_id("__nonexistent_block_id__") is None
|
||||
|
||||
|
||||
def test_mtime_cache_does_single_file_read(fixture_catalog_path, monkeypatch):
|
||||
import src.catalog as catalog_mod
|
||||
|
||||
read_count = {"n": 0}
|
||||
real_safe_load = catalog_mod.yaml.safe_load
|
||||
|
||||
def counting_safe_load(stream):
|
||||
read_count["n"] += 1
|
||||
return real_safe_load(stream)
|
||||
|
||||
monkeypatch.setattr(catalog_mod.yaml, "safe_load", counting_safe_load)
|
||||
|
||||
catalog_mod.load_root_catalog()
|
||||
catalog_mod.load_root_catalog()
|
||||
catalog_mod.load_blocks()
|
||||
catalog_mod.get_block_by_id("fixture-block-a")
|
||||
|
||||
assert read_count["n"] == 1, (
|
||||
f"Expected exactly one yaml.safe_load call on cold cache, "
|
||||
f"got {read_count['n']}"
|
||||
)
|
||||
|
||||
|
||||
def test_mtime_change_triggers_reload(fixture_catalog_path, monkeypatch):
|
||||
import os
|
||||
import src.catalog as catalog_mod
|
||||
|
||||
read_count = {"n": 0}
|
||||
real_safe_load = catalog_mod.yaml.safe_load
|
||||
|
||||
def counting_safe_load(stream):
|
||||
read_count["n"] += 1
|
||||
return real_safe_load(stream)
|
||||
|
||||
monkeypatch.setattr(catalog_mod.yaml, "safe_load", counting_safe_load)
|
||||
|
||||
catalog_mod.load_root_catalog()
|
||||
assert read_count["n"] == 1
|
||||
|
||||
# Forcibly advance file mtime and verify cache invalidates.
|
||||
original_mtime = fixture_catalog_path.stat().st_mtime
|
||||
os.utime(fixture_catalog_path, (original_mtime + 10, original_mtime + 10))
|
||||
|
||||
catalog_mod.load_root_catalog()
|
||||
assert read_count["n"] == 2
|
||||
|
||||
|
||||
def test_get_catalog_mtime_matches_file_after_load(fixture_catalog_path):
|
||||
from src import catalog
|
||||
|
||||
catalog.load_root_catalog()
|
||||
actual = fixture_catalog_path.stat().st_mtime
|
||||
assert catalog.get_catalog_mtime() == actual
|
||||
|
||||
|
||||
def test_get_catalog_mtime_is_zero_before_first_load(monkeypatch, tmp_path):
|
||||
import src.catalog as catalog_mod
|
||||
monkeypatch.setattr(catalog_mod, "CATALOG_PATH", tmp_path / "unused.yaml")
|
||||
_reset_catalog_module()
|
||||
|
||||
assert catalog_mod.get_catalog_mtime() == 0.0
|
||||
|
||||
|
||||
def test_load_root_catalog_missing_file_returns_empty_blocks(monkeypatch, tmp_path):
|
||||
import src.catalog as catalog_mod
|
||||
|
||||
missing_path = tmp_path / "no_such_catalog.yaml"
|
||||
monkeypatch.setattr(catalog_mod, "CATALOG_PATH", missing_path)
|
||||
_reset_catalog_module()
|
||||
|
||||
root = catalog_mod.load_root_catalog()
|
||||
assert root == {"blocks": []}
|
||||
assert catalog_mod.load_blocks() == []
|
||||
|
||||
|
||||
# ──────────────────────────────────────────────────────────
|
||||
# u2: block_reference delegation tests
|
||||
# ──────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_block_reference_load_catalog_returns_list_of_blocks(fixture_catalog_path):
|
||||
"""block_reference._load_catalog preserves list[dict] contract via delegation."""
|
||||
from src import block_reference
|
||||
|
||||
blocks = block_reference._load_catalog()
|
||||
assert isinstance(blocks, list)
|
||||
assert len(blocks) == 2
|
||||
assert all(isinstance(b, dict) for b in blocks)
|
||||
assert {b["id"] for b in blocks} == {"fixture-block-a", "fixture-block-b"}
|
||||
|
||||
|
||||
def test_block_reference_get_block_by_id_no_arg_signature(fixture_catalog_path):
|
||||
"""block_reference._get_block_by_id preserves no-catalog-argument contract."""
|
||||
from src import block_reference
|
||||
|
||||
found = block_reference._get_block_by_id("fixture-block-a")
|
||||
assert found is not None
|
||||
assert found["id"] == "fixture-block-a"
|
||||
assert found["category"] == "emphasis"
|
||||
|
||||
assert block_reference._get_block_by_id("__nonexistent__") is None
|
||||
|
||||
|
||||
def test_block_reference_shares_cache_with_shared_loader(fixture_catalog_path, monkeypatch):
|
||||
"""block_reference wrappers must hit the shared mtime cache, not a private copy."""
|
||||
import src.catalog as catalog_mod
|
||||
from src import block_reference
|
||||
|
||||
read_count = {"n": 0}
|
||||
real_safe_load = catalog_mod.yaml.safe_load
|
||||
|
||||
def counting_safe_load(stream):
|
||||
read_count["n"] += 1
|
||||
return real_safe_load(stream)
|
||||
|
||||
monkeypatch.setattr(catalog_mod.yaml, "safe_load", counting_safe_load)
|
||||
|
||||
block_reference._load_catalog()
|
||||
block_reference._get_block_by_id("fixture-block-a")
|
||||
catalog_mod.load_root_catalog()
|
||||
|
||||
assert read_count["n"] == 1, (
|
||||
f"block_reference wrappers should share the catalog cache; "
|
||||
f"got {read_count['n']} reads"
|
||||
)
|
||||
|
||||
|
||||
def test_block_reference_has_no_private_catalog_cache(fixture_catalog_path):
|
||||
"""IMP-27 u2 guard: block_reference must not retain a module-level _catalog_cache."""
|
||||
from src import block_reference
|
||||
|
||||
assert not hasattr(block_reference, "_catalog_cache"), (
|
||||
"block_reference._catalog_cache must be removed by u2 — "
|
||||
"loader is delegated to src.catalog"
|
||||
)
|
||||
|
||||
|
||||
# ──────────────────────────────────────────────────────────
|
||||
# u3: block_selector delegation tests
|
||||
# ──────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_block_selector_load_catalog_returns_root_dict(fixture_catalog_path):
|
||||
"""block_selector.load_catalog preserves root-dict contract via delegation."""
|
||||
from src import block_selector
|
||||
|
||||
root = block_selector.load_catalog()
|
||||
assert isinstance(root, dict)
|
||||
assert "blocks" in root
|
||||
assert isinstance(root["blocks"], list)
|
||||
assert {b["id"] for b in root["blocks"]} == {"fixture-block-a", "fixture-block-b"}
|
||||
|
||||
|
||||
def test_block_selector_get_block_by_id_catalog_injected_signature(fixture_catalog_path):
|
||||
"""block_selector._get_block_by_id preserves catalog-injected signature."""
|
||||
from src import block_selector
|
||||
|
||||
catalog_dict = block_selector.load_catalog()
|
||||
found = block_selector._get_block_by_id("fixture-block-b", catalog_dict)
|
||||
assert found is not None
|
||||
assert found["id"] == "fixture-block-b"
|
||||
assert found["category"] == "cards"
|
||||
|
||||
assert block_selector._get_block_by_id("__nonexistent__", catalog_dict) is None
|
||||
|
||||
|
||||
def test_block_selector_shares_cache_with_shared_loader(fixture_catalog_path, monkeypatch):
|
||||
"""block_selector wrappers must hit the shared mtime cache, not a private copy."""
|
||||
import src.catalog as catalog_mod
|
||||
from src import block_selector
|
||||
|
||||
read_count = {"n": 0}
|
||||
real_safe_load = catalog_mod.yaml.safe_load
|
||||
|
||||
def counting_safe_load(stream):
|
||||
read_count["n"] += 1
|
||||
return real_safe_load(stream)
|
||||
|
||||
monkeypatch.setattr(catalog_mod.yaml, "safe_load", counting_safe_load)
|
||||
|
||||
block_selector.load_catalog()
|
||||
block_selector._get_block_by_id("fixture-block-a", block_selector.load_catalog())
|
||||
catalog_mod.load_root_catalog()
|
||||
|
||||
assert read_count["n"] == 1, (
|
||||
f"block_selector wrappers should share the catalog cache; "
|
||||
f"got {read_count['n']} reads"
|
||||
)
|
||||
|
||||
|
||||
def test_block_selector_has_no_private_catalog_cache(fixture_catalog_path):
|
||||
"""IMP-27 u3 guard: block_selector must not retain module-level cache/mtime/CATALOG_PATH."""
|
||||
from src import block_selector
|
||||
|
||||
assert not hasattr(block_selector, "_catalog_cache"), (
|
||||
"block_selector._catalog_cache must be removed by u3 — "
|
||||
"loader is delegated to src.catalog"
|
||||
)
|
||||
assert not hasattr(block_selector, "_catalog_mtime"), (
|
||||
"block_selector._catalog_mtime must be removed by u3"
|
||||
)
|
||||
assert not hasattr(block_selector, "CATALOG_PATH"), (
|
||||
"block_selector.CATALOG_PATH must be removed by u3 — "
|
||||
"path lives in src.catalog only"
|
||||
)
|
||||
|
||||
|
||||
# ──────────────────────────────────────────────────────────
|
||||
# u4: renderer delegation tests
|
||||
# ──────────────────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_renderer_load_catalog_map_returns_id_to_template_dict(fixture_catalog_path):
|
||||
"""renderer._load_catalog_map preserves id → template projection contract."""
|
||||
from src import renderer
|
||||
|
||||
mapping = renderer._load_catalog_map()
|
||||
assert isinstance(mapping, dict)
|
||||
assert mapping["fixture-block-a"] == "blocks/emphasis/fixture-block-a.html"
|
||||
assert mapping["fixture-block-b"] == "blocks/cards/fixture-block-b.html"
|
||||
|
||||
|
||||
def test_renderer_load_catalog_map_with_variants_returns_compound_key_dict(fixture_catalog_path):
|
||||
"""renderer._load_catalog_map_with_variants preserves 'id--variant' → template projection."""
|
||||
from src import renderer
|
||||
|
||||
vmap = renderer._load_catalog_map_with_variants()
|
||||
assert isinstance(vmap, dict)
|
||||
# 'default' variants must be excluded (preserves pre-IMP-27 behavior).
|
||||
assert "fixture-block-b--default" not in vmap
|
||||
# Non-default variants must be present with the compound key.
|
||||
assert vmap["fixture-block-b--compact"] == "blocks/cards/fixture-block-b--compact.html"
|
||||
|
||||
|
||||
def test_renderer_shares_cache_with_shared_loader(fixture_catalog_path, monkeypatch):
|
||||
"""renderer projections must read through src.catalog, never opening the file directly."""
|
||||
import src.catalog as catalog_mod
|
||||
from src import renderer
|
||||
|
||||
read_count = {"n": 0}
|
||||
real_safe_load = catalog_mod.yaml.safe_load
|
||||
|
||||
def counting_safe_load(stream):
|
||||
read_count["n"] += 1
|
||||
return real_safe_load(stream)
|
||||
|
||||
monkeypatch.setattr(catalog_mod.yaml, "safe_load", counting_safe_load)
|
||||
|
||||
renderer._load_catalog_map()
|
||||
renderer._load_catalog_map_with_variants()
|
||||
catalog_mod.load_root_catalog()
|
||||
|
||||
assert read_count["n"] == 1, (
|
||||
f"renderer projections should share the catalog cache; "
|
||||
f"got {read_count['n']} reads"
|
||||
)
|
||||
|
||||
|
||||
def test_renderer_projection_invalidates_when_shared_mtime_changes(
|
||||
fixture_catalog_path, monkeypatch
|
||||
):
|
||||
"""IMP-27 u4 contract: renderer projection cache keyed off src.catalog.get_catalog_mtime."""
|
||||
import os
|
||||
|
||||
import src.catalog as catalog_mod
|
||||
from src import renderer
|
||||
|
||||
first_map = renderer._load_catalog_map()
|
||||
first_id_to_path = dict(first_map)
|
||||
|
||||
# Rewrite the fixture file with a different block id and bump mtime so the
|
||||
# shared cache (and therefore the renderer projection) must rebuild.
|
||||
new_catalog = {
|
||||
"blocks": [
|
||||
{
|
||||
"id": "fixture-block-c",
|
||||
"category": "headers",
|
||||
"template": "blocks/headers/fixture-block-c.html",
|
||||
},
|
||||
]
|
||||
}
|
||||
fixture_catalog_path.write_text(yaml.safe_dump(new_catalog), encoding="utf-8")
|
||||
original_mtime = fixture_catalog_path.stat().st_mtime
|
||||
os.utime(fixture_catalog_path, (original_mtime + 10, original_mtime + 10))
|
||||
# Force src.catalog to drop its in-memory cache so the next call re-reads.
|
||||
catalog_mod._catalog_cache = None
|
||||
catalog_mod._catalog_mtime = 0.0
|
||||
|
||||
second_map = renderer._load_catalog_map()
|
||||
assert "fixture-block-c" in second_map
|
||||
assert "fixture-block-a" not in second_map
|
||||
assert first_id_to_path != second_map
|
||||
|
||||
|
||||
def test_renderer_has_no_private_catalog_path_or_yaml(fixture_catalog_path):
|
||||
"""IMP-27 u4 guard: renderer must not retain its own file-read state."""
|
||||
from src import renderer
|
||||
|
||||
assert not hasattr(renderer, "CATALOG_PATH"), (
|
||||
"renderer.CATALOG_PATH must be removed by u4 — path lives in src.catalog only"
|
||||
)
|
||||
assert not hasattr(renderer, "yaml"), (
|
||||
"renderer.yaml import must be removed by u4 — file-read is delegated to src.catalog"
|
||||
)
|
||||
assert not hasattr(renderer, "_CATALOG_MTIME"), (
|
||||
"renderer._CATALOG_MTIME (legacy single mtime) must be removed by u4 — "
|
||||
"projection caches now key off src.catalog.get_catalog_mtime() via "
|
||||
"_CATALOG_MAP_MTIME / _CATALOG_VARIANT_MAP_MTIME"
|
||||
)
|
||||
Reference in New Issue
Block a user