From ae6e835955136c815eb496efa2bcb2f4329b0af6 Mon Sep 17 00:00:00 2001 From: Marek Wolan Date: Fri, 25 Aug 2023 15:58:07 +0100 Subject: [PATCH 1/2] Apply suggestions from code review. --- src/primaite/simulator/network/container.py | 35 +++++++++---------- .../simulator/network/hardware/base.py | 2 +- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/primaite/simulator/network/container.py b/src/primaite/simulator/network/container.py index 1c03358c..85676034 100644 --- a/src/primaite/simulator/network/container.py +++ b/src/primaite/simulator/network/container.py @@ -52,11 +52,11 @@ class Network(SimComponent): :type node: Node """ if node in self: - msg = f"Can't add node {node}. It is already in the network." - _LOGGER.warning(msg) - raise RuntimeWarning(msg) + _LOGGER.warning(f"Can't add node {node.uuid}. It is already in the network.") + return self.nodes[node.uuid] = node node.parent = self + _LOGGER.info(f"Added node {node.uuid} to Network {self.uuid}") def remove_node(self, node: Node) -> None: """ @@ -66,11 +66,11 @@ class Network(SimComponent): :type node: Node """ if node not in self: - msg = f"Can't remove node {node}. It's not in the network." - _LOGGER.warning(msg) - raise RuntimeWarning(msg) - del self.nodes[node.uuid] - del node.parent # misleading? + _LOGGER.warning(f"Can't remove node {node.uuid}. It's not in the network.") + return + self.nodes.pop(node.uuid) + node.parent = None + _LOGGER.info(f"Removed node {node.uuid} from network {self.uuid}") def connect(self, endpoint_a: Union[NIC, SwitchPort], endpoint_b: Union[NIC, SwitchPort], **kwargs) -> None: """Connect two nodes on the network by creating a link between an NIC/SwitchPort of each one. @@ -83,20 +83,18 @@ class Network(SimComponent): """ node_a = endpoint_a.parent node_b = endpoint_b.parent - msg = "" if node_a not in self: - msg = f"Cannot create a link to {endpoint_a} because the node is not in the network." + self.add_node(node_a) if node_b not in self: - msg = f"Cannot create a link to {endpoint_b} because the node is not in the network." + self.add_node(node_b) if node_a is node_b: - msg = f"Cannot link {endpoint_a} to {endpoint_b} because they belong to the same node." - if msg: - _LOGGER.error(msg) - raise RuntimeError(msg) + _LOGGER.warn(f"Cannot link endpoint {endpoint_a} to {endpoint_b} because they belong to the same node.") + return link = Link(endpoint_a=endpoint_a, endpoint_b=endpoint_b, **kwargs) self.links[link.uuid] = link link.parent = self + _LOGGER.info(f"Added link {link.uuid} to connect {endpoint_a} and {endpoint_b}") def remove_link(self, link: Link) -> None: """Disconnect a link from the network. @@ -106,12 +104,13 @@ class Network(SimComponent): """ link.endpoint_a.disconnect_link() link.endpoint_b.disconnect_link() - del self.links[link.uuid] - del link.parent + self.links.pop(link.uuid) + link.parent = None + _LOGGER.info(f"Removed link {link.uuid} from network {self.uuid}.") def __contains__(self, item: Any) -> bool: if isinstance(item, Node): return item.uuid in self.nodes elif isinstance(item, Link): return item.uuid in self.links - raise TypeError("") + return False diff --git a/src/primaite/simulator/network/hardware/base.py b/src/primaite/simulator/network/hardware/base.py index fe3b5b15..9acdf0b4 100644 --- a/src/primaite/simulator/network/hardware/base.py +++ b/src/primaite/simulator/network/hardware/base.py @@ -939,7 +939,7 @@ class Node(SimComponent): nic = self.nics.get(nic) if nic or nic.uuid in self.nics: self.nics.pop(nic.uuid) - del nic.parent + nic.parent = None nic.disable() self.sys_log.info(f"Disconnected NIC {nic}") else: From 6e602aa1514b92dc34ec7324d96bdd58fc72efdb Mon Sep 17 00:00:00 2001 From: Marek Wolan Date: Fri, 25 Aug 2023 17:56:05 +0100 Subject: [PATCH 2/2] Fix unit tests by removing warning checks --- src/primaite/simulator/core.py | 8 ++--- .../network/test_network_creation.py | 29 ++++++++++++++----- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/primaite/simulator/core.py b/src/primaite/simulator/core.py index 63120ecf..b7dfcf72 100644 --- a/src/primaite/simulator/core.py +++ b/src/primaite/simulator/core.py @@ -1,6 +1,6 @@ """Core of the PrimAITE Simulator.""" from abc import ABC, abstractmethod -from typing import Callable, Dict, List, Optional +from typing import Callable, Dict, List, Optional, Union from uuid import uuid4 from pydantic import BaseModel, ConfigDict, Extra @@ -199,9 +199,9 @@ class SimComponent(BaseModel): return self._parent @parent.setter - def parent(self, new_parent: "SimComponent") -> None: - if self._parent: - msg = f"Overwriting parent of {self}, {self._parent} with {new_parent}" + def parent(self, new_parent: Union["SimComponent", None]) -> None: + if self._parent and new_parent: + msg = f"Overwriting parent of {self.uuid}. Old parent: {self._parent.uuid}, New parent: {new_parent.uuid}" _LOGGER.warn(msg) raise RuntimeWarning(msg) self._parent = new_parent diff --git a/tests/integration_tests/network/test_network_creation.py b/tests/integration_tests/network/test_network_creation.py index 418f5e5f..356eb1db 100644 --- a/tests/integration_tests/network/test_network_creation.py +++ b/tests/integration_tests/network/test_network_creation.py @@ -22,8 +22,7 @@ def test_readding_node(): net = Network() n1 = Node(hostname="computer") net.add_node(n1) - with pytest.raises(RuntimeWarning): - net.add_node(n1) + net.add_node(n1) assert n1.parent is net assert n1 in net @@ -32,8 +31,7 @@ def test_removing_nonexistent_node(): """Check that warning is raised when trying to remove a node that is not in the network.""" net = Network() n1 = Node(hostname="computer") - with pytest.raises(RuntimeWarning): - net.remove_node(n1) + net.remove_node(n1) assert n1.parent is None assert n1 not in net @@ -69,8 +67,7 @@ def test_connecting_node_to_itself(): net.add_node(node) - with pytest.raises(RuntimeError): - net.connect(node.nics[nic1.uuid], node.nics[nic2.uuid], bandwidth=30) + net.connect(node.nics[nic1.uuid], node.nics[nic2.uuid], bandwidth=30) assert node in net assert nic1.connected_link is None @@ -79,4 +76,22 @@ def test_connecting_node_to_itself(): def test_disconnecting_nodes(): - ... + net = Network() + + n1 = Node(hostname="computer") + n1_nic = NIC(ip_address="120.30.0.1", gateway="192.168.0.1", subnet_mask="255.255.255.0") + n1.connect_nic(n1_nic) + net.add_node(n1) + + n2 = Node(hostname="server") + n2_nic = NIC(ip_address="120.30.0.2", gateway="192.168.0.1", subnet_mask="255.255.255.0") + n2.connect_nic(n2_nic) + net.add_node(n2) + + net.connect(n1.nics[n1_nic.uuid], n2.nics[n2_nic.uuid], bandwidth=30) + assert len(net.links) == 1 + + link = list(net.links.values())[0] + net.remove_link(link) + assert link not in net + assert len(net.links) == 0