From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E6F1C4337E; Mon, 20 Nov 2023 13:50:56 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 835F442DDE; Mon, 20 Nov 2023 13:50:56 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 0FE9042DD2 for ; Mon, 20 Nov 2023 13:50:55 +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 B6B19FEC; Mon, 20 Nov 2023 04:51:40 -0800 (PST) Received: from [10.57.74.8] (unknown [10.57.74.8]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 00FB43F6C4; Mon, 20 Nov 2023 04:50:52 -0800 (PST) Message-ID: <77a7faa3-785e-4e66-9bf8-4474b1e1263e@foss.arm.com> Date: Mon, 20 Nov 2023 12:50:52 +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?= Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, 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> <1a965105-53d2-45cc-9c9d-2054aabeafc0@foss.arm.com> From: Yoan Picchi In-Reply-To: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 11/20/23 10:17, Juraj Linkeš wrote: > On Thu, Nov 16, 2023 at 6:36 PM Yoan Picchi 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š >>> --- >>> 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 " >>> """ >>> - >>> # 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 " >>> """ >>> - >>> # 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 >> > > I'm not sure what you mean. The two are separate tests (one with the > fast-test, the other one with the driver-test unit test test suites) > and the docstring do capture the differences. I am a little bit confused as to how I deleted it in my reply, but I was referencing to this sentence in the patch: "The DPDK unit tests are basic tests that indicate regressions and other critical failures. These need to be addressed before other testing." But in any case, reading it again, I agree with you. > >>> + >>> + 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" >> > > This is more of a reason for why this particular test is a smoke test. > Since a smoke test failure results in all test suites being blocked, > this seemed like key information. > > We also don't have an exact format of what should be included in a > test case/suite documentation. We should use this opportunity to > document what we deem important in these test cases at this point in > time and improve the docs as we continue adding test cases. We can add > more custom sections (such as the "Setup:" and" "Test:" sections, > which can be added to Sphinx); I like adding a section with > explanation for why a test is a particular type of test (in this case, > a smoke test). The regular body could contain a description as you > suggested. What do you think? > I'm not really sure what way to go here. The thing I noticed here was mainly the lack of consistency between this test's description and the previous one. I agree that making it clear it's a smoke test is good, but compare it to test_device_bound_to_driver's description for instance. Both states clearly that they are a smoke test, but the formulation is quite different. I'm not entirely sure about adding more custom sections. I fear it might be more hassle than it's worth. A short guideline on how to write the doc and what section to use could be handy though. Reading the previous test, I think I see what you mean by having a section to describe the test type and another for the description. In short the type is: DPDK unit tests, test critical failures, needs to run first and then followed by the test's description But I think this type is redundant with the test suite's description? If so, only a description would be needed. >>> + >>> 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 >> > > Ack. > >>> + >>> Test: >>> Ensure that all drivers listed in the config are bound to the correct >>> driver.