test: close mirror-fixture gap exposed by post-fix audit
The eeschema-ground-truth fixture (_build_mirror_case) passed 'mirror': 'x' to ComponentManager.add_component, which silently drops the kwarg — so the resulting .kicad_sch had no (mirror x|y) token, eeschema rendered an unmirrored symbol, and our pin coords (also unmirrored) tautologically matched. The mirror tests were GREEN both before and after the rotation/mirror fix in7e67cb9, providing zero regression coverage for the mirror semantics. Fix: - _build_mirror_case now applies the mirror via the same low-level helper (WireDragger.update_symbol_rotation_mirror) that rotate_schematic_component uses, with a guard assertion that the written file actually contains (mirror x|y). - Two new pin-down unit tests in test_add_schematic_component.py document and lock down ComponentManager.add_component's silent-drop behavior for mirror, so the next person to touch that path knows to update the eeschema-truth fixture if they grow real mirror support. Verified: with the production fix at7e67cb9reverted, the kicad-cli mirror tests now go RED (previously they stayed GREEN regardless). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -19,9 +19,7 @@ EMPTY_SCH = TEMPLATES_DIR / "empty.kicad_sch"
|
||||
|
||||
|
||||
def _write_temp_sch(content: str) -> Path:
|
||||
tmp = tempfile.NamedTemporaryFile(
|
||||
suffix=".kicad_sch", delete=False, mode="w", encoding="utf-8"
|
||||
)
|
||||
tmp = tempfile.NamedTemporaryFile(suffix=".kicad_sch", delete=False, mode="w", encoding="utf-8")
|
||||
tmp.write(content)
|
||||
tmp.close()
|
||||
return Path(tmp.name)
|
||||
@@ -36,7 +34,10 @@ def _unit_values_in_file(path: Path) -> list[int]:
|
||||
"""Return all (unit N) values written for symbol instances in the schematic."""
|
||||
content = path.read_text()
|
||||
# Match top-level symbol instances: (symbol (lib_id ...) (at ...) (unit N) ...)
|
||||
return [int(n) for n in re.findall(r"\(symbol \(lib_id [^)]+\) \(at [^)]+\) \(unit (\d+)\)", content)]
|
||||
return [
|
||||
int(n)
|
||||
for n in re.findall(r"\(symbol \(lib_id [^)]+\) \(at [^)]+\) \(unit (\d+)\)", content)
|
||||
]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -155,18 +156,20 @@ class TestHandlerAddSchematicComponent:
|
||||
def test_unit_defaults_to_1_in_handler(self, tmp_path: Any) -> None:
|
||||
sch = tmp_path / "test.kicad_sch"
|
||||
shutil.copy(EMPTY_SCH, sch)
|
||||
result = self._call_handler({
|
||||
"schematicPath": str(sch),
|
||||
"component": {
|
||||
"library": "Device",
|
||||
"type": "R",
|
||||
"reference": "R99",
|
||||
"value": "1k",
|
||||
"x": 10,
|
||||
"y": 10,
|
||||
# no "unit" key — should default to 1
|
||||
},
|
||||
})
|
||||
result = self._call_handler(
|
||||
{
|
||||
"schematicPath": str(sch),
|
||||
"component": {
|
||||
"library": "Device",
|
||||
"type": "R",
|
||||
"reference": "R99",
|
||||
"value": "1k",
|
||||
"x": 10,
|
||||
"y": 10,
|
||||
# no "unit" key — should default to 1
|
||||
},
|
||||
}
|
||||
)
|
||||
assert result["success"] is True
|
||||
units = _unit_values_in_file(sch)
|
||||
assert 1 in units
|
||||
@@ -174,18 +177,82 @@ class TestHandlerAddSchematicComponent:
|
||||
def test_unit_2_passed_through_handler(self, tmp_path: Any) -> None:
|
||||
sch = tmp_path / "test.kicad_sch"
|
||||
shutil.copy(EMPTY_SCH, sch)
|
||||
result = self._call_handler({
|
||||
"schematicPath": str(sch),
|
||||
"component": {
|
||||
"library": "Device",
|
||||
"type": "R",
|
||||
"reference": "U10",
|
||||
"value": "TLP291-4",
|
||||
"x": 25,
|
||||
"y": 35,
|
||||
"unit": 2,
|
||||
},
|
||||
})
|
||||
result = self._call_handler(
|
||||
{
|
||||
"schematicPath": str(sch),
|
||||
"component": {
|
||||
"library": "Device",
|
||||
"type": "R",
|
||||
"reference": "U10",
|
||||
"value": "TLP291-4",
|
||||
"x": 25,
|
||||
"y": 35,
|
||||
"unit": 2,
|
||||
},
|
||||
}
|
||||
)
|
||||
assert result["success"] is True
|
||||
units = _unit_values_in_file(sch)
|
||||
assert 2 in units
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Mirror parameter — known gap
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
class TestAddComponentMirrorParam:
|
||||
"""ComponentManager.add_component does NOT honor a 'mirror' kwarg today.
|
||||
|
||||
The MCP add_schematic_component tool schema also doesn't expose mirror.
|
||||
A mirror is currently only applicable post-add via rotate_schematic_component.
|
||||
|
||||
These tests pin down the silent-drop behavior so a fixture that passes
|
||||
'mirror': 'x' and then asserts something against the resulting schematic
|
||||
cannot accidentally pass for the wrong reason (the symbol ends up
|
||||
unmirrored). If/when add_component grows real mirror support, update both
|
||||
tests together — the second test then becomes the positive assertion."""
|
||||
|
||||
def setup_method(self) -> None:
|
||||
from commands.component_schematic import ComponentManager
|
||||
from commands.schematic import SchematicManager
|
||||
|
||||
self.ComponentManager = ComponentManager
|
||||
self.SchematicManager = SchematicManager
|
||||
|
||||
def _add(self, sch_path: Path, mirror_value: Any) -> None:
|
||||
sch = self.SchematicManager.load_schematic(str(sch_path))
|
||||
params = {
|
||||
"type": "R",
|
||||
"reference": "R1",
|
||||
"value": "10k",
|
||||
"x": 100.0,
|
||||
"y": 100.0,
|
||||
"rotation": 0,
|
||||
}
|
||||
if mirror_value is not None:
|
||||
params["mirror"] = mirror_value
|
||||
self.ComponentManager.add_component(sch, params, sch_path)
|
||||
self.SchematicManager.save_schematic(sch, str(sch_path))
|
||||
|
||||
def test_mirror_x_arg_is_silently_dropped(self, tmp_path: Any) -> None:
|
||||
sch = tmp_path / "mirror_x.kicad_sch"
|
||||
shutil.copy(EMPTY_SCH, sch)
|
||||
self._add(sch, "x")
|
||||
text = sch.read_text()
|
||||
assert "(mirror x)" not in text, (
|
||||
"ComponentManager.add_component now appears to honor mirror='x'. "
|
||||
"Update _build_mirror_case in test_pin_world_xy_eeschema_truth.py "
|
||||
"to drop the post-add mirror application and remove this test."
|
||||
)
|
||||
|
||||
def test_mirror_y_arg_is_silently_dropped(self, tmp_path: Any) -> None:
|
||||
sch = tmp_path / "mirror_y.kicad_sch"
|
||||
shutil.copy(EMPTY_SCH, sch)
|
||||
self._add(sch, "y")
|
||||
text = sch.read_text()
|
||||
assert "(mirror y)" not in text, (
|
||||
"ComponentManager.add_component now appears to honor mirror='y'. "
|
||||
"See sibling test_mirror_x_arg_is_silently_dropped."
|
||||
)
|
||||
|
||||
@@ -106,6 +106,20 @@ def _build_diode_case(tmp: Path, rotation: int) -> tuple[Path, dict]:
|
||||
return sch_path, {("D1", "1"): "D1_K", ("D1", "2"): "D1_A"}
|
||||
|
||||
|
||||
def _apply_mirror_to_file(sch_path: Path, reference: str, axis: str) -> None:
|
||||
"""Apply (mirror x|y) to a placed symbol via direct sexpr mutation.
|
||||
|
||||
ComponentManager.add_component silently drops a 'mirror' kwarg, so this
|
||||
fixture goes around it via the same low-level helper rotate_schematic_component
|
||||
uses (WireDragger.update_symbol_rotation_mirror)."""
|
||||
import sexpdata
|
||||
|
||||
sch_data = sexpdata.loads(sch_path.read_text())
|
||||
if not WireDragger.update_symbol_rotation_mirror(sch_data, reference, 0, axis):
|
||||
raise RuntimeError(f"Failed to apply mirror={axis} to {reference}")
|
||||
sch_path.write_text(sexpdata.dumps(sch_data))
|
||||
|
||||
|
||||
def _build_mirror_case(tmp: Path, axis: str) -> tuple[Path, dict]:
|
||||
sch_path = tmp / f"resistor_mirror_{axis}.kicad_sch"
|
||||
template = PYTHON_DIR / "templates" / "template_with_symbols.kicad_sch"
|
||||
@@ -114,23 +128,23 @@ def _build_mirror_case(tmp: Path, axis: str) -> tuple[Path, dict]:
|
||||
sch = SchematicManager.load_schematic(str(sch_path))
|
||||
ComponentManager.add_component(
|
||||
sch,
|
||||
{
|
||||
"type": "R",
|
||||
"reference": "R1",
|
||||
"value": "10k",
|
||||
"x": 100.0,
|
||||
"y": 100.0,
|
||||
"rotation": 0,
|
||||
"mirror": axis,
|
||||
},
|
||||
{"type": "R", "reference": "R1", "value": "10k", "x": 100.0, "y": 100.0, "rotation": 0},
|
||||
sch_path,
|
||||
)
|
||||
SchematicManager.save_schematic(sch, str(sch_path))
|
||||
|
||||
_apply_mirror_to_file(sch_path, "R1", axis)
|
||||
if f"(mirror {axis})" not in sch_path.read_text():
|
||||
raise RuntimeError(
|
||||
f"Fixture failed to write (mirror {axis}) — the kicad-cli oracle would "
|
||||
f"silently match our pin coords for an unmirrored symbol."
|
||||
)
|
||||
|
||||
locator = PinLocator()
|
||||
p1 = locator.get_pin_location(sch_path, "R1", "1")
|
||||
p2 = locator.get_pin_location(sch_path, "R1", "2")
|
||||
assert p1 is not None and p2 is not None
|
||||
if p1 is None or p2 is None:
|
||||
raise RuntimeError(f"PinLocator returned None for R1 mirror={axis}")
|
||||
|
||||
_add_labels_to_file(sch_path, [("R1_PIN1", p1[0], p1[1]), ("R1_PIN2", p2[0], p2[1])])
|
||||
|
||||
|
||||
Reference in New Issue
Block a user