From 3c225809b95f9b58d9bb8104535345d57e357b94 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 21:50:14 +0100 Subject: [PATCH 1/8] =?UTF-8?q?fix(pin=5Flocator):=20apply=20lib=E2=86=92s?= =?UTF-8?q?creen=20Y-flip=20to=20get=5Fpin=5Fangle?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #145 restored the Y-axis flip in WireDragger.pin_world_xy so pin coordinates now match the schematic (Y-down) frame instead of the library (Y-up) frame. PinLocator.get_pin_angle was the companion to that transform but never received the matching fix: it was returning the library-frame angle (with mirror handling but no Y-flip), so angles came out 180° off along the Y axis. This was masked before PR #145 because pin_world_xy was wrong in the same direction — both functions skipped the Y-flip, so callers that compared pin endpoints to angles saw a self-consistent picture. Once pin_world_xy was corrected the inconsistency surfaced. Apply the same lib→screen Y-flip (negate angle) after the mirror handling and before the symbol-rotation add, matching pin_world_xy's order: mirror in lib space → Y-flip → rotate → translate (no translate for angles since angles are translation-invariant). Fixes the 24 parametrized cases in tests/test_get_pin_angle.py::test_get_pin_angle_matches_geometric_expectation (pin × mirror × rotation matrix). The test derives its expected value from pin_world_xy itself, making it the canonical geometric oracle. test_pin_locator_y_flip and test_move_with_wire_preservation continue to pass. Co-Authored-By: Claude Opus 4.7 --- python/commands/pin_locator.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/python/commands/pin_locator.py b/python/commands/pin_locator.py index 5b664b4..5b1c2ae 100644 --- a/python/commands/pin_locator.py +++ b/python/commands/pin_locator.py @@ -287,6 +287,15 @@ class PinLocator: if mirror_y: pin_def_angle = (-pin_def_angle) % 360 + # Library symbols are Y-up; the schematic is Y-down. Match the + # lib→screen Y-flip applied by WireDragger.pin_world_xy (mirror in + # lib space → Y-flip → rotate → translate). For an angle this + # negates the Y component, i.e. negates the angle. Without this + # step pin angles are 180° off along the Y axis; before PR #145 + # this was masked because pin_world_xy was missing the same flip, + # so the two were "wrong in the same direction" and consistent. + pin_def_angle = (-pin_def_angle) % 360 + absolute_angle = (pin_def_angle + symbol_rotation) % 360 return absolute_angle From 22eb3319f97dd8320e88bd0faa45bf30b74e1993 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 21:53:21 +0100 Subject: [PATCH 2/8] fix(pin_locator): rstrip "_" in WireDragger.find_symbol; clean stale tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves the four failing tests in tests/test_pin_locator_and_component.py left behind by the PR #145 / commit 3c22580 Y-flip work. Per-test rationale: - TestPinLocatorYAxisNegation::{test_pin1_y_above_center_for_rotation_0, test_pin2_y_below_center_for_rotation_0, test_pin1_rotated_90}: stale. Their assertions encoded the *correct* post-PR-145 convention (96.19, 103.81, etc.), but their setup MagicMock'd self._schematic_cache while bypassing _get_symbol_transform, which reads the .kicad_sch file directly via sexpdata. The end-to-end Y-flip behaviour is already covered against eeschema in tests/test_pin_locator_y_flip.py — keeping three mock-based duplicates added no value, so they were removed. - TestPinLocatorReferenceRstrip::test_get_pin_location_finds_symbol_with_trailing_underscore: revealed a real production bug. PinLocator.get_pin_location strips a trailing "_" on the kicad-skip lookup path, but the sexpdata-based _get_symbol_transform delegates to WireDragger.find_symbol which used an exact-equality comparison. With kicad-skip's "R1_" artifact the function returned None, so the whole pin-location call failed even when the symbol was clearly present. Fixed find_symbol to apply the same rstrip("_") on the stored reference before comparing, mirroring the existing behaviour in PinLocator. The test was also rewritten to use a real temp .kicad_sch (with the on-disk reference mangled to "R1_") so it actually exercises both lookup paths instead of bypassing one with mocks. Files changed: - python/commands/wire_dragger.py:78-89 — rstrip("_") on the reference read out of the symbol property before comparing to the caller-supplied reference. - tests/test_pin_locator_and_component.py — removed three stale mock-based Y-axis tests (covered by tests/test_pin_locator_y_flip.py end-to-end); rewrote rstrip tests to use a real schematic file so _get_symbol_transform is actually exercised. Verified: tests/test_pin_locator_and_component.py + test_pin_locator_y_flip.py + test_get_pin_angle.py + test_move_with_wire_preservation.py — 69 passed. Co-Authored-By: Claude Opus 4.7 --- python/commands/wire_dragger.py | 7 +- tests/test_pin_locator_and_component.py | 183 +++++++++--------------- 2 files changed, 74 insertions(+), 116 deletions(-) diff --git a/python/commands/wire_dragger.py b/python/commands/wire_dragger.py index 793457e..aac0fde 100644 --- a/python/commands/wire_dragger.py +++ b/python/commands/wire_dragger.py @@ -75,14 +75,17 @@ class WireDragger: if not (isinstance(item, list) and item and item[0] == sym_k): continue - # Check Reference property + # Check Reference property. + # kicad-skip may write a trailing "_" on references (e.g. "R1_") when + # cloning symbols; strip it so callers passing the canonical "R1" + # still find the symbol. Mirrors the rstrip in PinLocator.get_pin_location. ref_val = None for sub in item[1:]: if isinstance(sub, list) and len(sub) >= 3 and sub[0] == prop_k: if str(sub[1]).strip('"') == "Reference": ref_val = str(sub[2]).strip('"') break - if ref_val != reference: + if ref_val is None or ref_val.rstrip("_") != reference: continue old_x = old_y = rotation = 0.0 diff --git a/tests/test_pin_locator_and_component.py b/tests/test_pin_locator_and_component.py index 462b8f3..02e8eb7 100644 --- a/tests/test_pin_locator_and_component.py +++ b/tests/test_pin_locator_and_component.py @@ -1,17 +1,21 @@ """ -Regression tests for three bugs fixed in PR #103: +Regression tests for bugs originally fixed in PR #103 and updated for PR #145. 1. component_schematic.py: clone() + redundant append() causes trailing "_" on reference - 2. pin_locator.py: pin_rel_y must be negated (lib y-up → schematic y-down) - 3. pin_locator.py: reference comparison must tolerate trailing "_" from kicad-skip + 2. pin_locator.py: reference comparison must tolerate trailing "_" from kicad-skip + (this also covers WireDragger.find_symbol, used by _get_symbol_transform) + +The pre-PR-145 y-axis-negation tests were removed: their assertions encoded the +correct post-PR-145 convention, but their MagicMock setup bypassed +_get_symbol_transform (which reads the .kicad_sch file directly via sexpdata). +The y-flip behaviour is now covered end-to-end against eeschema in +tests/test_pin_locator_y_flip.py — duplicating it with mocks added no value. """ import shutil import sys import tempfile -import types from pathlib import Path -from unittest.mock import MagicMock, patch import pytest @@ -26,15 +30,6 @@ sys.path.insert(0, str(PYTHON_DIR)) _TEMPLATE_SCH = TEMPLATES_DIR / "template_with_symbols.kicad_sch" -def _stub_symbol(ref: str, at: list, lib_id: str = "Device:R") -> MagicMock: - """Build a minimal kicad-skip symbol stub.""" - sym = MagicMock() - sym.property.Reference.value = ref - sym.at.value = at - sym.lib_id.value = lib_id - return sym - - # =========================================================================== # 1. component_schematic — no trailing underscore after clone() # =========================================================================== @@ -71,118 +66,78 @@ class TestAddComponentNoTrailingUnderscore: # =========================================================================== -# 2. pin_locator — y-axis sign (lib y-up → schematic y-down) +# 2. pin_locator — .rstrip("_") tolerance in reference lookup # =========================================================================== -@pytest.mark.unit -class TestPinLocatorYAxisNegation: - """ - Device:R pin 1 is at library y=+3.81 (y-up). - For a symbol centred at (100, 100) with rotation=0, the schematic absolute y - must be 100 - 3.81 = 96.19, NOT 100 + 3.81 = 103.81. - """ - - @pytest.fixture(autouse=True) - def setup(self): - # Stub sexpdata and skip so the module can be imported without them installed - for mod_name in ("sexpdata", "skip"): - sys.modules.setdefault(mod_name, types.ModuleType(mod_name)) - from commands.pin_locator import PinLocator - - self.locator = PinLocator() - - def test_pin1_y_above_center_for_rotation_0(self): - """Pin at lib y=+3.81 should appear *above* the symbol centre (lower y value).""" - sym = _stub_symbol("R1", at=[100.0, 100.0, 0.0]) - self.locator._schematic_cache["test.kicad_sch"] = MagicMock(symbol=[sym]) - # Patch get_symbol_pins to return a Device:R-like pin definition - with patch.object( - self.locator, - "get_symbol_pins", - return_value={"1": {"x": 0.0, "y": 3.81, "angle": 270, "name": "~"}}, - ): - result = self.locator.get_pin_location(Path("test.kicad_sch"), "R1", "1") - - assert result is not None - x, y = result - assert abs(x - 100.0) < 1e-6, f"x should be 100.0, got {x}" - assert abs(y - 96.19) < 1e-4, ( - f"y should be ~96.19 (above centre), got {y}. " - "y was not negated — library y-up convention mismatch." - ) - - def test_pin2_y_below_center_for_rotation_0(self): - """Pin at lib y=-3.81 should appear *below* the symbol centre (higher y value).""" - sym = _stub_symbol("R1", at=[100.0, 100.0, 0.0]) - self.locator._schematic_cache["test.kicad_sch"] = MagicMock(symbol=[sym]) - with patch.object( - self.locator, - "get_symbol_pins", - return_value={"2": {"x": 0.0, "y": -3.81, "angle": 90, "name": "~"}}, - ): - result = self.locator.get_pin_location(Path("test.kicad_sch"), "R1", "2") - - assert result is not None - _, y = result - assert abs(y - 103.81) < 1e-4, f"y should be ~103.81 (below centre), got {y}." - - def test_pin1_rotated_90(self): - """ - Symbol rotated 90°. Pin at lib (x=0, y=+3.81). - After y-negation: (0, -3.81). After 90° CCW rotation: (x=3.81, y=0). - Absolute: (100+3.81, 100+0) = (103.81, 100). - """ - sym = _stub_symbol("C1", at=[100.0, 100.0, 90.0]) - self.locator._schematic_cache["test.kicad_sch"] = MagicMock(symbol=[sym]) - with patch.object( - self.locator, - "get_symbol_pins", - return_value={"1": {"x": 0.0, "y": 3.81, "angle": 270, "name": "~"}}, - ): - result = self.locator.get_pin_location(Path("test.kicad_sch"), "C1", "1") - - assert result is not None - x, y = result - assert abs(x - 103.81) < 1e-4, f"x should be ~103.81, got {x}" - assert abs(y - 100.0) < 1e-4, f"y should be ~100.0, got {y}" - - -# =========================================================================== -# 3. pin_locator — .rstrip("_") tolerance in reference lookup -# =========================================================================== - - -@pytest.mark.unit +@pytest.mark.integration class TestPinLocatorReferenceRstrip: - """kicad-skip may write 'R1_' — lookups must still find 'R1'.""" + """ + kicad-skip may write 'R1_' on disk after a clone; lookups for 'R1' must + still resolve. This must hold for *both* lookup paths inside + get_pin_location: the kicad-skip Schematic scan AND the sexpdata-based + _get_symbol_transform (via WireDragger.find_symbol). + """ - @pytest.fixture(autouse=True) - def setup(self): - for mod_name in ("sexpdata", "skip"): - sys.modules.setdefault(mod_name, types.ModuleType(mod_name)) - from commands.pin_locator import PinLocator + def _write_sch_with_underscored_ref(self, sch_path: Path) -> None: + """Add R1, then mangle the on-disk reference to 'R1_' to simulate the kicad-skip artifact.""" + from commands.component_schematic import ComponentManager + from commands.schematic import SchematicManager - self.locator = PinLocator() + shutil.copy(_TEMPLATE_SCH, sch_path) + 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}, + sch_path, + ) + SchematicManager.save_schematic(sch, str(sch_path)) + + # Rewrite the saved file, replacing the Reference "R1" with "R1_" + text = sch_path.read_text(encoding="utf-8") + text = text.replace('(property "Reference" "R1"', '(property "Reference" "R1_"', 1) + sch_path.write_text(text, encoding="utf-8") def test_get_pin_location_finds_symbol_with_trailing_underscore(self): - # Symbol stored in schematic with reference 'R1_' (kicad-skip artifact) - sym = _stub_symbol("R1_", at=[50.0, 50.0, 0.0]) - self.locator._schematic_cache["sch.kicad_sch"] = MagicMock(symbol=[sym]) - with patch.object( - self.locator, - "get_symbol_pins", - return_value={"1": {"x": 0.0, "y": 3.81, "angle": 270, "name": "~"}}, - ): - # Caller uses clean reference 'R1'; should still resolve - result = self.locator.get_pin_location(Path("sch.kicad_sch"), "R1", "1") + from commands.pin_locator import PinLocator + + with tempfile.TemporaryDirectory() as tmp: + sch_path = Path(tmp) / "sch.kicad_sch" + self._write_sch_with_underscored_ref(sch_path) + + locator = PinLocator() + # Caller uses clean reference 'R1'; should still resolve through both + # the kicad-skip path and the sexpdata _get_symbol_transform path. + result = locator.get_pin_location(sch_path, "R1", "1") assert ( result is not None ), "get_pin_location returned None for reference 'R1' when schematic stores 'R1_'" def test_get_pin_location_returns_none_for_genuinely_missing_symbol(self): - sym = _stub_symbol("R2", at=[50.0, 50.0, 0.0]) - self.locator._schematic_cache["sch.kicad_sch"] = MagicMock(symbol=[sym]) - result = self.locator.get_pin_location(Path("sch.kicad_sch"), "R1", "1") + from commands.component_schematic import ComponentManager + from commands.pin_locator import PinLocator + from commands.schematic import SchematicManager + + with tempfile.TemporaryDirectory() as tmp: + sch_path = Path(tmp) / "sch.kicad_sch" + shutil.copy(_TEMPLATE_SCH, sch_path) + sch = SchematicManager.load_schematic(str(sch_path)) + ComponentManager.add_component( + sch, + { + "type": "R", + "reference": "R2", + "value": "1k", + "x": 50.0, + "y": 50.0, + "rotation": 0, + }, + sch_path, + ) + SchematicManager.save_schematic(sch, str(sch_path)) + + locator = PinLocator() + result = locator.get_pin_location(sch_path, "R1", "1") + assert result is None From b4cbf606dcfae5031c794e511bb8a3db0784c8fe Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 21:59:04 +0100 Subject: [PATCH 3/8] test(hierarchical_pad_net_map): write symbol instances to disk for sexp transform read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All 10 failing tests in test_hierarchical_pad_net_map.py share the same root cause: broken test fixture, not a production bug. PR #88 (commit 7cafbda, "fix: get_schematic_pin_locations now accounts for mirror flags") introduced PinLocator._get_symbol_transform, which reads symbol position/rotation/lib_id directly from the .kicad_sch file via sexpdata + WireDragger.find_symbol — it deliberately bypasses the kicad-skip cache so mirror/rotation mutations are authoritative. The hierarchical-pad-net-map tests, however, only mocked skip.Schematic and wrote a lib_symbols-only stub to disk with no (symbol ...) instance, so _get_symbol_transform returned None and every pin lookup failed with "Could not read transform for R1/R2". The tests last passed at 8a42812 (introduction) and broke at 7cafbda; PR #145 did not touch this code path. Per-test classification (all 10): broken fixture. - TestLabelAtPin::test_global_label_pin1 — fixture - TestLabelAtPin::test_global_label_pin2 — fixture - TestLabelAtPin::test_both_pins_mapped — fixture - TestLabelAtPin::test_local_label_also_works — fixture - TestLabelAtPin::test_hierarchical_label_also_works — fixture - TestLabelViaWire::test_label_one_hop_away — fixture - TestLabelViaWire::test_label_two_hops_away — fixture - TestMultipleSubsheets::test_components_in_subsheet_collected — fixture - TestMultipleSubsheets::test_top_and_sub_components_merged — fixture - TestRotatedSymbol::test_90_degree_rotation — fixture (rotation expected values already match the post-PR-145 convention; only fixture needed) Fix: add _build_sch_with_instances() helper that emits a real (symbol ...) block alongside the existing TestLib:R lib_symbols, so sexpdata can resolve the transform. The skip.Schematic mock is still used for labels and wires. All 15 tests in this file now pass; the broader related set (test_pin_locator_y_flip, test_get_pin_angle, test_move_with_wire_preservation, test_pin_locator_and_component) also pass — 84 total. Co-Authored-By: Claude Opus 4.7 --- tests/test_hierarchical_pad_net_map.py | 57 +++++++++++++++++++------- 1 file changed, 43 insertions(+), 14 deletions(-) diff --git a/tests/test_hierarchical_pad_net_map.py b/tests/test_hierarchical_pad_net_map.py index 922edbb..8db04dd 100644 --- a/tests/test_hierarchical_pad_net_map.py +++ b/tests/test_hierarchical_pad_net_map.py @@ -36,8 +36,7 @@ sys.path.insert(0, str(Path(__file__).parent.parent / "python")) # pin "2" at ( 1.27, 0) → wire connects on the right # --------------------------------------------------------------------------- -_SCH_WITH_TESTLIB_R = textwrap.dedent("""\ - (kicad_sch (version 20231120) +_LIB_SYMBOLS_BLOCK = textwrap.dedent("""\ (lib_symbols (symbol "TestLib:R" (symbol "TestLib:R_1_1" @@ -52,12 +51,40 @@ _SCH_WITH_TESTLIB_R = textwrap.dedent("""\ ) ) ) - ) """) +_SCH_WITH_TESTLIB_R = "(kicad_sch (version 20231120)\n" + _LIB_SYMBOLS_BLOCK + ")\n" + _SCH_EMPTY = "(kicad_sch (version 20231120))" +def _build_sch_with_instances( + instances: "list[tuple[str, str, float, float, float]]" = (), +) -> str: + """Build a .kicad_sch text with TestLib:R lib_symbols + the given symbol instances. + + Each instance tuple: (reference, lib_id, x, y, rotation). + + Required because PinLocator._get_symbol_transform reads the symbol position, + rotation, and lib_id directly from disk via sexpdata (it does not consult + the kicad-skip cache that the tests mock for labels/wires). + """ + parts = ["(kicad_sch (version 20231120)", _LIB_SYMBOLS_BLOCK] + for i, (ref, lib_id, x, y, rot) in enumerate(instances, start=1): + uuid = f"00000000-0000-0000-0000-{i:012d}" + parts.append( + f' (symbol (lib_id "{lib_id}") (at {x} {y} {rot}) (unit 1)\n' + f" (uuid {uuid})\n" + f' (property "Reference" "{ref}" (at {x} {y - 2.54} 0))\n' + f' (property "Value" "~" (at {x} {y + 2.54} 0))\n' + f' (pin "1" (uuid {uuid[:-1]}a))\n' + f' (pin "2" (uuid {uuid[:-1]}b))\n' + f" )\n" + ) + parts.append(")\n") + return "".join(parts) + + # --------------------------------------------------------------------------- # Mock helpers # --------------------------------------------------------------------------- @@ -176,7 +203,7 @@ class TestLabelAtPin: def test_global_label_pin1(self, iface, tmp_path): """Global label at pin-1 position → (R1, 1) in map.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) # R1 at (10, 10); pin 1 abs = (10 − 1.27, 10) = (8.73, 10) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -189,7 +216,7 @@ class TestLabelAtPin: def test_global_label_pin2(self, iface, tmp_path): """Global label at pin-2 position → (R1, 2) in map.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) # pin 2 abs = (10 + 1.27, 10) = (11.27, 10) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -202,7 +229,7 @@ class TestLabelAtPin: def test_both_pins_mapped(self, iface, tmp_path): """Labels at both pin positions → both (ref, pin) keys present.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], global_labels=[ @@ -217,7 +244,7 @@ class TestLabelAtPin: def test_local_label_also_works(self, iface, tmp_path): """Local (net) labels are treated identically to global labels.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], labels=[_lbl_mock("LOCAL_NET", 8.73, 10.0)], @@ -228,7 +255,7 @@ class TestLabelAtPin: def test_hierarchical_label_also_works(self, iface, tmp_path): """Hierarchical labels are treated identically to global labels.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], hier_labels=[_lbl_mock("HIER_NET", 8.73, 10.0)], @@ -249,7 +276,7 @@ class TestLabelViaWire: def test_label_one_hop_away(self, iface, tmp_path): """Label at wire start, wire end at pin → net assigned.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) # pin 1 at (8.73, 10); label at (5.0, 10); wire (5.0,10)→(8.73,10) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -262,7 +289,7 @@ class TestLabelViaWire: def test_label_two_hops_away(self, iface, tmp_path): """Net propagates through two chained wire segments.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) # label at (3.0, 10); wire1: 3→6; wire2: 6→8.73 → pin 1 mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -330,7 +357,7 @@ class TestMultipleSubsheets: sub_dir = tmp_path / "sheets" sub_dir.mkdir() sub = sub_dir / "component_sheet.kicad_sch" - sub.write_text(_SCH_WITH_TESTLIB_R) + sub.write_text(_build_sch_with_instances([("R2", "TestLib:R", 10.0, 10.0, 0)])) top_mock = _sch_mock() # top sheet has no components sub_mock = _sch_mock( @@ -340,6 +367,7 @@ class TestMultipleSubsheets: def _factory(path: str) -> MagicMock: from pathlib import Path as _P + return sub_mock if _P(path).name == "component_sheet.kicad_sch" else top_mock with ( @@ -354,11 +382,11 @@ class TestMultipleSubsheets: def test_top_and_sub_components_merged(self, iface, tmp_path): """Components from both top-level and sub-sheet appear in the same map.""" top = tmp_path / "top.kicad_sch" - top.write_text(_SCH_WITH_TESTLIB_R) + top.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 0)])) sub_dir = tmp_path / "sheets" sub_dir.mkdir() sub = sub_dir / "component_sheet.kicad_sch" - sub.write_text(_SCH_WITH_TESTLIB_R) + sub.write_text(_build_sch_with_instances([("R2", "TestLib:R", 10.0, 10.0, 0)])) top_mock = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0)], @@ -371,6 +399,7 @@ class TestMultipleSubsheets: def _factory(path: str) -> MagicMock: from pathlib import Path as _P + return sub_mock if _P(path).name == "component_sheet.kicad_sch" else top_mock with ( @@ -393,7 +422,7 @@ class TestRotatedSymbol: def test_90_degree_rotation(self, iface, tmp_path): """A 90° CCW rotation maps pin (-1.27, 0) → (0, -1.27) in local coords.""" sch = tmp_path / "top.kicad_sch" - sch.write_text(_SCH_WITH_TESTLIB_R) + sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 90)])) # R1 at (10, 10), rotation=90° # pin 1 (-1.27, 0) rotated 90° CCW → (0, -1.27) → abs (10.0, 8.73) # pin 2 ( 1.27, 0) rotated 90° CCW → (0, 1.27) → abs (10.0, 11.27) From 91011304236bb3aa71fdd84c1f0da757f33b97b2 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 22:00:08 +0100 Subject: [PATCH 4/8] fix: tighten _build_sch_with_instances default arg Use None default + 'instances or []' to keep mutable-default lint happy and satisfy the Iterable annotation. Co-Authored-By: Claude Opus 4.7 --- tests/test_hierarchical_pad_net_map.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_hierarchical_pad_net_map.py b/tests/test_hierarchical_pad_net_map.py index 8db04dd..2c24780 100644 --- a/tests/test_hierarchical_pad_net_map.py +++ b/tests/test_hierarchical_pad_net_map.py @@ -59,7 +59,7 @@ _SCH_EMPTY = "(kicad_sch (version 20231120))" def _build_sch_with_instances( - instances: "list[tuple[str, str, float, float, float]]" = (), + instances: "list[tuple[str, str, float, float, float]] | None" = None, ) -> str: """Build a .kicad_sch text with TestLib:R lib_symbols + the given symbol instances. @@ -70,7 +70,7 @@ def _build_sch_with_instances( the kicad-skip cache that the tests mock for labels/wires). """ parts = ["(kicad_sch (version 20231120)", _LIB_SYMBOLS_BLOCK] - for i, (ref, lib_id, x, y, rot) in enumerate(instances, start=1): + for i, (ref, lib_id, x, y, rot) in enumerate(instances or [], start=1): uuid = f"00000000-0000-0000-0000-{i:012d}" parts.append( f' (symbol (lib_id "{lib_id}") (at {x} {y} {rot}) (unit 1)\n' From 118318c2f3a8cab55778f6827870b82feb68c7e0 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 22:02:22 +0100 Subject: [PATCH 5/8] test(rotate_schematic_mirror): isolate sys.modules stubs to fix test pollution `test_rotate_handler_no_crash` permanently replaced `sys.modules["schemas.tool_schemas"].TOOL_SCHEMAS` with `[]`, leaking into later tests. When test_wire_connectivity (or any test) ran after this one and did `from schemas.tool_schemas import TOOL_SCHEMAS`, it got the empty list and `TOOL_SCHEMAS["get_wire_connections"]` raised `TypeError: list indices must be integers or slices, not str`. Save the original sys.modules entries and restore them in a `finally` block so the stubs are scoped to the test body. Whole suite now passes (678 tests, previously 4 failed in TestSchema when run in suite order). Co-Authored-By: Claude Opus 4.7 --- tests/test_rotate_schematic_mirror.py | 80 +++++++++++++++++---------- 1 file changed, 52 insertions(+), 28 deletions(-) diff --git a/tests/test_rotate_schematic_mirror.py b/tests/test_rotate_schematic_mirror.py index dba2c03..f5837ae 100644 --- a/tests/test_rotate_schematic_mirror.py +++ b/tests/test_rotate_schematic_mirror.py @@ -6,13 +6,13 @@ Tests are split into two layers: 2. Handler integration smoke test — patches SchematicManager away. """ +import importlib.util +import math import os import sys -import math -import textwrap import tempfile -import importlib.util -from unittest.mock import patch, MagicMock +import textwrap +from unittest.mock import MagicMock, patch import sexpdata from sexpdata import Symbol @@ -40,6 +40,7 @@ WireDragger = _wd_mod.WireDragger # Helpers # --------------------------------------------------------------------------- + def _parse(text: str) -> list: return sexpdata.loads(text) @@ -80,6 +81,7 @@ def _make_sch(sym_extra: str = "", wires: str = "") -> list: # Tests: update_symbol_rotation_mirror # --------------------------------------------------------------------------- + def test_update_rotation_sets_angle(): sch = _make_sch() result = WireDragger.update_symbol_rotation_mirror(sch, "Q1", 90.0, None) @@ -130,6 +132,7 @@ def test_update_unknown_reference_returns_false(): # Tests: compute_pin_positions_for_rotation # --------------------------------------------------------------------------- + def test_pin_positions_change_on_rotation(): """Pins at non-zero local offsets should move when the symbol rotates.""" sch = _make_sch() @@ -179,6 +182,7 @@ def test_pin_positions_mirror_x_flips_x(): # Integration smoke test: handler uses sexpdata, not kicad-skip # --------------------------------------------------------------------------- + def test_rotate_handler_no_crash(tmp_path): """_handle_rotate_schematic_component should succeed without kicad-skip.""" # Ensure python/ is on sys.path so commands.* imports resolve @@ -186,26 +190,44 @@ def test_rotate_handler_no_crash(tmp_path): if _python_dir not in sys.path: sys.path.insert(0, _python_dir) - # Stub heavy imports before loading kicad_interface - for modname in ("pcbnew", "skip", "resources", "schemas", - "resources.resource_definitions", "schemas.tool_schemas", - "annotations"): - sys.modules.setdefault(modname, MagicMock()) - sys.modules["resources.resource_definitions"].RESOURCE_DEFINITIONS = {} - sys.modules["resources.resource_definitions"].handle_resource_read = MagicMock() - sys.modules["schemas.tool_schemas"].TOOL_SCHEMAS = [] - - _pcbnew = sys.modules["pcbnew"] - _pcbnew.__file__ = "/fake/pcbnew.so" - _pcbnew.GetBuildVersion.return_value = "9.0.0" - - ki_spec = importlib.util.spec_from_file_location( - "kicad_interface_smoke", - os.path.join(os.path.dirname(__file__), "..", "python", "kicad_interface.py"), + # Stub heavy imports before loading kicad_interface. Save and restore + # sys.modules state so we don't pollute already-imported real modules + # (notably schemas.tool_schemas, whose TOOL_SCHEMAS dict is shared across + # the test session). + _stub_modnames = ( + "pcbnew", + "skip", + "resources", + "schemas", + "resources.resource_definitions", + "schemas.tool_schemas", + "annotations", ) - ki_mod = importlib.util.module_from_spec(ki_spec) - ki_spec.loader.exec_module(ki_mod) - KiCADInterface = ki_mod.KiCADInterface + _saved_modules = {n: sys.modules.get(n) for n in _stub_modnames} + try: + for modname in _stub_modnames: + sys.modules[modname] = MagicMock() + sys.modules["resources.resource_definitions"].RESOURCE_DEFINITIONS = {} + sys.modules["resources.resource_definitions"].handle_resource_read = MagicMock() + sys.modules["schemas.tool_schemas"].TOOL_SCHEMAS = [] + + _pcbnew = sys.modules["pcbnew"] + _pcbnew.__file__ = "/fake/pcbnew.so" + _pcbnew.GetBuildVersion.return_value = "9.0.0" + + ki_spec = importlib.util.spec_from_file_location( + "kicad_interface_smoke", + os.path.join(os.path.dirname(__file__), "..", "python", "kicad_interface.py"), + ) + ki_mod = importlib.util.module_from_spec(ki_spec) + ki_spec.loader.exec_module(ki_mod) + KiCADInterface = ki_mod.KiCADInterface + finally: + for modname, mod in _saved_modules.items(): + if mod is None: + sys.modules.pop(modname, None) + else: + sys.modules[modname] = mod # Write a minimal schematic file sch_path = str(tmp_path / "test.kicad_sch") @@ -233,11 +255,13 @@ def test_rotate_handler_no_crash(tmp_path): f.write(sch_content) iface = KiCADInterface.__new__(KiCADInterface) - result = iface._handle_rotate_schematic_component({ - "schematicPath": sch_path, - "reference": "R1", - "angle": 90, - }) + result = iface._handle_rotate_schematic_component( + { + "schematicPath": sch_path, + "reference": "R1", + "angle": 90, + } + ) assert result["success"] is True assert result["angle"] == 90 From f7660e15addf3723a8f93640cbb7aeba0bea9f8d Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 22:24:14 +0100 Subject: [PATCH 6/8] test: encode eeschema kicad-cli ground truth (currently RED) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update existing rotation/mirror assertions and add a kicad-cli-grounded regression suite. eeschema's pin transform is verified against: - rotation: TRANSFORM(0,1,-1,0) for rot=90 — CCW in screen Y-down - mirror x: SYM_MIRROR_X = TRANSFORM(1,0,0,-1) — negates internal Y - mirror y: SYM_MIRROR_Y = TRANSFORM(-1,0,0,1) — negates internal X Updated assertions: - test_pin_locator_y_flip: rotated cap pin 1 X 153.81 → 146.19 - test_move_with_wire_preservation::test_resistor_rotated_90: pin 1 X 103.81 → 96.19 - test_hierarchical_pad_net_map::test_90_degree_rotation: swapped UP_NET / DN_NET label coords New file tests/test_pin_world_xy_eeschema_truth.py: - 4 parametrized diode tests via kicad-cli netlist (oracle = eeschema) - 2 parametrized resistor mirror tests via kicad-cli netlist - 3 pure-math pin_world_xy assertions for rot=90, mirror_x, mirror_y Currently RED on rotation 90/270 (rotation direction) and mirror_x/y (axis semantics swapped). Production fix in WireDragger.pin_world_xy to follow. Co-Authored-By: Claude Opus 4.7 --- tests/test_hierarchical_pad_net_map.py | 14 +- tests/test_move_with_wire_preservation.py | 7 +- tests/test_pin_locator_y_flip.py | 9 +- tests/test_pin_world_xy_eeschema_truth.py | 216 ++++++++++++++++++++++ 4 files changed, 233 insertions(+), 13 deletions(-) create mode 100644 tests/test_pin_world_xy_eeschema_truth.py diff --git a/tests/test_hierarchical_pad_net_map.py b/tests/test_hierarchical_pad_net_map.py index 2c24780..6ff8a0f 100644 --- a/tests/test_hierarchical_pad_net_map.py +++ b/tests/test_hierarchical_pad_net_map.py @@ -420,17 +420,19 @@ class TestMultipleSubsheets: @pytest.mark.unit class TestRotatedSymbol: def test_90_degree_rotation(self, iface, tmp_path): - """A 90° CCW rotation maps pin (-1.27, 0) → (0, -1.27) in local coords.""" + """eeschema rot=90 is CCW in screen Y-down: TRANSFORM(0,1,-1,0). + Pin 1 lib (-1.27, 0) → internal (-1.27, 0) → (0,1,-1,0) applied = (0, 1.27) + → world (10.0, 11.27). + Pin 2 lib ( 1.27, 0) → internal ( 1.27, 0) → (0,1,-1,0) applied = (0, -1.27) + → world (10.0, 8.73). + Verified vs kicad-cli netlist on a Device:D rotated 90.""" sch = tmp_path / "top.kicad_sch" sch.write_text(_build_sch_with_instances([("R1", "TestLib:R", 10.0, 10.0, 90)])) - # R1 at (10, 10), rotation=90° - # pin 1 (-1.27, 0) rotated 90° CCW → (0, -1.27) → abs (10.0, 8.73) - # pin 2 ( 1.27, 0) rotated 90° CCW → (0, 1.27) → abs (10.0, 11.27) mock_sch = _sch_mock( symbols=[_sym_mock("R1", "TestLib:R", 10.0, 10.0, rotation=90)], global_labels=[ - _lbl_mock("UP_NET", 10.0, 8.73), - _lbl_mock("DN_NET", 10.0, 11.27), + _lbl_mock("UP_NET", 10.0, 11.27), + _lbl_mock("DN_NET", 10.0, 8.73), ], ) pad_net_map, _ = _call(iface, sch, mock_sch) diff --git a/tests/test_move_with_wire_preservation.py b/tests/test_move_with_wire_preservation.py index 65579cb..4edbd4b 100644 --- a/tests/test_move_with_wire_preservation.py +++ b/tests/test_move_with_wire_preservation.py @@ -222,13 +222,14 @@ class TestComputePinPositions: assert abs(new2[1] - 23.81) < 1e-4 def test_resistor_rotated_90(self) -> None: - """Device:R at (100, 100) rot=90. Pin 1 lib (0, +3.81) → y-flipped to - (0, -3.81) → rotated 90° → (3.81, 0) → world (103.81, 100).""" + """Device:R at (100, 100) rot=90. Pin 1 lib (0, +3.81), Y-flip → (0, -3.81), + eeschema rot=90 TRANSFORM(0,1,-1,0): (0*0+1*-3.81, -1*0+0*-3.81) = (-3.81, 0). + World (96.19, 100). Verified vs kicad-cli netlist.""" sch = _make_sch_data([_make_symbol("R1", 100, 100, rotation=90)]) positions = WireDragger.compute_pin_positions(sch, "R1", 100, 100) old1, _ = positions["1"] old2, _ = positions["2"] - assert abs(old1[0] - 103.81) < 1e-3 + assert abs(old1[0] - 96.19) < 1e-3 assert abs(old1[1] - 100) < 1e-3 def test_returns_empty_for_missing_component(self) -> None: diff --git a/tests/test_pin_locator_y_flip.py b/tests/test_pin_locator_y_flip.py index ec83f0d..e734171 100644 --- a/tests/test_pin_locator_y_flip.py +++ b/tests/test_pin_locator_y_flip.py @@ -105,8 +105,9 @@ def test_rotated_capacitor_pin_x_matches_render_convention(): p1 = locator.get_pin_location(sch_path, "C1", "1") assert p1 is not None - # Device:C pin 1 is at symbol (0, +3.81). After y-negate → (0, -3.81). - # Rotated 90° CCW in screen coords: (0, -3.81) → (3.81, 0). - # Absolute: (150+3.81, 100+0) = (153.81, 100). - assert p1[0] == pytest.approx(153.81), f"rotated pin 1 X wrong: {p1[0]}" + # Device:C pin 1 lib (0, +3.81). parseXY(invertY=true) → internal (0, -3.81). + # Rotation 90 in eeschema is CCW in screen Y-down: TRANSFORM(0,1,-1,0). + # Apply: (0*0 + 1*(-3.81), -1*0 + 0*(-3.81)) = (-3.81, 0). + # World: (150-3.81, 100) = (146.19, 100). Verified vs kicad-cli netlist. + assert p1[0] == pytest.approx(146.19), f"rotated pin 1 X wrong: {p1[0]}" assert p1[1] == pytest.approx(100.0), f"rotated pin 1 Y wrong: {p1[1]}" diff --git a/tests/test_pin_world_xy_eeschema_truth.py b/tests/test_pin_world_xy_eeschema_truth.py new file mode 100644 index 0000000..454c5f5 --- /dev/null +++ b/tests/test_pin_world_xy_eeschema_truth.py @@ -0,0 +1,216 @@ +""" +Ground-truth regression for WireDragger.pin_world_xy + label snap. + +The oracle is eeschema itself, accessed via `kicad-cli sch export netlist`. +For each (rotation, mirror) corner case, we: + 1. Build a schematic with a polarized component (Device:D, K=pin1, A=pin2). + 2. Apply the transform via the MCP rotate handler. + 3. Snap a label "_K" to pin 1 and "_A" to pin 2 via PinLocator coords. + 4. Run kicad-cli to extract the netlist. + 5. Assert each label's net binds to the *named* pin in the netlist — + i.e. label "_K" must end up on pin 1 (K), not pin 2 (A). + +If our pin coords agree with eeschema's render, the labels land on the +intended pins. If they disagree, the netlist swaps them, exposing the bug. + +Skips if kicad-cli or the system Device library aren't available. +""" + +import os +import shutil +import subprocess +import sys +import tempfile +import xml.etree.ElementTree as ET +from pathlib import Path + +import pytest + +PYTHON_DIR = Path(__file__).parent.parent / "python" +sys.path.insert(0, str(PYTHON_DIR)) + +from commands.component_schematic import ComponentManager # noqa: E402 +from commands.pin_locator import PinLocator # noqa: E402 +from commands.schematic import SchematicManager # noqa: E402 +from commands.wire_dragger import WireDragger # noqa: E402 + +_KICAD_CLI = shutil.which("kicad-cli") +_DEVICE_LIB = Path("/usr/share/kicad/symbols/Device.kicad_sym") + +pytestmark = [ + pytest.mark.integration, + pytest.mark.skipif(_KICAD_CLI is None, reason="kicad-cli not on PATH"), + pytest.mark.skipif(not _DEVICE_LIB.exists(), reason="Device lib not installed"), +] + + +def _add_labels_to_file(sch_path: Path, labels: list[tuple[str, float, float]]) -> None: + """Inject (label ...) tokens before the closing ')' of a .kicad_sch file.""" + text = sch_path.read_text() + block = "\n" + for name, x, y in labels: + block += ( + f' (label "{name}"\n' + f" (at {x} {y} 0)\n" + f" (effects (font (size 1.27 1.27)) (justify left bottom))\n" + f' (uuid "00000000-0000-0000-0000-{abs(hash(name)) % 10**12:012d}")\n' + f" )\n" + ) + last = text.rstrip().rfind(")") + sch_path.write_text(text[:last] + block + text[last:]) + + +def _extract_pin_to_net(netlist_xml: Path) -> dict: + """Return {(ref, pin_num): net_name} from a kicad XML netlist.""" + tree = ET.parse(netlist_xml) + root = tree.getroot() + out = {} + for net in root.findall(".//net"): + net_name = net.attrib.get("name", "").lstrip("/") + for node in net.findall("node"): + ref = node.attrib.get("ref") + pin = node.attrib.get("pin") + if ref and pin: + out[(ref, pin)] = net_name + return out + + +def _build_diode_case(tmp: Path, rotation: int) -> tuple[Path, dict]: + """Place a Device:D, rotate, snap labels, save. Returns (sch_path, expected_map).""" + sch_path = tmp / f"diode_rot{rotation}.kicad_sch" + template = PYTHON_DIR / "templates" / "template_with_symbols.kicad_sch" + shutil.copy(template, sch_path) + + sch = SchematicManager.load_schematic(str(sch_path)) + ComponentManager.add_component( + sch, + { + "type": "D", + "reference": "D1", + "value": "1N4148", + "x": 100.0, + "y": 100.0, + "rotation": rotation, + }, + sch_path, + ) + SchematicManager.save_schematic(sch, str(sch_path)) + + locator = PinLocator() + p_k = locator.get_pin_location(sch_path, "D1", "1") + p_a = locator.get_pin_location(sch_path, "D1", "2") + assert p_k is not None and p_a is not None + + _add_labels_to_file(sch_path, [("D1_K", p_k[0], p_k[1]), ("D1_A", p_a[0], p_a[1])]) + + return sch_path, {("D1", "1"): "D1_K", ("D1", "2"): "D1_A"} + + +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" + shutil.copy(template, sch_path) + + 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, + }, + sch_path, + ) + SchematicManager.save_schematic(sch, str(sch_path)) + + 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 + + _add_labels_to_file(sch_path, [("R1_PIN1", p1[0], p1[1]), ("R1_PIN2", p2[0], p2[1])]) + + return sch_path, {("R1", "1"): "R1_PIN1", ("R1", "2"): "R1_PIN2"} + + +def _run_netlist(sch_path: Path) -> dict: + out = sch_path.with_suffix(".net") + env = {**os.environ, "KICAD_SYMBOL_DIR": "/usr/share/kicad/symbols"} + subprocess.run( + [ + _KICAD_CLI, + "sch", + "export", + "netlist", + "--format", + "kicadxml", + "-o", + str(out), + str(sch_path), + ], + check=True, + capture_output=True, + env=env, + ) + return _extract_pin_to_net(out) + + +@pytest.mark.parametrize("rotation", [0, 90, 180, 270]) +def test_diode_label_polarity_through_eeschema(rotation): + """Snap-labelled K must show up on pin 1 in the kicad-cli netlist.""" + with tempfile.TemporaryDirectory() as td: + sch_path, expected = _build_diode_case(Path(td), rotation) + actual = _run_netlist(sch_path) + for (ref, pin), net in expected.items(): + assert actual.get((ref, pin)) == net, ( + f"rotation={rotation}: D1.{pin} label landed on wrong pin. " + f"Expected net={net}, got {actual.get((ref, pin))}. " + f"Full mapping: {actual}" + ) + + +@pytest.mark.parametrize("axis", ["x", "y"]) +def test_mirrored_resistor_label_through_eeschema(axis): + """Snap-labelled pin 1 must show up on pin 1 after (mirror x) / (mirror y).""" + with tempfile.TemporaryDirectory() as td: + sch_path, expected = _build_mirror_case(Path(td), axis) + actual = _run_netlist(sch_path) + for (ref, pin), net in expected.items(): + assert actual.get((ref, pin)) == net, ( + f"mirror={axis}: R1.{pin} label landed on wrong pin. " + f"Expected net={net}, got {actual.get((ref, pin))}. " + f"Full mapping: {actual}" + ) + + +def test_pin_world_xy_rot90_matches_eeschema_transform(): + """Pure-math regression: pin_world_xy for Device:R rot=90 must match + eeschema's TRANSFORM(0,1,-1,0) applied to internal Y-flipped pin.""" + # Device:R pin 1: lib (0, +3.81). parseXY(invertY=true) → internal (0, -3.81). + # TRANSFORM(0,1,-1,0) applied: (0*0 + 1*-3.81, -1*0 + 0*-3.81) = (-3.81, 0). + # Symbol at (100, 100) → world (96.19, 100). + wx, wy = WireDragger.pin_world_xy(0.0, 3.81, 100.0, 100.0, 90, False, False) + assert wx == pytest.approx(96.19), f"rot=90 X wrong: {wx} (expected 96.19)" + assert wy == pytest.approx(100.0), f"rot=90 Y wrong: {wy} (expected 100.0)" + + +def test_pin_world_xy_mirror_x_matches_eeschema(): + """(mirror x) = SYM_MIRROR_X = TRANSFORM(1,0,0,-1) → negates internal Y. + Device:R pin 1 lib (0, +3.81) → internal (0, -3.81) → mirror_x → (0, 3.81) + → symbol (100, 100) → world (100, 103.81). Pin should NOT be at (100, 96.19).""" + wx, wy = WireDragger.pin_world_xy(0.0, 3.81, 100.0, 100.0, 0, True, False) + assert wx == pytest.approx(100.0) + assert wy == pytest.approx(103.81), f"mirror_x Y wrong: {wy} (expected 103.81)" + + +def test_pin_world_xy_mirror_y_matches_eeschema(): + """(mirror y) = SYM_MIRROR_Y = TRANSFORM(-1,0,0,1) → negates internal X. + Device:R pin 1 lib (0, +3.81) → internal (0, -3.81) → mirror_y → (0, -3.81) + → world (100, 96.19). Y of pin 1 is unchanged by mirror across Y axis.""" + wx, wy = WireDragger.pin_world_xy(0.0, 3.81, 100.0, 100.0, 0, False, True) + assert wx == pytest.approx(100.0) + assert wy == pytest.approx(96.19), f"mirror_y Y wrong: {wy} (expected 96.19)" From 7e67cb91c465c77e7c5a01ebadefa8b6b5603a08 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 22:30:17 +0100 Subject: [PATCH 7/8] fix(pin_world_xy): align rotation direction and mirror axis with eeschema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs in WireDragger.pin_world_xy (and corresponding bugs in PinLocator.get_pin_angle) caused pin coordinates and angles to land on the wrong pin in 4 of 8 polarized cases (rot=90, rot=270, mirror x on a vertical part, mirror y on a vertical part). Verified end-to-end against `kicad-cli sch export netlist`. (1) Rotation direction. After PR #145's `-ly` Y-flip, calling the standard math (Y-up CCW) `_rotate` is effectively CW in screen Y-down. eeschema's TRANSFORM(0,1,-1,0) for rot=90 is screen-CCW. They agreed at 0° and 180° (where the rotation matrices coincide) but disagreed at 90° and 270°. (2) Mirror axis semantics swapped. Per eeschema symbol.h:43-44, SYM_MIRROR_X = TRANSFORM(1,0,0,-1) negates Y, and SYM_MIRROR_Y = TRANSFORM(-1,0,0,1) negates X. Our code did the inverse: `mirror_x` negated the X component and `mirror_y` negated the Y component. Fix shape for `_rotate`: chose option (b) — leave `_rotate` as standard math and negate the angle at the call site (`_rotate(lx, ly, -rotation)`). This converts math-CCW to screen-CCW without disturbing `TestRotatePoint`'s direct expectations of `_rotate`. Final composition order in `pin_world_xy` matches eeschema's parser (rotation set first into m_transform, then mirror composed via `new = old * temp` so the mirror is applied first to the coordinate): 1. Y-flip: ly = -ly (lib Y-up → screen Y-down) 2. Mirror: if mirror_x: ly = -ly (negate screen-Y) if mirror_y: lx = -lx (negate screen-X) 3. Rotate: _rotate(lx, ly, -rotation) (screen-CCW) 4. Translate: add (sym_x, sym_y) Verified by hand for {rot=90, rot=270} × {none, mirror_x, mirror_y} against the TRANSFORM matrices in transform.cpp:44 and symbol.h:43-44. `PinLocator.get_pin_angle` mirrors the same composition in angle space. For an angle, Y-flip and mirror_x both negate the angle; mirror_y maps to (180 - angle). The screen-CCW rotation in `pin_world_xy` corresponds to subtracting (not adding) the symbol rotation in standard atan2 convention — fixed accordingly. Geometry test (`test_get_pin_angle.py::test_get_pin_angle_matches_geometric_expectation`) derives expected angles from `pin_world_xy` itself, so it pins the two together. `tests/test_rotate_schematic_mirror.py::test_pin_positions_mirror_x_flips_x` encoded the OLD inverted semantics and is updated/renamed to `test_pin_positions_mirror_x_flips_y` with a pin that has non-zero Y so the assertion is meaningful under the corrected semantics. Co-Authored-By: Claude Opus 4.7 --- python/commands/pin_locator.py | 34 ++++++++++++++------------- python/commands/wire_dragger.py | 19 ++++++++++----- tests/test_rotate_schematic_mirror.py | 16 +++++++------ 3 files changed, 40 insertions(+), 29 deletions(-) diff --git a/python/commands/pin_locator.py b/python/commands/pin_locator.py index 5b1c2ae..e49289b 100644 --- a/python/commands/pin_locator.py +++ b/python/commands/pin_locator.py @@ -279,24 +279,26 @@ class PinLocator: pin_def_angle = pins[pin_number].get("angle", 0) - # Mirror flips the angle before applying symbol rotation. - # mirror_x flips the X component of local vectors → reflects across Y axis → 180 - angle. - # mirror_y flips the Y component of local vectors → reflects across X axis → negate angle. - if mirror_x: - pin_def_angle = (180 - pin_def_angle) % 360 - if mirror_y: - pin_def_angle = (-pin_def_angle) % 360 - - # Library symbols are Y-up; the schematic is Y-down. Match the - # lib→screen Y-flip applied by WireDragger.pin_world_xy (mirror in - # lib space → Y-flip → rotate → translate). For an angle this - # negates the Y component, i.e. negates the angle. Without this - # step pin angles are 180° off along the Y axis; before PR #145 - # this was masked because pin_world_xy was missing the same flip, - # so the two were "wrong in the same direction" and consistent. + # Mirror this exactly the way WireDragger.pin_world_xy does, in the + # same order: Y-flip (lib Y-up → screen Y-down) → mirror → rotate. + # + # Y-flip on an angle: negate it (reflects across X axis). pin_def_angle = (-pin_def_angle) % 360 - absolute_angle = (pin_def_angle + symbol_rotation) % 360 + # eeschema (symbol.h:43-44): + # (mirror x) = SYM_MIRROR_X = TRANSFORM(1,0,0,-1) → negates Y → + # reflect angle across X axis → -angle. + # (mirror y) = SYM_MIRROR_Y = TRANSFORM(-1,0,0,1) → negates X → + # reflect angle across Y axis → 180 - angle. + if mirror_x: + pin_def_angle = (-pin_def_angle) % 360 + if mirror_y: + pin_def_angle = (180 - pin_def_angle) % 360 + + # eeschema's rotation TRANSFORM is screen-CCW in Y-down, which is + # math-CW in standard atan2 convention — so subtract the rotation + # to match `pin_world_xy`'s `_rotate(..., -rotation)` call. + absolute_angle = (pin_def_angle - symbol_rotation) % 360 return absolute_angle except Exception: diff --git a/python/commands/wire_dragger.py b/python/commands/wire_dragger.py index aac0fde..7c8e110 100644 --- a/python/commands/wire_dragger.py +++ b/python/commands/wire_dragger.py @@ -156,15 +156,22 @@ class WireDragger: Compute the world coordinate of a pin given the symbol transform. Library pins are stored Y-up; the schematic is Y-down. Order matches - eeschema: mirror in lib space → Y-flip to screen → rotate → translate. - Without the Y-flip, polarized parts get pin 1/pin 2 silently swapped. + eeschema: Y-flip to screen → mirror → rotate (screen-CCW) → translate. + + eeschema's TRANSFORM matrix for rotation 90 is (0, 1, -1, 0) — + i.e. screen-CCW in Y-down: (x, y) → (y, -x). Our `_rotate` helper is + standard math (Y-up CCW), so we negate the rotation angle to convert. + + Mirror axis semantics match eeschema's symbol.h: + (mirror x) = SYM_MIRROR_X = TRANSFORM(1, 0, 0, -1) → negates Y. + (mirror y) = SYM_MIRROR_Y = TRANSFORM(-1, 0, 0, 1) → negates X. """ - lx, ly = px, py + lx, ly = px, -py # Y-flip: lib Y-up → screen Y-down if mirror_x: - lx = -lx + ly = -ly # SYM_MIRROR_X negates screen-Y if mirror_y: - ly = -ly - rx, ry = _rotate(lx, -ly, rotation) + lx = -lx # SYM_MIRROR_Y negates screen-X + rx, ry = _rotate(lx, ly, -rotation) # negate angle: math-CCW → screen-CCW return sym_x + rx, sym_y + ry @staticmethod diff --git a/tests/test_rotate_schematic_mirror.py b/tests/test_rotate_schematic_mirror.py index f5837ae..eb67545 100644 --- a/tests/test_rotate_schematic_mirror.py +++ b/tests/test_rotate_schematic_mirror.py @@ -163,19 +163,21 @@ def test_pin_positions_unchanged_at_same_transform(): assert old_xy == new_xy -def test_pin_positions_mirror_x_flips_x(): - """mirror_x should negate the local X coordinate before rotation.""" +def test_pin_positions_mirror_x_flips_y(): + """mirror_x = SYM_MIRROR_X = TRANSFORM(1,0,0,-1) negates the screen-Y + coordinate (eeschema symbol.h:43-44), not X. With the lib→screen Y-flip + applied first, this means the pin's screen Y is reflected back to lib Y.""" sch = _make_sch() # at (75, 105, 0), no mirror - fake_pins = {"1": {"x": 2.0, "y": 0.0}} + fake_pins = {"1": {"x": 0.0, "y": 2.0}} with patch.object(WireDragger, "get_pin_defs", return_value=fake_pins): pos = WireDragger.compute_pin_positions_for_rotation(sch, "Q1", 0.0, True, False) _, (old_xy, new_xy) = next(iter(pos.items())) - # old: pin at local (2, 0), world = (75+2, 105) = (77, 105) - assert abs(old_xy[0] - 77.0) < 1e-4 - # new: mirror_x → local (-2, 0), world = (75-2, 105) = (73, 105) - assert abs(new_xy[0] - 73.0) < 1e-4 + # old: pin at lib (0, 2). Y-flip → (0, -2). No mirror. World = (75, 105-2) = (75, 103). + assert abs(old_xy[1] - 103.0) < 1e-4 + # new: mirror_x → negate screen-Y → (0, 2). World = (75, 105+2) = (75, 107). + assert abs(new_xy[1] - 107.0) < 1e-4 # --------------------------------------------------------------------------- From 496be317e97f0015d3b4275d1d5fdfd15961b94c Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 22:40:34 +0100 Subject: [PATCH 8/8] 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])])