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 9E6F943382; Wed, 22 Nov 2023 14:40:32 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1848C402E8; Wed, 22 Nov 2023 14:40:32 +0100 (CET) Received: from mail-ej1-f53.google.com (mail-ej1-f53.google.com [209.85.218.53]) by mails.dpdk.org (Postfix) with ESMTP id 60B124028C for ; Wed, 22 Nov 2023 14:40:31 +0100 (CET) Received: by mail-ej1-f53.google.com with SMTP id a640c23a62f3a-a00d5b0ec44so359623566b.0 for ; Wed, 22 Nov 2023 05:40:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700660431; x=1701265231; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=KTZDmLhh+PFj2zVyJe5H7MhzrZhzDMdIJic7vkqN5Cs=; b=YPF7TDvCfXAlL6qmq0NXiGV5Bokg3jC5AbS0WoTNils5RVMEZ+SgfHQ10W7OV8lURU fqKLhWWL5ED5EOKTFaKtD9prn/Zpvy4Wc2sux8lj8nCXZ2hWuTXhEES+RiYsCvZe7l/G oGZdh3wAyQhhBc38+zQMmMvmwtY8RAmaeIsAcIT7+6LX0w4cP/23YQ2knyUQVMHbT6ZQ /i7zTLN0/vs0qEPqcRdLnWNXaEtgVos7N3+mKUPAf9tcoc0JWIOGx8PyHbrGYrmzYTG0 gzbaSvSVZrciqT0lguGvuLUMRFKUzRJipbEGm/0d9lKLczARVCqci8prxC5xGvFoYZ25 5Txg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700660431; x=1701265231; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=KTZDmLhh+PFj2zVyJe5H7MhzrZhzDMdIJic7vkqN5Cs=; b=fQloASSCcnkZ682fXV65yE/5KQYg1cJzZM0kANMXDFaQ9sGpBrOFFiskgDMDjVDob1 IdUe5t3DWRoONVyT8k8uFGdc9HkpxbQsGbd9+3dMjCKaiTR7GoB9WBH0lI4plQML71wp qEoYchxrnsdXIybC1TJLrBujkJSe9i6mQfxMv76VUA6odyMIOwVyleaNFQu0qlrZsOG0 mKtJgge3YZkfcP64HaYq6wpQ/dCOLUg68ljAoWmUpWUruJqQWLlU6/g7FqHupYxujTVE grj7qaaVZy0V28LTOjO98uhVKDRNsBrzFgYCcX2BAzOEcTgvMM9Gq/0ZBjSoISyatZhI O/mQ== X-Gm-Message-State: AOJu0Yxf0zhIT43tnQzw+JW01Egl/AfH/bA50+T3MRFBf1hdtEC3ieco LlCIrK1p0Swy3oFDIfJZMILJoSS1BI1xByvX7I3jNw== X-Google-Smtp-Source: AGHT+IEU4Jm0AaMOBG1n1jKKVBYkRWYVMrzVNox6A2rEvzCGtJ5c+DH/8OdYTNoeZKAd5QoHN7i4tiRy4OF/k0KScvM= X-Received: by 2002:a17:907:d38a:b0:9c7:59d1:b2ce with SMTP id vh10-20020a170907d38a00b009c759d1b2cemr1921403ejc.5.1700660430904; Wed, 22 Nov 2023 05:40:30 -0800 (PST) MIME-Version: 1.0 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> <77a7faa3-785e-4e66-9bf8-4474b1e1263e@foss.arm.com> In-Reply-To: <77a7faa3-785e-4e66-9bf8-4474b1e1263e@foss.arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 22 Nov 2023 14:40:20 +0100 Message-ID: Subject: Re: [PATCH v7 21/21] dts: test suites docstring update To: Yoan Picchi Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Mon, Nov 20, 2023 at 1:50=E2=80=AFPM Yoan Picchi wrote: > > On 11/20/23 10:17, Juraj Linke=C5=A1 wrote: > > On Thu, Nov 16, 2023 at 6:36=E2=80=AFPM Yoan Picchi wrote: > >> > >> On 11/15/23 13:09, Juraj Linke=C5=A1 wrote: > >>> Format according to the Google format and PEP257, with slight > >>> deviations. > >>> > >>> Signed-off-by: Juraj Linke=C5=A1 > >>> --- > >>> 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 =3D 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 cor= e. > >>> Verify: > >>> The app prints a message from the used core: > >>> "hello from core " > >>> """ > >>> - > >>> # get the first usable core > >>> lcore_amount =3D LogicalCoreCount(1, 1, 1) > >>> lcores =3D LogicalCoreCountFilter(self.sut_node.lcores, lc= ore_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 =3D self.sut_node.create_eal_parameters( > >>> lcore_filter_specifier=3DLogicalCoreList(self.sut_node= .lcores) > >>> diff --git a/dts/tests/TestSuite_os_udp.py b/dts/tests/TestSuite_os_u= dp.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=3DFalse) > >>> 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 =3D Ether() / IP() / UDP() > >>> > >>> received_packets =3D 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 min= imal 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 featu= res, > >>> +there isn't that much of a reason to continue testing, as the softwa= re 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 misconfigurat= ion). > >>> +""" > >>> + > >>> 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 =3D True > >>> # dicts in this list are expected to have two keys: > >>> # "pci_address" and "current_driver" > >>> nics_in_node: list[PortConfig] =3D [] > >>> > >>> def set_up_suite(self) -> None: > >>> - """ > >>> + """Set up the test suite. > >>> + > >>> Setup: > >>> Set the build directory path and generate a list of NI= Cs in the SUT node. > >>> """ > >>> @@ -25,7 +48,13 @@ def set_up_suite(self) -> None: > >>> self.nics_in_node =3D 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 regression= s and other critical failures. > >>> + These need to be addressed before other testing. > >>> + > >>> + The fast-tests unit tests are a subset with only the most ba= sic 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 o= n > >> 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 driv= ers. 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 ca= n'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. > Ok, let's not complicate this any further. I thought a bit more about this and I've also come to the conclusion that the test suite description is enough (because of the redundancy). I'll just try to make the test case short descriptions more consistent. > >>> + > >>> Test: > >>> Uses testpmd driver to verify that devices have been f= ound 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 th= ey 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. >