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

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

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: