Re: [PATCH] dts: add l2fwd test suite

[Jeremy Spewock <jspewock@iol.unh.edu>]

Hey Luca,

Thanks for the patch! Just a few pretty minor comments below.

On Tue, Aug 6, 2024 at 8:53 AM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> Add a basic L2 forwarding test suite which tests the correct
> functionality of the forwarding facility built-in in the DPDK.
>
> The tests are performed with different queues numbers per port.
>
> Bugzilla ID: 1481
>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
> Depends-on: series-32714 ("dts: add pktgen and testpmd changes")

Out of my own curiosity, are depends on supposed to be outside of the
commit body? I don't think it really matters for automation or
anything regardless, but I just didn't know if there was a rule about
it.

> ---
>  dts/framework/config/conf_yaml_schema.json |  3 +-
>  dts/tests/TestSuite_l2fwd.py               | 58 ++++++++++++++++++++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
>  create mode 100644 dts/tests/TestSuite_l2fwd.py
>
> diff --git a/dts/framework/config/conf_yaml_schema.json b/dts/framework/config/conf_yaml_schema.json
> index df390e8ae2..58a719a923 100644
> --- a/dts/framework/config/conf_yaml_schema.json
> +++ b/dts/framework/config/conf_yaml_schema.json
> @@ -187,7 +187,8 @@
>        "enum": [
>          "hello_world",
>          "os_udp",
> -        "pmd_buffer_scatter"
> +        "pmd_buffer_scatter",
> +        "l2fwd"
>        ]
>      },
>      "test_target": {
> diff --git a/dts/tests/TestSuite_l2fwd.py b/dts/tests/TestSuite_l2fwd.py
> new file mode 100644
> index 0000000000..46f07b78eb
> --- /dev/null
> +++ b/dts/tests/TestSuite_l2fwd.py
> @@ -0,0 +1,58 @@

This file looks like it is missing the copyright information at the top.

> +"""Basic L2 forwarding test suite.
> +
> +This testing suites runs basic L2 forwarding on testpmd with different queue sizes per port.

The phrasing of "different queue sizes per port" makes me initially
think that like, port 0 will have 2 queues and port 1 will have 4.
Maybe something like "This testing suites runs basic L2 forwarding on
testpmd across multiple different queue sizes" would make this more
clear.

> +The forwarding test is performed with several packets being sent at once.
> +"""
> +
> +from framework.params.testpmd import EthPeer, SimpleForwardingModes
> +from framework.remote_session.testpmd_shell import TestPmdShell
> +from framework.test_suite import TestSuite
> +from framework.testbed_model.cpu import LogicalCoreCount
> +from framework.utils import generate_random_packets
> +
> +
> +class TestL2fwd(TestSuite):
> +    """L2 forwarding test suite."""
> +
> +    #: The total number of packets to generate and send for forwarding.
> +    NUMBER_OF_PACKETS_TO_SEND = 50
> +    #: The payload size to use for the generated packets in bytes.
> +    PAYLOAD_SIZE = 100
> +
> +    def set_up_suite(self) -> None:
> +        """Set up the test suite.
> +
> +        Setup:
> +            Verify that we have at least 2 ports in the current test. Generate the random packets
> +            that will be sent and spawn a reusable testpmd shell.

Seems like this method is no longer spawning a testpmd shell, so this
part of the doc-string is no longer relevant.

> +        """
> +        self.verify(len(self.sut_node.ports) >= 2, "At least 2 ports are required for this test.")
> +        self.packets = generate_random_packets(self.NUMBER_OF_PACKETS_TO_SEND, self.PAYLOAD_SIZE)
> +
> +    def test_l2fwd_integrity(self) -> None:
> +        """Test the L2 forwarding integrity.
> +
> +        Test:
> +            Configure a testpmd shell with a different numbers of queues per run. Start up L2

It might make sense to name the numbers of queues in the doc-string
just so that the rst for the suite is more clear.

> +            forwarding, send random packets from the TG and verify they were all received back.
> +        """
> +        queues = [1, 2, 4, 8]
> +
> +        with TestPmdShell(
> +            self.sut_node,
> +            lcore_filter_specifier=LogicalCoreCount(cores_per_socket=4),
> +            forward_mode=SimpleForwardingModes.mac,
> +            eth_peer=[EthPeer(1, self.tg_node.ports[1].mac_address)],
> +            disable_device_start=True,
> +        ) as shell:
> +            for queues_num in queues:
> +                self._logger.info(f"Testing L2 forwarding with {queues_num} queue(s)")
> +                shell.set_ports_queues(queues_num)
> +                shell.start()
> +
> +                received_packets = self.send_packets_and_capture(self.packets)
> +
> +                expected_packets = [self.get_expected_packet(packet) for packet in self.packets]

Ahh, the get_expected_packet method also sheds some light on how the
match_all_packets could be useful.

> +                self.match_all_packets(expected_packets, received_packets)
> +
> +                shell.stop()
> --
> 2.34.1
>