Re: [PATCH v3 2/5] dts: rework traffic generator inheritance structure

[Luca Vizzarro]

On 02/07/2025 06:21, Patrick Robb wrote:
+++ 
b/dts/framework/testbed_model/traffic_generator/performance_traffic_generator.py
@@ -0,0 +1,63 @@
<snip>
+
+@dataclass(slots=True)
+class PerformanceTrafficStats(ABC):

Why is this inheriting ABC? I don't see why a dataclass would be 
abstract given it's even missing abstract methods.

<snip>
+class PerformanceTrafficGenerator(TrafficGenerator):
+    """An abstract base class for all performance-oriented traffic generators.
+
+    Provides an intermediary interface for performance-based traffic generator.
+    """
+
+    @abstractmethod
+    def calculate_traffic_and_stats(
+        self,
+        packet: Packet,
+        send_mpps: int,
+        duration: float,
+    ) -> PerformanceTrafficStats:
+        """Send packet traffic and acquire associated statistics.
+
+        Args:
+        packet: The packet to send.
+        send_mpps: The millions packets per second send rate.
+        duration: Performance test duration (in seconds).

bad indentation for args.

diff --git a/dts/framework/testbed_model/traffic_generator/scapy.py 
b/dts/framework/testbed_model/traffic_generator/scapy.py
index e21ba4ed96..602b93d473 100644
--- a/dts/framework/testbed_model/traffic_generator/scapy.py
+++ b/dts/framework/testbed_model/traffic_generator/scapy.py
@@ -26,7 +26,7 @@
  from scapy.packet import Packet
  
  from framework.config.node import OS
-from framework.config.test_run import ScapyTrafficGeneratorConfig
+from framework.config.test_run import TrafficGeneratorConfig

Making the Scapy class use a more generic config class looks like 
breaking behaviour. Why is this happening?

@@ -332,6 +332,10 @@ def setup(self, topology: Topology):
          self._shell.send_command("from scapy.all import *")
          self._shell.send_command("from scapy.contrib.lldp import *")
  
+    def teardown(self):
+        """Overrides :meth:`.traffic_generator.TrafficGenerator.teardown`."""
+        pass

nit: `pass` is not needed here, because the docstring already completes 
the method as noop.

<snip>
diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py 
b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
index 8f53b07daf..ea3075989d 100644
--- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py
+++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
@@ -10,14 +10,10 @@
<snip>
  
  class TrafficGenerator(ABC):
@@ -54,46 +50,6 @@ def setup(self, topology: Topology):
  
      def teardown(self):
          """Teardown the traffic generator."""
-        self.close()

how come are we removing the default close behaviour? I don't see it 
being replaced appropriately.

<snip>
-    @property
-    def is_capturing(self) -> bool:
-        """This traffic generator can't capture traffic."""
-        return False
Why is the above property being removed?
  
      @abstractmethod
      def close(self) -> None: