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

Message ID 20231123151344.162812-20-juraj.linkes@pantheon.tech (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series dts: docstrings update |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Juraj Linkeš Nov. 23, 2023, 3:13 p.m. UTC
  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            | 45 +++++++++++--------
 .../traffic_generator/traffic_generator.py    | 33 ++++++++------
 3 files changed, 67 insertions(+), 33 deletions(-)
  

Comments

Jeremy Spewock Dec. 1, 2023, 6:05 p.m. UTC | #1
On Thu, Nov 23, 2023 at 10:14 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
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            | 45 +++++++++++--------
>  .../traffic_generator/traffic_generator.py    | 33 ++++++++------
>  3 files changed, 67 insertions(+), 33 deletions(-)
>
> diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py
> b/dts/framework/testbed_model/traffic_generator/__init__.py
> index 52888d03fa..11e2bd7d97 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.
> +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 1fc7f98c05..0246590333 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,21 @@
>
>
>  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:`~.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 +46,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 +57,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 +72,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 +86,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 +103,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(
> @@ -121,10 +128,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 receiving no packets.
>          """
>
>      def _write_capture_from_packets(self, capture_name: str, packets:
> list[Packet]) -> None:
> diff --git
> a/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
> index 0d9902ddb7..5fb9824568 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,14 +31,20 @@ 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(f"{self._tg_node.name}
> {self._config.traffic_generator_type}")
>
>      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.
> @@ -46,9 +53,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.
> @@ -60,19 +67,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.
>          """
>

I think this should be "what fully sent means"


>      @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
> --
> 2.34.1
>
>
  
Juraj Linkeš Dec. 4, 2023, 10:03 a.m. UTC | #2
On Fri, Dec 1, 2023 at 7:05 PM Jeremy Spewock <jspewock@iol.unh.edu> wrote:
>
>
>
> On Thu, Nov 23, 2023 at 10:14 AM Juraj Linkeš <juraj.linkes@pantheon.tech> 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            | 45 +++++++++++--------
>>  .../traffic_generator/traffic_generator.py    | 33 ++++++++------
>>  3 files changed, 67 insertions(+), 33 deletions(-)
>>
>> diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py b/dts/framework/testbed_model/traffic_generator/__init__.py
>> index 52888d03fa..11e2bd7d97 100644
>> --- a/dts/framework/testbed_model/traffic_generator/__init__.py
>> +++ b/dts/framework/testbed_model/traffic_generator/__init__.py
<snip>
>> @@ -60,19 +67,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.
>>          """
>
>
> I think this should be "what fully sent means"
>

Thanks, Yoan also caught this.
  

Patch

diff --git a/dts/framework/testbed_model/traffic_generator/__init__.py b/dts/framework/testbed_model/traffic_generator/__init__.py
index 52888d03fa..11e2bd7d97 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.
+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 1fc7f98c05..0246590333 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,21 @@ 
 
 
 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:`~.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 +46,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 +57,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 +72,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 +86,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 +103,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(
@@ -121,10 +128,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 receiving no packets.
         """
 
     def _write_capture_from_packets(self, capture_name: str, packets: list[Packet]) -> None:
diff --git a/dts/framework/testbed_model/traffic_generator/traffic_generator.py b/dts/framework/testbed_model/traffic_generator/traffic_generator.py
index 0d9902ddb7..5fb9824568 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,14 +31,20 @@  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(f"{self._tg_node.name} {self._config.traffic_generator_type}")
 
     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.
@@ -46,9 +53,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.
@@ -60,19 +67,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.
         """
 
     @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