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

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

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.

> > +
> > +        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?

> > +
> >           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.