Re: [PATCH v7 21/21] dts: test suites docstring update

[Yoan Picchi <yoan.picchi@foss.arm.com>]

On 11/20/23 10:17, Juraj Linkeš wrote:
> On Thu, Nov 16, 2023 at 6:36 PM Yoan Picchi <yoan.picchi@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@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
>>
> 
> 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.