Fix unit test follow-ups from post-merge review

osism/tasks/conductor/sonic/connections.py

@@ -75,18 +75,24 @@ def get_connected_interfaces(
     Returns:
         tuple: (set of connected interfaces, set of connected port channels)
     """
-    connected_interfaces = set()
-    connected_portchannels = set()
+    connected_interfaces: Set[str] = set()
+    connected_portchannels: Set[str] = set()
 
     try:
         # Get all interfaces for the device (using cache)
         interfaces = get_cached_device_interfaces(device.id)
+    except Exception as e:
+        logger.warning(
+            f"Could not get interface connections for device {device.name}: {e}"
+        )
+        return connected_interfaces, connected_portchannels
 
-        for interface in interfaces:
-            # Skip management-only interfaces
-            if hasattr(interface, "mgmt_only") and interface.mgmt_only:
-                continue
+    for interface in interfaces:
+        # Skip management-only interfaces
+        if hasattr(interface, "mgmt_only") and interface.mgmt_only:
+            continue
 
+        try:
             # Check if interface is connected using connected_endpoints API
             connected_device = get_connected_device_via_interface(interface, device.id)
 
@@ -104,11 +110,12 @@ def get_connected_interfaces(
                 ):
                     pc_name = portchannel_info["member_mapping"][sonic_interface_name]
                     connected_portchannels.add(pc_name)
-
-    except Exception as e:
-        logger.warning(
-            f"Could not get interface connections for device {device.name}: {e}"
-        )
+        except Exception as e:
+            logger.warning(
+                f"Could not process interface {getattr(interface, 'name', interface)}"
+                f" on device {device.name}: {e}"
+            )
+            continue
 
     return connected_interfaces, connected_portchannels
 
@@ -290,8 +297,7 @@ def find_interconnected_devices(
                             visited.add(neighbor_id)
                             queue.append(neighbor_id)
 
-                if len(group) > 1:  # Only include groups with multiple devices
-                    all_groups.append(group)
+                all_groups.append(group)
 
     return all_groups
 

osism/tasks/conductor/sonic/interface.py

@@ -386,6 +386,12 @@ def _convert_using_port_config(
                 )
                 return result
 
+            logger.warning(
+                f"Could not extract port number from alias '{base_alias}' "
+                f"for breakout interface {sonic_interface_name}"
+            )
+            return _convert_using_speed_calculation(ethernet_num, None, is_breakout)
+
         # Fallback if base port not found
         logger.warning(
             f"Could not find base port for breakout interface {sonic_interface_name}"
@@ -445,7 +451,16 @@ def _extract_port_number_from_alias(alias):
         )
         return port_number
 
-    # Fallback to number at end of alias
+    # Handle Eth(PortN) format where no digit follows "Eth" directly
+    paren_port_match = re.search(r"\(Port(\d+)\)", alias)
+    if paren_port_match:
+        port_number = int(paren_port_match.group(1))
+        logger.debug(
+            f"Extracted port number {port_number} from Eth(Port) alias '{alias}'"
+        )
+        return port_number
+
+    # Fallback to trailing number (e.g. "hundredGigE49", "twentyFiveGigE1")
     match = re.search(r"(\d+)$", alias)
     if match:
         port_number = int(match.group(1))
@@ -993,15 +1008,6 @@ def detect_port_channels(device):
             if hasattr(interface, "lag") and interface.lag:
                 lag_parent = interface.lag
 
-                # Convert NetBox interface name to SONiC format
-                interface_speed = getattr(interface, "speed", None)
-                if (
-                    not interface_speed
-                    and hasattr(interface, "type")
-                    and interface.type
-                ):
-                    interface_speed = get_speed_from_port_type(interface.type.value)
-
                 sonic_interface_name = convert_netbox_interface_to_sonic(
                     interface, device
                 )

osism/tasks/conductor/utils.py

@@ -26,7 +26,7 @@ def deep_compare(a, b, updates):
                 updates[key] = value
         else:
             updates[key] = {}
-            deep_compare(a[key], b[key], updates[key])
+            deep_compare(a[key], b.get(key, {}), updates[key])
             if not updates[key]:
                 updates.pop(key)
 

tests/unit/data/test_enums.py

@@ -254,6 +254,18 @@ def test_map_role2role_collection_openstack_core_includes_core_services():
     assert {"keystone", "neutron", "nova", "glance", "cinder", "placement"} <= names
 
 
+def test_map_role2role_collection_openstack_core_chain_keystone_to_octavia():
+    roots = MAP_ROLE2ROLE["collection-openstack-core"]
+    keystone = find_role(roots, "keystone")
+    assert keystone is not None and isinstance(keystone, Role)
+    neutron = find_role(keystone.dependencies, "neutron")
+    assert neutron is not None and isinstance(neutron, Role)
+    wait_for_nova = find_role(neutron.dependencies, "wait-for-nova")
+    assert wait_for_nova is not None and isinstance(wait_for_nova, Role)
+    octavia = find_role(wait_for_nova.dependencies, "octavia")
+    assert octavia is not None and isinstance(octavia, Role)
+
+
 def test_map_role2role_collection_monitoring_grafana_depends_on_prometheus():
     prometheus = find_role(MAP_ROLE2ROLE["collection-monitoring"], "prometheus")
 

tests/unit/tasks/conductor/sonic/test_cache.py

@@ -5,6 +5,7 @@
 
 import pytest
 
+import osism.tasks.conductor.sonic.cache as _cache_mod
 from osism.tasks.conductor.sonic.cache import (
     InterfaceCache,
     clear_interface_cache,
@@ -43,6 +44,7 @@ def _reset_thread_local():
     """
     yield
     clear_interface_cache()
+    _cache_mod._thread_local.__dict__.pop("interface_cache", None)
 
 
 # ---------------------------------------------------------------------------
@@ -294,6 +296,34 @@ def worker():
     assert mock_nb.dcim.interfaces.filter.call_count == 1
 
 
+def test_get_device_interfaces_acquires_lock_once(mock_nb):
+    mock_nb.dcim.interfaces.filter.return_value = []
+    cache = InterfaceCache()
+    cache._lock = MagicMock(wraps=threading.Lock())
+
+    cache.get_device_interfaces(1)
+
+    assert cache._lock.__enter__.call_count == 1
+
+
+def test_clear_acquires_lock_once():
+    cache = InterfaceCache()
+    cache._lock = MagicMock(wraps=threading.Lock())
+
+    cache.clear()
+
+    assert cache._lock.__enter__.call_count == 1
+
+
+def test_get_cache_stats_acquires_lock_once():
+    cache = InterfaceCache()
+    cache._lock = MagicMock(wraps=threading.Lock())
+
+    cache.get_cache_stats()
+
+    assert cache._lock.__enter__.call_count == 1
+
+
 # ---------------------------------------------------------------------------
 # get_interface_cache (module-level, thread-local)
 # ---------------------------------------------------------------------------

tests/unit/tasks/conductor/sonic/test_connections.py

@@ -239,6 +239,33 @@ def test_get_connected_interfaces_helper_exception_logs_warning(mocker, make_int
     assert "sw1" in warning.call_args.args[0]
 
 
+def test_get_connected_interfaces_continues_after_per_interface_exception(
+    mocker, make_interface
+):
+    """An exception on one interface must not abort processing of subsequent ones."""
+    device = SimpleNamespace(id=1, name="sw1")
+    bad_iface = make_interface(name="Ethernet0")
+    good_iface = make_interface(name="Ethernet4")
+    peer = SimpleNamespace(id=2, name="peer")
+
+    mocker.patch(
+        "osism.tasks.conductor.sonic.connections.get_cached_device_interfaces",
+        return_value=[bad_iface, good_iface],
+    )
+    mocker.patch(
+        "osism.tasks.conductor.sonic.connections.get_connected_device_via_interface",
+        side_effect=[RuntimeError("boom"), peer],
+    )
+    mocker.patch(
+        "osism.tasks.conductor.sonic.connections.convert_netbox_interface_to_sonic",
+        return_value="Ethernet4",
+    )
+
+    connected, _ = connections.get_connected_interfaces(device)
+
+    assert connected == {"Ethernet4"}
+
+
 def test_get_connected_interfaces_cache_lookup_failure_returns_empty(mocker):
     device = SimpleNamespace(id=1, name="sw1")
     mocker.patch(
@@ -509,8 +536,8 @@ def test_find_interconnected_devices_single_spine_no_peers(make_device, wire_top
     spine = make_device(1, role_slug="spine")
     wire_topology(device_interfaces={1: []}, connections_map={})
 
-    # Single device with no peers — the role_graphs entry is empty and
-    # groups of size 1 are dropped.
+    # Single device with no in-role peers — the device never enters
+    # role_graphs, so the BFS has nothing to walk and returns no groups.
     assert connections.find_interconnected_devices([spine]) == []
 
 

tests/unit/tasks/conductor/sonic/test_interface_conversion.py

@@ -46,6 +46,10 @@ def _reset_port_config_cache():
     clear_port_config_cache()
 
 
+def _has_log(records, level, substring):
+    return any(r["level"] == level and substring in r["message"] for r in records)
+
+
 def _make_device(hwsku="Accton-AS7326-56X", device_id=1, name="sw-1"):
     """Build a minimal NetBox-shaped device with sonic_parameters."""
     custom_fields = (
@@ -105,24 +109,27 @@ def test_convert_netbox_interface_already_sonic_returns_unchanged():
     assert convert_netbox_interface_to_sonic(iface, device) == "Ethernet4"
 
 
-def test_convert_netbox_interface_string_without_device_returns_input():
+def test_convert_netbox_interface_string_without_device_returns_input(loguru_logs):
     assert convert_netbox_interface_to_sonic("Eth1/1", device=None) == "Eth1/1"
+    assert _has_log(loguru_logs, "WARNING", "Device object required")
 
 
-def test_convert_netbox_interface_object_without_device_returns_name():
+def test_convert_netbox_interface_object_without_device_returns_name(loguru_logs):
     iface = _make_iface("Eth1/1")
 
     assert convert_netbox_interface_to_sonic(iface, device=None) == "Eth1/1"
+    assert _has_log(loguru_logs, "WARNING", "Device object required")
 
 
-def test_convert_netbox_interface_device_without_hwsku_returns_input():
+def test_convert_netbox_interface_device_without_hwsku_returns_input(loguru_logs):
     device = _make_device(hwsku=None)  # sonic_parameters dict, no "hwsku" key
     iface = _make_iface("Eth1/1")
 
     assert convert_netbox_interface_to_sonic(iface, device) == "Eth1/1"
+    assert _has_log(loguru_logs, "WARNING", "No HWSKU found")
 
 
-def test_convert_netbox_interface_cache_failure_returns_input(mocker):
+def test_convert_netbox_interface_cache_failure_returns_input(mocker, loguru_logs):
     mocker.patch(
         "osism.tasks.conductor.sonic.interface.get_cached_device_interfaces",
         side_effect=RuntimeError("netbox down"),
@@ -131,9 +138,10 @@ def test_convert_netbox_interface_cache_failure_returns_input(mocker):
     iface = _make_iface("Eth1/1")
 
     assert convert_netbox_interface_to_sonic(iface, device) == "Eth1/1"
+    assert _has_log(loguru_logs, "WARNING", "Could not fetch device interfaces")
 
 
-def test_convert_netbox_interface_empty_port_config_returns_input(mocker):
+def test_convert_netbox_interface_empty_port_config_returns_input(mocker, loguru_logs):
     mocker.patch(
         "osism.tasks.conductor.sonic.interface.get_cached_device_interfaces",
         return_value=[_make_iface("Eth1/1")],
@@ -145,9 +153,10 @@ def test_convert_netbox_interface_empty_port_config_returns_input(mocker):
     device = _make_device()
 
     assert convert_netbox_interface_to_sonic(_make_iface("Eth1/1"), device) == "Eth1/1"
+    assert _has_log(loguru_logs, "WARNING", "No port config found")
 
 
-def test_convert_netbox_interface_port_config_raises_returns_input(mocker):
+def test_convert_netbox_interface_port_config_raises_returns_input(mocker, loguru_logs):
     mocker.patch(
         "osism.tasks.conductor.sonic.interface.get_cached_device_interfaces",
         return_value=[_make_iface("Eth1/1")],
@@ -159,6 +168,7 @@ def test_convert_netbox_interface_port_config_raises_returns_input(mocker):
     device = _make_device()
 
     assert convert_netbox_interface_to_sonic(_make_iface("Eth1/1"), device) == "Eth1/1"
+    assert _has_log(loguru_logs, "WARNING", "Could not load port config")
 
 
 def test_convert_netbox_interface_standard_form_resolves_via_alias(mocker):
@@ -516,6 +526,22 @@ def test_convert_using_port_config_breakout_no_base_port_falls_back():
     assert _convert_using_port_config("Ethernet5", 5, True, {}) == "Eth1/2/2"
 
 
+def test_convert_using_port_config_breakout_base_port_found_but_alias_has_no_number(
+    loguru_logs,
+):
+    # is_breakout=True; base port Ethernet4 IS found in port_config, but its
+    # alias "no-digits" yields no extractable number.  The code should log a
+    # warning and fall back to speed-based calculation.
+    # _convert_using_speed_calculation(5, None, True):
+    #   base_port=4, subport=2, physical_port=2 → "Eth1/2/2"
+    port_config = {"Ethernet4": {"alias": "no-digits"}}
+
+    assert _convert_using_port_config("Ethernet5", 5, True, port_config) == "Eth1/2/2"
+    assert _has_log(
+        loguru_logs, "WARNING", "Could not extract port number from alias 'no-digits'"
+    )
+
+
 def test_convert_using_port_config_regular_missing_alias_falls_back():
     # Regular path with port present but alias unparseable → legacy calc.
     # Ethernet4 with speed=None falls to multiplier 1 → Eth1/5.
@@ -574,10 +600,17 @@ def test_extract_port_number_no_digits_returns_none():
     assert _extract_port_number_from_alias("foo") is None
 
 
-def test_extract_port_number_paren_alias_without_leading_digit_returns_none():
-    # "Eth(Port5)" matches neither regex (no \d+ before "(", trailing char
-    # is ")", so $-anchored digit search fails). Documents actual behavior.
-    assert _extract_port_number_from_alias("Eth(Port5)") is None
+def test_extract_port_number_paren_alias_without_leading_digit():
+    # "Eth(Port5)" has no digit immediately after "Eth", so the primary
+    # Eth\d+(Port\d+) regex does not match.  The explicit \(Port(\d+)\)
+    # pattern handles it next and returns 5.
+    assert _extract_port_number_from_alias("Eth(Port5)") == 5
+
+
+def test_extract_port_number_alias_with_prefix_digits_uses_trailing():
+    # "QSFP28-49": the \(Port(\d+)\) pattern does not match, and the
+    # trailing-number fallback correctly returns 49 rather than 28.
+    assert _extract_port_number_from_alias("QSFP28-49") == 49
 
 
 # ---------------------------------------------------------------------------

tests/unit/tasks/conductor/test_utils.py

@@ -99,6 +99,16 @@ def test_deep_compare_records_list_value_difference():
     assert updates == {"items": [1, 2, 3]}
 
 
+def test_deep_compare_nested_key_missing_from_b_records_whole_subtree():
+    a = {"outer": {"inner": "val", "other": 1}}
+    b = {}
+    updates = {}
+
+    deep_compare(a, b, updates)
+
+    assert updates == {"outer": {"inner": "val", "other": 1}}
+
+
 # ---------------------------------------------------------------------------
 # deep_merge
 # ---------------------------------------------------------------------------