From 496be317e97f0015d3b4275d1d5fdfd15961b94c Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 22:40:34 +0100 Subject: [PATCH] test: close mirror-fixture gap exposed by post-fix audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 in 7e67cb9, 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 at 7e67cb9 reverted, the kicad-cli mirror tests now go RED (previously they stayed GREEN regardless). Co-Authored-By: Claude Opus 4.7 --- tests/test_add_schematic_component.py | 123 +++++++++++++++++----- tests/test_pin_world_xy_eeschema_truth.py | 34 ++++-- 2 files changed, 119 insertions(+), 38 deletions(-) diff --git a/tests/test_add_schematic_component.py b/tests/test_add_schematic_component.py index dff0227..92f1d1f 100644 --- a/tests/test_add_schematic_component.py +++ b/tests/test_add_schematic_component.py @@ -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." + ) diff --git a/tests/test_pin_world_xy_eeschema_truth.py b/tests/test_pin_world_xy_eeschema_truth.py index 454c5f5..06d6848 100644 --- a/tests/test_pin_world_xy_eeschema_truth.py +++ b/tests/test_pin_world_xy_eeschema_truth.py @@ -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])])