[v7,19/21] dts: base traffic generators docstring update

  Format according to the Google format and PEP257, with slight
deviations.

Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
 .../traffic_generator/__init__.py             | 22 ++++++++-
 .../capturing_traffic_generator.py            | 46 +++++++++++--------
 .../traffic_generator/traffic_generator.py    | 33 +++++++------
 3 files changed, 68 insertions(+), 33 deletions(-)
  

On 11/15/23 13:09, Juraj Linkeš wrote:
> Format according to the Google format and PEP257, with slight
> deviations.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>   .../traffic_generator/__init__.py             | 22 ++++++++-
>   .../capturing_traffic_generator.py            | 46 +++++++++++--------
>   .../traffic_generator/traffic_generator.py    | 33 +++++++------
>   3 files changed, 68 insertions(+), 33 deletions(-)
> 
> diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py b/dts/framework/testbed_model/traffic_generator/__init__.py
> index 11bfa1ee0f..51cca77da4 100644
> --- a/dts/framework/testbed_model/traffic_generator/__init__.py
> +++ b/dts/framework/testbed_model/traffic_generator/__init__.py
> @@ -1,6 +1,19 @@
>   # SPDX-License-Identifier: BSD-3-Clause
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   
> +"""DTS traffic generators.
> +
> +A traffic generator is capable of generating traffic and then monitor returning traffic.
> +A traffic generator may just count the number of received packets
> +and it may additionally capture individual packets.

The sentence feels odd. Isn't it supposed to be "or" here? and no need 
for that early of a line break

> +
> +A traffic generator may be software running on generic hardware or it could be specialized hardware.
> +
> +The traffic generators that only count the number of received packets are suitable only for
> +performance testing. In functional testing, we need to be able to dissect each arrived packet
> +and a capturing traffic generator is required.
> +"""
> +
>   from framework.config import ScapyTrafficGeneratorConfig, TrafficGeneratorType
>   from framework.exception import ConfigurationError
>   from framework.testbed_model.node import Node
> @@ -12,8 +25,15 @@
>   def create_traffic_generator(
>       tg_node: Node, traffic_generator_config: ScapyTrafficGeneratorConfig
>   ) -> CapturingTrafficGenerator:
> -    """A factory function for creating traffic generator object from user config."""
> +    """The factory function for creating traffic generator objects from the test run configuration.
> +
> +    Args:
> +        tg_node: The traffic generator node where the created traffic generator will be running.
> +        traffic_generator_config: The traffic generator config.
>   
> +    Returns:
> +        A traffic generator capable of capturing received packets.
> +    """
>       match traffic_generator_config.traffic_generator_type:
>           case TrafficGeneratorType.SCAPY:
>               return ScapyTrafficGenerator(tg_node, traffic_generator_config)
> diff --git a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> index e521211ef0..b0a43ad003 100644
> --- a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> +++ b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> @@ -23,19 +23,22 @@
>   
>   
>   def _get_default_capture_name() -> str:
> -    """
> -    This is the function used for the default implementation of capture names.
> -    """
>       return str(uuid.uuid4())
>   
>   
>   class CapturingTrafficGenerator(TrafficGenerator):
>       """Capture packets after sending traffic.
>   
> -    A mixin interface which enables a packet generator to declare that it can capture
> +    The intermediary interface which enables a packet generator to declare that it can capture
>       packets and return them to the user.
>   
> +    Similarly to
> +    :class:`~framework.testbed_model.traffic_generator.traffic_generator.TrafficGenerator`,
> +    this class exposes the public methods specific to capturing traffic generators and defines
> +    a private method that must implement the traffic generation and capturing logic in subclasses.
> +
>       The methods of capturing traffic generators obey the following workflow:
> +
>           1. send packets
>           2. capture packets
>           3. write the capture to a .pcap file
> @@ -44,6 +47,7 @@ class CapturingTrafficGenerator(TrafficGenerator):
>   
>       @property
>       def is_capturing(self) -> bool:
> +        """This traffic generator can capture traffic."""
>           return True
>   
>       def send_packet_and_capture(
> @@ -54,11 +58,12 @@ def send_packet_and_capture(
>           duration: float,
>           capture_name: str = _get_default_capture_name(),
>       ) -> list[Packet]:
> -        """Send a packet, return received traffic.
> +        """Send `packet` and capture received traffic.
> +
> +        Send `packet` on `send_port` and then return all traffic captured
> +        on `receive_port` for the given `duration`.
>   
> -        Send a packet on the send_port and then return all traffic captured
> -        on the receive_port for the given duration. Also record the captured traffic
> -        in a pcap file.
> +        The captured traffic is recorded in the `capture_name`.pcap file.
>   
>           Args:
>               packet: The packet to send.
> @@ -68,7 +73,7 @@ def send_packet_and_capture(
>               capture_name: The name of the .pcap file where to store the capture.
>   
>           Returns:
> -             A list of received packets. May be empty if no packets are captured.
> +             The received packets. May be empty if no packets are captured.
>           """
>           return self.send_packets_and_capture(
>               [packet], send_port, receive_port, duration, capture_name
> @@ -82,11 +87,14 @@ def send_packets_and_capture(
>           duration: float,
>           capture_name: str = _get_default_capture_name(),
>       ) -> list[Packet]:
> -        """Send packets, return received traffic.
> +        """Send `packets` and capture received traffic.
>   
> -        Send packets on the send_port and then return all traffic captured
> -        on the receive_port for the given duration. Also record the captured traffic
> -        in a pcap file.
> +        Send `packets` on `send_port` and then return all traffic captured
> +        on `receive_port` for the given `duration`.
> +
> +        The captured traffic is recorded in the `capture_name`.pcap file. The target directory
> +        can be configured with the :option:`--output-dir` command line argument or
> +        the :envvar:`DTS_OUTPUT_DIR` environment variable.
>   
>           Args:
>               packets: The packets to send.
> @@ -96,7 +104,7 @@ def send_packets_and_capture(
>               capture_name: The name of the .pcap file where to store the capture.
>   
>           Returns:
> -             A list of received packets. May be empty if no packets are captured.
> +             The received packets. May be empty if no packets are captured.
>           """
>           self._logger.debug(get_packet_summaries(packets))
>           self._logger.debug(
> @@ -124,10 +132,12 @@ def _send_packets_and_capture(
>           receive_port: Port,
>           duration: float,
>       ) -> list[Packet]:
> -        """
> -        The extended classes must implement this method which
> -        sends packets on send_port and receives packets on the receive_port
> -        for the specified duration. It must be able to handle no received packets.
> +        """The implementation of :method:`send_packets_and_capture`.
> +
> +        The subclasses must implement this method which sends `packets` on `send_port`
> +        and receives packets on `receive_port` for the specified `duration`.
> +
> +        It must be able to handle no received packets.

This sentence feels odd too. Maybe "It must be able to handle receiving 
no packets."

>           """
>   
>       def _write_capture_from_packets(
> diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> index ea7c3963da..ed396c6a2f 100644
> --- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> @@ -22,7 +22,8 @@
>   class TrafficGenerator(ABC):
>       """The base traffic generator.
>   
> -    Defines the few basic methods that each traffic generator must implement.
> +    Exposes the common public methods of all traffic generators and defines private methods
> +    that must implement the traffic generation logic in subclasses.
>       """
>   
>       _config: TrafficGeneratorConfig
> @@ -30,6 +31,12 @@ class TrafficGenerator(ABC):
>       _logger: DTSLOG
>   
>       def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
> +        """Initialize the traffic generator.
> +
> +        Args:
> +            tg_node: The traffic generator node where the created traffic generator will be running.
> +            config: The traffic generator's test run configuration.
> +        """
>           self._config = config
>           self._tg_node = tg_node
>           self._logger = getLogger(
> @@ -37,9 +44,9 @@ def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
>           )
>   
>       def send_packet(self, packet: Packet, port: Port) -> None:
> -        """Send a packet and block until it is fully sent.
> +        """Send `packet` and block until it is fully sent.
>   
> -        What fully sent means is defined by the traffic generator.
> +        Send `packet` on `port`, then wait until `packet` is fully sent.
>   
>           Args:
>               packet: The packet to send.
> @@ -48,9 +55,9 @@ def send_packet(self, packet: Packet, port: Port) -> None:
>           self.send_packets([packet], port)
>   
>       def send_packets(self, packets: list[Packet], port: Port) -> None:
> -        """Send packets and block until they are fully sent.
> +        """Send `packets` and block until they are fully sent.
>   
> -        What fully sent means is defined by the traffic generator.
> +        Send `packets` on `port`, then wait until `packets` are fully sent.
>   
>           Args:
>               packets: The packets to send.
> @@ -62,19 +69,17 @@ def send_packets(self, packets: list[Packet], port: Port) -> None:
>   
>       @abstractmethod
>       def _send_packets(self, packets: list[Packet], port: Port) -> None:
> -        """
> -        The extended classes must implement this method which
> -        sends packets on send_port. The method should block until all packets
> -        are fully sent.
> +        """The implementation of :method:`send_packets`.
> +
> +        The subclasses must implement this method which sends `packets` on `port`.
> +        The method should block until all `packets` are fully sent.
> +
> +        What full sent means is defined by the traffic generator.

full -> fully

>           """
>   
>       @property
>       def is_capturing(self) -> bool:
> -        """Whether this traffic generator can capture traffic.
> -
> -        Returns:
> -            True if the traffic generator can capture traffic, False otherwise.
> -        """
> +        """This traffic generator can't capture traffic."""
>           return False
>   
>       @abstractmethod
  

On Tue, Nov 21, 2023 at 5:20 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>
> On 11/15/23 13:09, Juraj Linkeš wrote:
> > Format according to the Google format and PEP257, with slight
> > deviations.
> >
> > Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> > ---
> >   .../traffic_generator/__init__.py             | 22 ++++++++-
> >   .../capturing_traffic_generator.py            | 46 +++++++++++--------
> >   .../traffic_generator/traffic_generator.py    | 33 +++++++------
> >   3 files changed, 68 insertions(+), 33 deletions(-)
> >
> > diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py b/dts/framework/testbed_model/traffic_generator/__init__.py
> > index 11bfa1ee0f..51cca77da4 100644
> > --- a/dts/framework/testbed_model/traffic_generator/__init__.py
> > +++ b/dts/framework/testbed_model/traffic_generator/__init__.py
> > @@ -1,6 +1,19 @@
> >   # SPDX-License-Identifier: BSD-3-Clause
> >   # Copyright(c) 2023 PANTHEON.tech s.r.o.
> >
> > +"""DTS traffic generators.
> > +
> > +A traffic generator is capable of generating traffic and then monitor returning traffic.
> > +A traffic generator may just count the number of received packets
> > +and it may additionally capture individual packets.
>
> The sentence feels odd. Isn't it supposed to be "or" here? and no need
> for that early of a line break
>

There are two mays, so there probably should be an or. But I'd like to
reword it to this:

All traffic generators count the number of received packets, and they
may additionally
capture individual packets.

What do you think?

> > +
> > +A traffic generator may be software running on generic hardware or it could be specialized hardware.
> > +
> > +The traffic generators that only count the number of received packets are suitable only for
> > +performance testing. In functional testing, we need to be able to dissect each arrived packet
> > +and a capturing traffic generator is required.
> > +"""
> > +
> >   from framework.config import ScapyTrafficGeneratorConfig, TrafficGeneratorType
> >   from framework.exception import ConfigurationError
> >   from framework.testbed_model.node import Node
> > @@ -12,8 +25,15 @@
> >   def create_traffic_generator(
> >       tg_node: Node, traffic_generator_config: ScapyTrafficGeneratorConfig
> >   ) -> CapturingTrafficGenerator:
> > -    """A factory function for creating traffic generator object from user config."""
> > +    """The factory function for creating traffic generator objects from the test run configuration.
> > +
> > +    Args:
> > +        tg_node: The traffic generator node where the created traffic generator will be running.
> > +        traffic_generator_config: The traffic generator config.
> >
> > +    Returns:
> > +        A traffic generator capable of capturing received packets.
> > +    """
> >       match traffic_generator_config.traffic_generator_type:
> >           case TrafficGeneratorType.SCAPY:
> >               return ScapyTrafficGenerator(tg_node, traffic_generator_config)
> > diff --git a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> > index e521211ef0..b0a43ad003 100644
> > --- a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> > +++ b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> > @@ -23,19 +23,22 @@
> >
> >
> >   def _get_default_capture_name() -> str:
> > -    """
> > -    This is the function used for the default implementation of capture names.
> > -    """
> >       return str(uuid.uuid4())
> >
> >
> >   class CapturingTrafficGenerator(TrafficGenerator):
> >       """Capture packets after sending traffic.
> >
> > -    A mixin interface which enables a packet generator to declare that it can capture
> > +    The intermediary interface which enables a packet generator to declare that it can capture
> >       packets and return them to the user.
> >
> > +    Similarly to
> > +    :class:`~framework.testbed_model.traffic_generator.traffic_generator.TrafficGenerator`,
> > +    this class exposes the public methods specific to capturing traffic generators and defines
> > +    a private method that must implement the traffic generation and capturing logic in subclasses.
> > +
> >       The methods of capturing traffic generators obey the following workflow:
> > +
> >           1. send packets
> >           2. capture packets
> >           3. write the capture to a .pcap file
> > @@ -44,6 +47,7 @@ class CapturingTrafficGenerator(TrafficGenerator):
> >
> >       @property
> >       def is_capturing(self) -> bool:
> > +        """This traffic generator can capture traffic."""
> >           return True
> >
> >       def send_packet_and_capture(
> > @@ -54,11 +58,12 @@ def send_packet_and_capture(
> >           duration: float,
> >           capture_name: str = _get_default_capture_name(),
> >       ) -> list[Packet]:
> > -        """Send a packet, return received traffic.
> > +        """Send `packet` and capture received traffic.
> > +
> > +        Send `packet` on `send_port` and then return all traffic captured
> > +        on `receive_port` for the given `duration`.
> >
> > -        Send a packet on the send_port and then return all traffic captured
> > -        on the receive_port for the given duration. Also record the captured traffic
> > -        in a pcap file.
> > +        The captured traffic is recorded in the `capture_name`.pcap file.
> >
> >           Args:
> >               packet: The packet to send.
> > @@ -68,7 +73,7 @@ def send_packet_and_capture(
> >               capture_name: The name of the .pcap file where to store the capture.
> >
> >           Returns:
> > -             A list of received packets. May be empty if no packets are captured.
> > +             The received packets. May be empty if no packets are captured.
> >           """
> >           return self.send_packets_and_capture(
> >               [packet], send_port, receive_port, duration, capture_name
> > @@ -82,11 +87,14 @@ def send_packets_and_capture(
> >           duration: float,
> >           capture_name: str = _get_default_capture_name(),
> >       ) -> list[Packet]:
> > -        """Send packets, return received traffic.
> > +        """Send `packets` and capture received traffic.
> >
> > -        Send packets on the send_port and then return all traffic captured
> > -        on the receive_port for the given duration. Also record the captured traffic
> > -        in a pcap file.
> > +        Send `packets` on `send_port` and then return all traffic captured
> > +        on `receive_port` for the given `duration`.
> > +
> > +        The captured traffic is recorded in the `capture_name`.pcap file. The target directory
> > +        can be configured with the :option:`--output-dir` command line argument or
> > +        the :envvar:`DTS_OUTPUT_DIR` environment variable.
> >
> >           Args:
> >               packets: The packets to send.
> > @@ -96,7 +104,7 @@ def send_packets_and_capture(
> >               capture_name: The name of the .pcap file where to store the capture.
> >
> >           Returns:
> > -             A list of received packets. May be empty if no packets are captured.
> > +             The received packets. May be empty if no packets are captured.
> >           """
> >           self._logger.debug(get_packet_summaries(packets))
> >           self._logger.debug(
> > @@ -124,10 +132,12 @@ def _send_packets_and_capture(
> >           receive_port: Port,
> >           duration: float,
> >       ) -> list[Packet]:
> > -        """
> > -        The extended classes must implement this method which
> > -        sends packets on send_port and receives packets on the receive_port
> > -        for the specified duration. It must be able to handle no received packets.
> > +        """The implementation of :method:`send_packets_and_capture`.
> > +
> > +        The subclasses must implement this method which sends `packets` on `send_port`
> > +        and receives packets on `receive_port` for the specified `duration`.
> > +
> > +        It must be able to handle no received packets.
>
> This sentence feels odd too. Maybe "It must be able to handle receiving
> no packets."
>

Right, your suggestion is better.

> >           """
> >
> >       def _write_capture_from_packets(
> > diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> > index ea7c3963da..ed396c6a2f 100644
> > --- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> > +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> > @@ -22,7 +22,8 @@
> >   class TrafficGenerator(ABC):
> >       """The base traffic generator.
> >
> > -    Defines the few basic methods that each traffic generator must implement.
> > +    Exposes the common public methods of all traffic generators and defines private methods
> > +    that must implement the traffic generation logic in subclasses.
> >       """
> >
> >       _config: TrafficGeneratorConfig
> > @@ -30,6 +31,12 @@ class TrafficGenerator(ABC):
> >       _logger: DTSLOG
> >
> >       def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
> > +        """Initialize the traffic generator.
> > +
> > +        Args:
> > +            tg_node: The traffic generator node where the created traffic generator will be running.
> > +            config: The traffic generator's test run configuration.
> > +        """
> >           self._config = config
> >           self._tg_node = tg_node
> >           self._logger = getLogger(
> > @@ -37,9 +44,9 @@ def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
> >           )
> >
> >       def send_packet(self, packet: Packet, port: Port) -> None:
> > -        """Send a packet and block until it is fully sent.
> > +        """Send `packet` and block until it is fully sent.
> >
> > -        What fully sent means is defined by the traffic generator.
> > +        Send `packet` on `port`, then wait until `packet` is fully sent.
> >
> >           Args:
> >               packet: The packet to send.
> > @@ -48,9 +55,9 @@ def send_packet(self, packet: Packet, port: Port) -> None:
> >           self.send_packets([packet], port)
> >
> >       def send_packets(self, packets: list[Packet], port: Port) -> None:
> > -        """Send packets and block until they are fully sent.
> > +        """Send `packets` and block until they are fully sent.
> >
> > -        What fully sent means is defined by the traffic generator.
> > +        Send `packets` on `port`, then wait until `packets` are fully sent.
> >
> >           Args:
> >               packets: The packets to send.
> > @@ -62,19 +69,17 @@ def send_packets(self, packets: list[Packet], port: Port) -> None:
> >
> >       @abstractmethod
> >       def _send_packets(self, packets: list[Packet], port: Port) -> None:
> > -        """
> > -        The extended classes must implement this method which
> > -        sends packets on send_port. The method should block until all packets
> > -        are fully sent.
> > +        """The implementation of :method:`send_packets`.
> > +
> > +        The subclasses must implement this method which sends `packets` on `port`.
> > +        The method should block until all `packets` are fully sent.
> > +
> > +        What full sent means is defined by the traffic generator.
>
> full -> fully
>
> >           """
> >
> >       @property
> >       def is_capturing(self) -> bool:
> > -        """Whether this traffic generator can capture traffic.
> > -
> > -        Returns:
> > -            True if the traffic generator can capture traffic, False otherwise.
> > -        """
> > +        """This traffic generator can't capture traffic."""
> >           return False
> >
> >       @abstractmethod
>
  

On 11/22/23 11:38, Juraj Linkeš wrote:
> On Tue, Nov 21, 2023 at 5:20 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>>
>> On 11/15/23 13:09, Juraj Linkeš wrote:
>>> Format according to the Google format and PEP257, with slight
>>> deviations.
>>>
>>> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
>>> ---
>>>    .../traffic_generator/__init__.py             | 22 ++++++++-
>>>    .../capturing_traffic_generator.py            | 46 +++++++++++--------
>>>    .../traffic_generator/traffic_generator.py    | 33 +++++++------
>>>    3 files changed, 68 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py b/dts/framework/testbed_model/traffic_generator/__init__.py
>>> index 11bfa1ee0f..51cca77da4 100644
>>> --- a/dts/framework/testbed_model/traffic_generator/__init__.py
>>> +++ b/dts/framework/testbed_model/traffic_generator/__init__.py
>>> @@ -1,6 +1,19 @@
>>>    # SPDX-License-Identifier: BSD-3-Clause
>>>    # Copyright(c) 2023 PANTHEON.tech s.r.o.
>>>
>>> +"""DTS traffic generators.
>>> +
>>> +A traffic generator is capable of generating traffic and then monitor returning traffic.
>>> +A traffic generator may just count the number of received packets
>>> +and it may additionally capture individual packets.
>>
>> The sentence feels odd. Isn't it supposed to be "or" here? and no need
>> for that early of a line break
>>
> 
> There are two mays, so there probably should be an or. But I'd like to
> reword it to this:
> 
> All traffic generators count the number of received packets, and they
> may additionally
> capture individual packets.
> 
> What do you think?

I think it's better with the new sentence. But I think it'd be even 
better to split into two sentences to highlight the must/may:
All traffic generators must count the number of received packets. Some
may additionally capture individual packets.

> 
>>> +
>>> +A traffic generator may be software running on generic hardware or it could be specialized hardware.
>>> +
>>> +The traffic generators that only count the number of received packets are suitable only for
>>> +performance testing. In functional testing, we need to be able to dissect each arrived packet
>>> +and a capturing traffic generator is required.
>>> +"""
>>> +
>>>    from framework.config import ScapyTrafficGeneratorConfig, TrafficGeneratorType
>>>    from framework.exception import ConfigurationError
>>>    from framework.testbed_model.node import Node
>>> @@ -12,8 +25,15 @@
>>>    def create_traffic_generator(
>>>        tg_node: Node, traffic_generator_config: ScapyTrafficGeneratorConfig
>>>    ) -> CapturingTrafficGenerator:
>>> -    """A factory function for creating traffic generator object from user config."""
>>> +    """The factory function for creating traffic generator objects from the test run configuration.
>>> +
>>> +    Args:
>>> +        tg_node: The traffic generator node where the created traffic generator will be running.
>>> +        traffic_generator_config: The traffic generator config.
>>>
>>> +    Returns:
>>> +        A traffic generator capable of capturing received packets.
>>> +    """
>>>        match traffic_generator_config.traffic_generator_type:
>>>            case TrafficGeneratorType.SCAPY:
>>>                return ScapyTrafficGenerator(tg_node, traffic_generator_config)
>>> diff --git a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
>>> index e521211ef0..b0a43ad003 100644
>>> --- a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
>>> +++ b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
>>> @@ -23,19 +23,22 @@
>>>
>>>
>>>    def _get_default_capture_name() -> str:
>>> -    """
>>> -    This is the function used for the default implementation of capture names.
>>> -    """
>>>        return str(uuid.uuid4())
>>>
>>>
>>>    class CapturingTrafficGenerator(TrafficGenerator):
>>>        """Capture packets after sending traffic.
>>>
>>> -    A mixin interface which enables a packet generator to declare that it can capture
>>> +    The intermediary interface which enables a packet generator to declare that it can capture
>>>        packets and return them to the user.
>>>
>>> +    Similarly to
>>> +    :class:`~framework.testbed_model.traffic_generator.traffic_generator.TrafficGenerator`,
>>> +    this class exposes the public methods specific to capturing traffic generators and defines
>>> +    a private method that must implement the traffic generation and capturing logic in subclasses.
>>> +
>>>        The methods of capturing traffic generators obey the following workflow:
>>> +
>>>            1. send packets
>>>            2. capture packets
>>>            3. write the capture to a .pcap file
>>> @@ -44,6 +47,7 @@ class CapturingTrafficGenerator(TrafficGenerator):
>>>
>>>        @property
>>>        def is_capturing(self) -> bool:
>>> +        """This traffic generator can capture traffic."""
>>>            return True
>>>
>>>        def send_packet_and_capture(
>>> @@ -54,11 +58,12 @@ def send_packet_and_capture(
>>>            duration: float,
>>>            capture_name: str = _get_default_capture_name(),
>>>        ) -> list[Packet]:
>>> -        """Send a packet, return received traffic.
>>> +        """Send `packet` and capture received traffic.
>>> +
>>> +        Send `packet` on `send_port` and then return all traffic captured
>>> +        on `receive_port` for the given `duration`.
>>>
>>> -        Send a packet on the send_port and then return all traffic captured
>>> -        on the receive_port for the given duration. Also record the captured traffic
>>> -        in a pcap file.
>>> +        The captured traffic is recorded in the `capture_name`.pcap file.
>>>
>>>            Args:
>>>                packet: The packet to send.
>>> @@ -68,7 +73,7 @@ def send_packet_and_capture(
>>>                capture_name: The name of the .pcap file where to store the capture.
>>>
>>>            Returns:
>>> -             A list of received packets. May be empty if no packets are captured.
>>> +             The received packets. May be empty if no packets are captured.
>>>            """
>>>            return self.send_packets_and_capture(
>>>                [packet], send_port, receive_port, duration, capture_name
>>> @@ -82,11 +87,14 @@ def send_packets_and_capture(
>>>            duration: float,
>>>            capture_name: str = _get_default_capture_name(),
>>>        ) -> list[Packet]:
>>> -        """Send packets, return received traffic.
>>> +        """Send `packets` and capture received traffic.
>>>
>>> -        Send packets on the send_port and then return all traffic captured
>>> -        on the receive_port for the given duration. Also record the captured traffic
>>> -        in a pcap file.
>>> +        Send `packets` on `send_port` and then return all traffic captured
>>> +        on `receive_port` for the given `duration`.
>>> +
>>> +        The captured traffic is recorded in the `capture_name`.pcap file. The target directory
>>> +        can be configured with the :option:`--output-dir` command line argument or
>>> +        the :envvar:`DTS_OUTPUT_DIR` environment variable.
>>>
>>>            Args:
>>>                packets: The packets to send.
>>> @@ -96,7 +104,7 @@ def send_packets_and_capture(
>>>                capture_name: The name of the .pcap file where to store the capture.
>>>
>>>            Returns:
>>> -             A list of received packets. May be empty if no packets are captured.
>>> +             The received packets. May be empty if no packets are captured.
>>>            """
>>>            self._logger.debug(get_packet_summaries(packets))
>>>            self._logger.debug(
>>> @@ -124,10 +132,12 @@ def _send_packets_and_capture(
>>>            receive_port: Port,
>>>            duration: float,
>>>        ) -> list[Packet]:
>>> -        """
>>> -        The extended classes must implement this method which
>>> -        sends packets on send_port and receives packets on the receive_port
>>> -        for the specified duration. It must be able to handle no received packets.
>>> +        """The implementation of :method:`send_packets_and_capture`.
>>> +
>>> +        The subclasses must implement this method which sends `packets` on `send_port`
>>> +        and receives packets on `receive_port` for the specified `duration`.
>>> +
>>> +        It must be able to handle no received packets.
>>
>> This sentence feels odd too. Maybe "It must be able to handle receiving
>> no packets."
>>
> 
> Right, your suggestion is better.
> 
>>>            """
>>>
>>>        def _write_capture_from_packets(
>>> diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
>>> index ea7c3963da..ed396c6a2f 100644
>>> --- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py
>>> +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
>>> @@ -22,7 +22,8 @@
>>>    class TrafficGenerator(ABC):
>>>        """The base traffic generator.
>>>
>>> -    Defines the few basic methods that each traffic generator must implement.
>>> +    Exposes the common public methods of all traffic generators and defines private methods
>>> +    that must implement the traffic generation logic in subclasses.
>>>        """
>>>
>>>        _config: TrafficGeneratorConfig
>>> @@ -30,6 +31,12 @@ class TrafficGenerator(ABC):
>>>        _logger: DTSLOG
>>>
>>>        def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
>>> +        """Initialize the traffic generator.
>>> +
>>> +        Args:
>>> +            tg_node: The traffic generator node where the created traffic generator will be running.
>>> +            config: The traffic generator's test run configuration.
>>> +        """
>>>            self._config = config
>>>            self._tg_node = tg_node
>>>            self._logger = getLogger(
>>> @@ -37,9 +44,9 @@ def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
>>>            )
>>>
>>>        def send_packet(self, packet: Packet, port: Port) -> None:
>>> -        """Send a packet and block until it is fully sent.
>>> +        """Send `packet` and block until it is fully sent.
>>>
>>> -        What fully sent means is defined by the traffic generator.
>>> +        Send `packet` on `port`, then wait until `packet` is fully sent.
>>>
>>>            Args:
>>>                packet: The packet to send.
>>> @@ -48,9 +55,9 @@ def send_packet(self, packet: Packet, port: Port) -> None:
>>>            self.send_packets([packet], port)
>>>
>>>        def send_packets(self, packets: list[Packet], port: Port) -> None:
>>> -        """Send packets and block until they are fully sent.
>>> +        """Send `packets` and block until they are fully sent.
>>>
>>> -        What fully sent means is defined by the traffic generator.
>>> +        Send `packets` on `port`, then wait until `packets` are fully sent.
>>>
>>>            Args:
>>>                packets: The packets to send.
>>> @@ -62,19 +69,17 @@ def send_packets(self, packets: list[Packet], port: Port) -> None:
>>>
>>>        @abstractmethod
>>>        def _send_packets(self, packets: list[Packet], port: Port) -> None:
>>> -        """
>>> -        The extended classes must implement this method which
>>> -        sends packets on send_port. The method should block until all packets
>>> -        are fully sent.
>>> +        """The implementation of :method:`send_packets`.
>>> +
>>> +        The subclasses must implement this method which sends `packets` on `port`.
>>> +        The method should block until all `packets` are fully sent.
>>> +
>>> +        What full sent means is defined by the traffic generator.
>>
>> full -> fully
>>
>>>            """
>>>
>>>        @property
>>>        def is_capturing(self) -> bool:
>>> -        """Whether this traffic generator can capture traffic.
>>> -
>>> -        Returns:
>>> -            True if the traffic generator can capture traffic, False otherwise.
>>> -        """
>>> +        """This traffic generator can't capture traffic."""
>>>            return False
>>>
>>>        @abstractmethod
>>
  

On Wed, Nov 22, 2023 at 1:05 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>
> On 11/22/23 11:38, Juraj Linkeš wrote:
> > On Tue, Nov 21, 2023 at 5:20 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
> >>
> >> On 11/15/23 13:09, Juraj Linkeš wrote:
> >>> Format according to the Google format and PEP257, with slight
> >>> deviations.
> >>>
> >>> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> >>> ---
> >>>    .../traffic_generator/__init__.py             | 22 ++++++++-
> >>>    .../capturing_traffic_generator.py            | 46 +++++++++++--------
> >>>    .../traffic_generator/traffic_generator.py    | 33 +++++++------
> >>>    3 files changed, 68 insertions(+), 33 deletions(-)
> >>>
> >>> diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py b/dts/framework/testbed_model/traffic_generator/__init__.py
> >>> index 11bfa1ee0f..51cca77da4 100644
> >>> --- a/dts/framework/testbed_model/traffic_generator/__init__.py
> >>> +++ b/dts/framework/testbed_model/traffic_generator/__init__.py
> >>> @@ -1,6 +1,19 @@
> >>>    # SPDX-License-Identifier: BSD-3-Clause
> >>>    # Copyright(c) 2023 PANTHEON.tech s.r.o.
> >>>
> >>> +"""DTS traffic generators.
> >>> +
> >>> +A traffic generator is capable of generating traffic and then monitor returning traffic.
> >>> +A traffic generator may just count the number of received packets
> >>> +and it may additionally capture individual packets.
> >>
> >> The sentence feels odd. Isn't it supposed to be "or" here? and no need
> >> for that early of a line break
> >>
> >
> > There are two mays, so there probably should be an or. But I'd like to
> > reword it to this:
> >
> > All traffic generators count the number of received packets, and they
> > may additionally
> > capture individual packets.
> >
> > What do you think?
>
> I think it's better with the new sentence. But I think it'd be even
> better to split into two sentences to highlight the must/may:
> All traffic generators must count the number of received packets. Some
> may additionally capture individual packets.
>

I like this, I'll reword it.

> >
> >>> +
> >>> +A traffic generator may be software running on generic hardware or it could be specialized hardware.
> >>> +
> >>> +The traffic generators that only count the number of received packets are suitable only for
> >>> +performance testing. In functional testing, we need to be able to dissect each arrived packet
> >>> +and a capturing traffic generator is required.
> >>> +"""
> >>> +
> >>>    from framework.config import ScapyTrafficGeneratorConfig, TrafficGeneratorType
> >>>    from framework.exception import ConfigurationError
> >>>    from framework.testbed_model.node import Node
> >>> @@ -12,8 +25,15 @@
> >>>    def create_traffic_generator(
> >>>        tg_node: Node, traffic_generator_config: ScapyTrafficGeneratorConfig
> >>>    ) -> CapturingTrafficGenerator:
> >>> -    """A factory function for creating traffic generator object from user config."""
> >>> +    """The factory function for creating traffic generator objects from the test run configuration.
> >>> +
> >>> +    Args:
> >>> +        tg_node: The traffic generator node where the created traffic generator will be running.
> >>> +        traffic_generator_config: The traffic generator config.
> >>>
> >>> +    Returns:
> >>> +        A traffic generator capable of capturing received packets.
> >>> +    """
> >>>        match traffic_generator_config.traffic_generator_type:
> >>>            case TrafficGeneratorType.SCAPY:
> >>>                return ScapyTrafficGenerator(tg_node, traffic_generator_config)
> >>> diff --git a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> >>> index e521211ef0..b0a43ad003 100644
> >>> --- a/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> >>> +++ b/dts/framework/testbed_model/traffic_generator/capturing_traffic_generator.py
> >>> @@ -23,19 +23,22 @@
> >>>
> >>>
> >>>    def _get_default_capture_name() -> str:
> >>> -    """
> >>> -    This is the function used for the default implementation of capture names.
> >>> -    """
> >>>        return str(uuid.uuid4())
> >>>
> >>>
> >>>    class CapturingTrafficGenerator(TrafficGenerator):
> >>>        """Capture packets after sending traffic.
> >>>
> >>> -    A mixin interface which enables a packet generator to declare that it can capture
> >>> +    The intermediary interface which enables a packet generator to declare that it can capture
> >>>        packets and return them to the user.
> >>>
> >>> +    Similarly to
> >>> +    :class:`~framework.testbed_model.traffic_generator.traffic_generator.TrafficGenerator`,
> >>> +    this class exposes the public methods specific to capturing traffic generators and defines
> >>> +    a private method that must implement the traffic generation and capturing logic in subclasses.
> >>> +
> >>>        The methods of capturing traffic generators obey the following workflow:
> >>> +
> >>>            1. send packets
> >>>            2. capture packets
> >>>            3. write the capture to a .pcap file
> >>> @@ -44,6 +47,7 @@ class CapturingTrafficGenerator(TrafficGenerator):
> >>>
> >>>        @property
> >>>        def is_capturing(self) -> bool:
> >>> +        """This traffic generator can capture traffic."""
> >>>            return True
> >>>
> >>>        def send_packet_and_capture(
> >>> @@ -54,11 +58,12 @@ def send_packet_and_capture(
> >>>            duration: float,
> >>>            capture_name: str = _get_default_capture_name(),
> >>>        ) -> list[Packet]:
> >>> -        """Send a packet, return received traffic.
> >>> +        """Send `packet` and capture received traffic.
> >>> +
> >>> +        Send `packet` on `send_port` and then return all traffic captured
> >>> +        on `receive_port` for the given `duration`.
> >>>
> >>> -        Send a packet on the send_port and then return all traffic captured
> >>> -        on the receive_port for the given duration. Also record the captured traffic
> >>> -        in a pcap file.
> >>> +        The captured traffic is recorded in the `capture_name`.pcap file.
> >>>
> >>>            Args:
> >>>                packet: The packet to send.
> >>> @@ -68,7 +73,7 @@ def send_packet_and_capture(
> >>>                capture_name: The name of the .pcap file where to store the capture.
> >>>
> >>>            Returns:
> >>> -             A list of received packets. May be empty if no packets are captured.
> >>> +             The received packets. May be empty if no packets are captured.
> >>>            """
> >>>            return self.send_packets_and_capture(
> >>>                [packet], send_port, receive_port, duration, capture_name
> >>> @@ -82,11 +87,14 @@ def send_packets_and_capture(
> >>>            duration: float,
> >>>            capture_name: str = _get_default_capture_name(),
> >>>        ) -> list[Packet]:
> >>> -        """Send packets, return received traffic.
> >>> +        """Send `packets` and capture received traffic.
> >>>
> >>> -        Send packets on the send_port and then return all traffic captured
> >>> -        on the receive_port for the given duration. Also record the captured traffic
> >>> -        in a pcap file.
> >>> +        Send `packets` on `send_port` and then return all traffic captured
> >>> +        on `receive_port` for the given `duration`.
> >>> +
> >>> +        The captured traffic is recorded in the `capture_name`.pcap file. The target directory
> >>> +        can be configured with the :option:`--output-dir` command line argument or
> >>> +        the :envvar:`DTS_OUTPUT_DIR` environment variable.
> >>>
> >>>            Args:
> >>>                packets: The packets to send.
> >>> @@ -96,7 +104,7 @@ def send_packets_and_capture(
> >>>                capture_name: The name of the .pcap file where to store the capture.
> >>>
> >>>            Returns:
> >>> -             A list of received packets. May be empty if no packets are captured.
> >>> +             The received packets. May be empty if no packets are captured.
> >>>            """
> >>>            self._logger.debug(get_packet_summaries(packets))
> >>>            self._logger.debug(
> >>> @@ -124,10 +132,12 @@ def _send_packets_and_capture(
> >>>            receive_port: Port,
> >>>            duration: float,
> >>>        ) -> list[Packet]:
> >>> -        """
> >>> -        The extended classes must implement this method which
> >>> -        sends packets on send_port and receives packets on the receive_port
> >>> -        for the specified duration. It must be able to handle no received packets.
> >>> +        """The implementation of :method:`send_packets_and_capture`.
> >>> +
> >>> +        The subclasses must implement this method which sends `packets` on `send_port`
> >>> +        and receives packets on `receive_port` for the specified `duration`.
> >>> +
> >>> +        It must be able to handle no received packets.
> >>
> >> This sentence feels odd too. Maybe "It must be able to handle receiving
> >> no packets."
> >>
> >
> > Right, your suggestion is better.
> >
> >>>            """
> >>>
> >>>        def _write_capture_from_packets(
> >>> diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> >>> index ea7c3963da..ed396c6a2f 100644
> >>> --- a/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> >>> +++ b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> >>> @@ -22,7 +22,8 @@
> >>>    class TrafficGenerator(ABC):
> >>>        """The base traffic generator.
> >>>
> >>> -    Defines the few basic methods that each traffic generator must implement.
> >>> +    Exposes the common public methods of all traffic generators and defines private methods
> >>> +    that must implement the traffic generation logic in subclasses.
> >>>        """
> >>>
> >>>        _config: TrafficGeneratorConfig
> >>> @@ -30,6 +31,12 @@ class TrafficGenerator(ABC):
> >>>        _logger: DTSLOG
> >>>
> >>>        def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
> >>> +        """Initialize the traffic generator.
> >>> +
> >>> +        Args:
> >>> +            tg_node: The traffic generator node where the created traffic generator will be running.
> >>> +            config: The traffic generator's test run configuration.
> >>> +        """
> >>>            self._config = config
> >>>            self._tg_node = tg_node
> >>>            self._logger = getLogger(
> >>> @@ -37,9 +44,9 @@ def __init__(self, tg_node: Node, config: TrafficGeneratorConfig):
> >>>            )
> >>>
> >>>        def send_packet(self, packet: Packet, port: Port) -> None:
> >>> -        """Send a packet and block until it is fully sent.
> >>> +        """Send `packet` and block until it is fully sent.
> >>>
> >>> -        What fully sent means is defined by the traffic generator.
> >>> +        Send `packet` on `port`, then wait until `packet` is fully sent.
> >>>
> >>>            Args:
> >>>                packet: The packet to send.
> >>> @@ -48,9 +55,9 @@ def send_packet(self, packet: Packet, port: Port) -> None:
> >>>            self.send_packets([packet], port)
> >>>
> >>>        def send_packets(self, packets: list[Packet], port: Port) -> None:
> >>> -        """Send packets and block until they are fully sent.
> >>> +        """Send `packets` and block until they are fully sent.
> >>>
> >>> -        What fully sent means is defined by the traffic generator.
> >>> +        Send `packets` on `port`, then wait until `packets` are fully sent.
> >>>
> >>>            Args:
> >>>                packets: The packets to send.
> >>> @@ -62,19 +69,17 @@ def send_packets(self, packets: list[Packet], port: Port) -> None:
> >>>
> >>>        @abstractmethod
> >>>        def _send_packets(self, packets: list[Packet], port: Port) -> None:
> >>> -        """
> >>> -        The extended classes must implement this method which
> >>> -        sends packets on send_port. The method should block until all packets
> >>> -        are fully sent.
> >>> +        """The implementation of :method:`send_packets`.
> >>> +
> >>> +        The subclasses must implement this method which sends `packets` on `port`.
> >>> +        The method should block until all `packets` are fully sent.
> >>> +
> >>> +        What full sent means is defined by the traffic generator.
> >>
> >> full -> fully
> >>
> >>>            """
> >>>
> >>>        @property
> >>>        def is_capturing(self) -> bool:
> >>> -        """Whether this traffic generator can capture traffic.
> >>> -
> >>> -        Returns:
> >>> -            True if the traffic generator can capture traffic, False otherwise.
> >>> -        """
> >>> +        """This traffic generator can't capture traffic."""
> >>>            return False
> >>>
> >>>        @abstractmethod
> >>
>