[PATCH v7 19/21] dts: base traffic generators docstring update
Juraj Linkeš
juraj.linkes at pantheon.tech
Wed Nov 22 14:11:28 CET 2023
On Wed, Nov 22, 2023 at 1:05 PM Yoan Picchi <yoan.picchi at 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 at 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 at 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
> >>
>
More information about the dev
mailing list