From 22eb3319f97dd8320e88bd0faa45bf30b74e1993 Mon Sep 17 00:00:00 2001 From: Eugene Mikhantyev Date: Sun, 3 May 2026 21:53:21 +0100 Subject: [PATCH] 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