From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 8327043347;
	Thu, 16 Nov 2023 18:36:34 +0100 (CET)
Received: from mails.dpdk.org (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id 2661A402C6;
	Thu, 16 Nov 2023 18:36:34 +0100 (CET)
Received: from foss.arm.com (foss.arm.com [217.140.110.172])
 by mails.dpdk.org (Postfix) with ESMTP id 0A5A240150
 for <dev@dpdk.org>; Thu, 16 Nov 2023 18:36:33 +0100 (CET)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
 by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A3CF11595;
 Thu, 16 Nov 2023 09:37:18 -0800 (PST)
Received: from [10.1.28.149] (e125442.arm.com [10.1.28.149])
 by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 21D3F3F6C4;
 Thu, 16 Nov 2023 09:36:30 -0800 (PST)
Message-ID: <1a965105-53d2-45cc-9c9d-2054aabeafc0@foss.arm.com>
Date: Thu, 16 Nov 2023 17:36:21 +0000
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 21/21] dts: test suites docstring update
Content-Language: en-US
To: =?UTF-8?Q?Juraj_Linke=C5=A1?= <juraj.linkes@pantheon.tech>,
 thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu,
 probb@iol.unh.edu, paul.szczepanek@arm.com
Cc: dev@dpdk.org
References: <20231108125324.191005-23-juraj.linkes@pantheon.tech>
 <20231115130959.39420-1-juraj.linkes@pantheon.tech>
 <20231115130959.39420-22-juraj.linkes@pantheon.tech>
From: Yoan Picchi <yoan.picchi@foss.arm.com>
In-Reply-To: <20231115130959.39420-22-juraj.linkes@pantheon.tech>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

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>
> ---
>   dts/tests/TestSuite_hello_world.py | 16 +++++----
>   dts/tests/TestSuite_os_udp.py      | 19 +++++++----
>   dts/tests/TestSuite_smoke_tests.py | 53 +++++++++++++++++++++++++++---
>   3 files changed, 70 insertions(+), 18 deletions(-)
> 
> diff --git a/dts/tests/TestSuite_hello_world.py b/dts/tests/TestSuite_hello_world.py
> index 7e3d95c0cf..662a8f8726 100644
> --- a/dts/tests/TestSuite_hello_world.py
> +++ b/dts/tests/TestSuite_hello_world.py
> @@ -1,7 +1,8 @@
>   # SPDX-License-Identifier: BSD-3-Clause
>   # Copyright(c) 2010-2014 Intel Corporation
>   
> -"""
> +"""The DPDK hello world app test suite.
> +
>   Run the helloworld example app and verify it prints a message for each used core.
>   No other EAL parameters apart from cores are used.
>   """
> @@ -15,22 +16,25 @@
>   
>   
>   class TestHelloWorld(TestSuite):
> +    """DPDK hello world app test suite."""
> +
>       def set_up_suite(self) -> None:
> -        """
> +        """Set up the test suite.
> +
>           Setup:
>               Build the app we're about to test - helloworld.
>           """
>           self.app_helloworld_path = self.sut_node.build_dpdk_app("helloworld")
>   
>       def test_hello_world_single_core(self) -> None:
> -        """
> +        """Single core test case.
> +
>           Steps:
>               Run the helloworld app on the first usable logical core.
>           Verify:
>               The app prints a message from the used core:
>               "hello from core <core_id>"
>           """
> -
>           # get the first usable core
>           lcore_amount = LogicalCoreCount(1, 1, 1)
>           lcores = LogicalCoreCountFilter(self.sut_node.lcores, lcore_amount).filter()
> @@ -44,14 +48,14 @@ def test_hello_world_single_core(self) -> None:
>           )
>   
>       def test_hello_world_all_cores(self) -> None:
> -        """
> +        """All cores test case.
> +
>           Steps:
>               Run the helloworld app on all usable logical cores.
>           Verify:
>               The app prints a message from all used cores:
>               "hello from core <core_id>"
>           """
> -
>           # get the maximum logical core number
>           eal_para = self.sut_node.create_eal_parameters(
>               lcore_filter_specifier=LogicalCoreList(self.sut_node.lcores)
> diff --git a/dts/tests/TestSuite_os_udp.py b/dts/tests/TestSuite_os_udp.py
> index bf6b93deb5..e0c5239612 100644
> --- a/dts/tests/TestSuite_os_udp.py
> +++ b/dts/tests/TestSuite_os_udp.py
> @@ -1,7 +1,8 @@
>   # SPDX-License-Identifier: BSD-3-Clause
>   # Copyright(c) 2023 PANTHEON.tech s.r.o.
>   
> -"""
> +"""Basic IPv4 OS routing test suite.
> +
>   Configure SUT node to route traffic from if1 to if2.
>   Send a packet to the SUT node, verify it comes back on the second port on the TG node.
>   """
> @@ -13,24 +14,27 @@
>   
>   
>   class TestOSUdp(TestSuite):
> +    """IPv4 UDP OS routing test suite."""
> +
>       def set_up_suite(self) -> None:
> -        """
> +        """Set up the test suite.
> +
>           Setup:
> -            Configure SUT ports and SUT to route traffic from if1 to if2.
> +            Bind the SUT ports to the OS driver, configure the ports and configure the SUT
> +            to route traffic from if1 to if2.
>           """
>   
> -        # This test uses kernel drivers
>           self.sut_node.bind_ports_to_driver(for_dpdk=False)
>           self.configure_testbed_ipv4()
>   
>       def test_os_udp(self) -> None:
> -        """
> +        """Basic UDP IPv4 traffic test case.
> +
>           Steps:
>               Send a UDP packet.
>           Verify:
>               The packet with proper addresses arrives at the other TG port.
>           """
> -
>           packet = Ether() / IP() / UDP()
>   
>           received_packets = self.send_packet_and_capture(packet)
> @@ -40,7 +44,8 @@ def test_os_udp(self) -> None:
>           self.verify_packets(expected_packet, received_packets)
>   
>       def tear_down_suite(self) -> None:
> -        """
> +        """Tear down the test suite.
> +
>           Teardown:
>               Remove the SUT port configuration configured in setup.
>           """
> diff --git a/dts/tests/TestSuite_smoke_tests.py b/dts/tests/TestSuite_smoke_tests.py
> index e8016d1b54..6fae099a0e 100644
> --- a/dts/tests/TestSuite_smoke_tests.py
> +++ b/dts/tests/TestSuite_smoke_tests.py
> @@ -1,6 +1,17 @@
>   # SPDX-License-Identifier: BSD-3-Clause
>   # Copyright(c) 2023 University of New Hampshire
>   
> +"""Smoke test suite.
> +
> +Smoke tests are a class of tests which are used for validating a minimal set of important features.
> +These are the most important features without which (or when they're faulty) the software wouldn't
> +work properly. Thus, if any failure occurs while testing these features,
> +there isn't that much of a reason to continue testing, as the software is fundamentally broken.
> +
> +These tests don't have to include only DPDK tests, as the reason for failures could be
> +in the infrastructure (a faulty link between NICs or a misconfiguration).
> +"""
> +
>   import re
>   
>   from framework.config import PortConfig
> @@ -11,13 +22,25 @@
>   
>   
>   class SmokeTests(TestSuite):
> +    """DPDK and infrastructure smoke test suite.
> +
> +    The test cases validate the most basic DPDK functionality needed for all other test suites.
> +    The infrastructure also needs to be tested, as that is also used by all other test suites.
> +
> +    Attributes:
> +        is_blocking: This test suite will block the execution of all other test suites
> +            in the build target after it.
> +        nics_in_node: The NICs present on the SUT node.
> +    """
> +
>       is_blocking = True
>       # dicts in this list are expected to have two keys:
>       # "pci_address" and "current_driver"
>       nics_in_node: list[PortConfig] = []
>   
>       def set_up_suite(self) -> None:
> -        """
> +        """Set up the test suite.
> +
>           Setup:
>               Set the build directory path and generate a list of NICs in the SUT node.
>           """
> @@ -25,7 +48,13 @@ def set_up_suite(self) -> None:
>           self.nics_in_node = self.sut_node.config.ports
>   
>       def test_unit_tests(self) -> None:
> -        """
> +        """DPDK meson fast-tests unit tests.
> +
> +        The DPDK unit tests are basic tests that indicate regressions and other critical failures.
> +        These need to be addressed before other testing.
> +
> +        The fast-tests unit tests are a subset with only the most basic tests.
> +
>           Test:
>               Run the fast-test unit-test suite through meson.
>           """
> @@ -37,7 +66,14 @@ def test_unit_tests(self) -> None:
>           )
>   
>       def test_driver_tests(self) -> None:
> -        """
> +        """DPDK meson driver-tests unit tests.
> +

Copy paste from the previous unit test in the driver tests. If it is on 
purpose as both are considered unit tests, then the previous function is 
test_unit_tests and deal with fast-tests

> +
> +        The driver-tests unit tests are a subset that test only drivers. These may be run
> +        with virtual devices as well.
> +
>           Test:
>               Run the driver-test unit-test suite through meson.
>           """
> @@ -63,7 +99,10 @@ def test_driver_tests(self) -> None:
>           )
>   
>       def test_devices_listed_in_testpmd(self) -> None:
> -        """
> +        """Testpmd device discovery.
> +
> +        If the configured devices can't be found in testpmd, they can't be tested.

Maybe a bit nitpicky. This is more of a statement as to why the test 
exist than a description of the test. Suggestion: "Tests that the 
configured devices can be found in testpmd. If they aren't, the 
configuration might be wrong and tests might be skipped"

> +
>           Test:
>               Uses testpmd driver to verify that devices have been found by testpmd.
>           """
> @@ -79,7 +118,11 @@ def test_devices_listed_in_testpmd(self) -> None:
>               )
>   
>       def test_device_bound_to_driver(self) -> None:
> -        """
> +        """Device driver in OS.
> +
> +        The devices must be bound to the proper driver, otherwise they can't be used by DPDK
> +        or the traffic generators.

Same as the previous comment. It is more of a statement as to why the 
test exist than a description of the test

> +
>           Test:
>               Ensure that all drivers listed in the config are bound to the correct
>               driver.