[RFC PATCH v2] dts: skip test cases based on capabilities

[Nicholas Pratte]

I think this implementation is great, and I plan on testing it
properly with the jumbo frames suite that I am developing before
giving the final review. The only input that I could reasonably give
is a couple rewordings on the docstrings which I'll highlight below.

On Thu, Apr 11, 2024 at 4:48 AM Juraj Linkeš <juraj.linkes at pantheon.tech> wrote:
>
> The devices under test may not support the capabilities required by
> various test cases. Add support for:
> 1. Marking test suites and test cases with required capabilities,
> 2. Getting which required capabilities are supported by the device under
>    test,
> 3. And then skipping test suites and test cases if their required
>    capabilities are not supported by the device.
>
> Signed-off-by: Juraj Linkeš <juraj.linkes at pantheon.tech>
> ---
>  dts/framework/remote_session/__init__.py      |  2 +-
>  dts/framework/remote_session/testpmd_shell.py | 44 ++++++++-
>  dts/framework/runner.py                       | 46 ++++++++--
>  dts/framework/test_result.py                  | 90 +++++++++++++++----
>  dts/framework/test_suite.py                   | 25 ++++++
>  dts/framework/testbed_model/sut_node.py       | 25 +++++-
>  dts/tests/TestSuite_hello_world.py            |  4 +-
>  7 files changed, 204 insertions(+), 32 deletions(-)
>
> diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py
> index 1910c81c3c..f18a9f2259 100644
> --- a/dts/framework/remote_session/__init__.py
> +++ b/dts/framework/remote_session/__init__.py
> @@ -22,7 +22,7 @@
>  from .python_shell import PythonShell
>  from .remote_session import CommandResult, RemoteSession
>  from .ssh_session import SSHSession
> -from .testpmd_shell import TestPmdShell
> +from .testpmd_shell import NicCapability, TestPmdShell
>
>
>  def create_remote_session(
> diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
> index cb2ab6bd00..f6783af621 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -16,7 +16,9 @@
>  """
>
>  import time
> -from enum import auto
> +from collections.abc import MutableSet
> +from enum import Enum, auto
> +from functools import partial
>  from pathlib import PurePath
>  from typing import Callable, ClassVar
>
> @@ -229,3 +231,43 @@ def close(self) -> None:
>          """Overrides :meth:`~.interactive_shell.close`."""
>          self.send_command("quit", "")
>          return super().close()
> +
> +    def get_capas_rxq(
> +        self, supported_capabilities: MutableSet, unsupported_capabilities: MutableSet
> +    ) -> None:
> +        """Get all rxq capabilities and divide them into supported and unsupported.
> +
> +        Args:
> +            supported_capabilities: A set where capabilities which are supported will be stored.
> +            unsupported_capabilities: A set where capabilities which are
> +                not supported will be stored.

Maybe change the arg descriptions to something like "A set where
supported capabilities are stored" and "A set where unsupported
capabilities are stored."

> +        """
> +        self._logger.debug("Getting rxq capabilities.")
> +        command = "show rxq info 0 0"
> +        rxq_info = self.send_command(command)
> +        for line in rxq_info.split("\n"):
> +            bare_line = line.strip()
> +            if bare_line.startswith("RX scattered packets:"):
> +                if bare_line.endswith("on"):
> +                    supported_capabilities.add(NicCapability.scattered_rx)
> +                else:
> +                    unsupported_capabilities.add(NicCapability.scattered_rx)
> +
> +
> +class NicCapability(Enum):
> +    """A mapping between capability names and the associated :class:`TestPmdShell` methods.
> +
> +    The :class:`TestPmdShell` method executes the command that checks
> +    whether the capability is supported.
> +
> +    The signature of each :class:`TestPmdShell` method must be::
> +
> +        fn(self, supported_capabilities: MutableSet, unsupported_capabilities: MutableSet) -> None
> +
> +    The function must execute the testpmd command from which the capability support can be obtained.

"Which capability supported can be obtained." I think there was tense
error here.

> +    If multiple capabilities can be obtained from the same testpmd command, each should be obtained
> +    in one function. These capabilities should then be added to `supported_capabilities` or
> +    `unsupported_capabilities` based on their support.
> +    """
> +
> +    scattered_rx = partial(TestPmdShell.get_capas_rxq)
> diff --git a/dts/framework/runner.py b/dts/framework/runner.py
> index db8e3ba96b..7407ea30b8 100644
> --- a/dts/framework/runner.py
> +++ b/dts/framework/runner.py
> @@ -501,6 +501,12 @@ def _run_test_suites(
>          The method assumes the build target we're testing has already been built on the SUT node.
>          The current build target thus corresponds to the current DPDK build present on the SUT node.
>
> +        Before running any suites, the method determines whether they should be skipped
> +        by inspecting any required capabilities the test suite needs and comparing those
> +        to capabilities supported by the tested NIC. If all capabilities are supported,
> +        the suite is run. If all test cases in a test suite would be skipped, the whole test suite
> +        is skipped (the setup and teardown is not run).
> +
>          If a blocking test suite (such as the smoke test suite) fails, the rest of the test suites
>          in the current build target won't be executed.
>
> @@ -512,10 +518,30 @@ def _run_test_suites(
>              test_suites_with_cases: The test suites with test cases to run.
>          """
>          end_build_target = False
> +        required_capabilities = set()
> +        supported_capabilities = set()
> +        for test_suite_with_cases in test_suites_with_cases:
> +            required_capabilities.update(test_suite_with_cases.req_capabilities)
> +        self._logger.debug(f"Found required capabilities: {required_capabilities}")
> +        if required_capabilities:
> +            supported_capabilities = sut_node.get_supported_capabilities(required_capabilities)
>          for test_suite_with_cases in test_suites_with_cases:
>              test_suite_result = build_target_result.add_test_suite(test_suite_with_cases)
> +            test_suite_with_cases.mark_skip(supported_capabilities)
>              try:
> -                self._run_test_suite(sut_node, tg_node, test_suite_result, test_suite_with_cases)
> +                if test_suite_with_cases:
> +                    self._run_test_suite(
> +                        sut_node,
> +                        tg_node,
> +                        test_suite_result,
> +                        test_suite_with_cases,
> +                    )
> +                else:
> +                    self._logger.info(
> +                        f"Test suite execution SKIPPED: "
> +                        f"{test_suite_with_cases.test_suite_class.__name__}"
> +                    )
> +                    test_suite_result.update_setup(Result.SKIP)
>              except BlockingTestSuiteError as e:
>                  self._logger.exception(
>                      f"An error occurred within {test_suite_with_cases.test_suite_class.__name__}. "
> @@ -614,14 +640,18 @@ def _execute_test_suite(
>              test_case_result = test_suite_result.add_test_case(test_case_name)
>              all_attempts = SETTINGS.re_run + 1
>              attempt_nr = 1
> -            self._run_test_case(test_suite, test_case_method, test_case_result)
> -            while not test_case_result and attempt_nr < all_attempts:
> -                attempt_nr += 1
> -                self._logger.info(
> -                    f"Re-running FAILED test case '{test_case_name}'. "
> -                    f"Attempt number {attempt_nr} out of {all_attempts}."
> -                )
> +            if TestSuiteWithCases.should_not_be_skipped(test_case_method):
>                  self._run_test_case(test_suite, test_case_method, test_case_result)
> +                while not test_case_result and attempt_nr < all_attempts:
> +                    attempt_nr += 1
> +                    self._logger.info(
> +                        f"Re-running FAILED test case '{test_case_name}'. "
> +                        f"Attempt number {attempt_nr} out of {all_attempts}."
> +                    )
> +                    self._run_test_case(test_suite, test_case_method, test_case_result)
> +            else:
> +                self._logger.info(f"Test case execution SKIPPED: {test_case_name}")
> +                test_case_result.update_setup(Result.SKIP)
>
>      def _run_test_case(
>          self,
> diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
> index 28f84fd793..26c58a8606 100644
> --- a/dts/framework/test_result.py
> +++ b/dts/framework/test_result.py
> @@ -24,12 +24,14 @@
>  """
>
>  import os.path
> -from collections.abc import MutableSequence
> -from dataclasses import dataclass
> +from collections.abc import MutableSequence, MutableSet
> +from dataclasses import dataclass, field
>  from enum import Enum, auto
>  from types import MethodType
>  from typing import Union
>
> +from framework.remote_session import NicCapability
> +
>  from .config import (
>      OS,
>      Architecture,
> @@ -64,6 +66,14 @@ class is to hold a subset of test cases (which could be all test cases) because
>
>      test_suite_class: type[TestSuite]
>      test_cases: list[MethodType]
> +    req_capabilities: set[NicCapability] = field(default_factory=set, init=False)
> +
> +    def __post_init__(self):
> +        """Gather the required capabilities of the test suite and all test cases."""
> +        for test_object in [self.test_suite_class] + self.test_cases:
> +            test_object.skip = False
> +            if hasattr(test_object, "req_capa"):
> +                self.req_capabilities.update(test_object.req_capa)
>
>      def create_config(self) -> TestSuiteConfig:
>          """Generate a :class:`TestSuiteConfig` from the stored test suite with test cases.
> @@ -76,6 +86,47 @@ def create_config(self) -> TestSuiteConfig:
>              test_cases=[test_case.__name__ for test_case in self.test_cases],
>          )
>
> +    def mark_skip(self, supported_capabilities: MutableSet[NicCapability]) -> None:
> +        """Mark the test suite and test cases to be skipped.
> +
> +        The mark is applied is object to be skipped requires any capabilities and at least one of

"The mark is applied if the object to be skipped."


> +        them is not among `capabilities`.

Maybe change 'capabilities' to 'supported_capabilities.' Unless I'm
just misunderstanding the comment.

> +
> +        Args:
> +            supported_capabilities: The supported capabilities.
> +        """
> +        for test_object in [self.test_suite_class] + self.test_cases:
> +            if set(getattr(test_object, "req_capa", [])) - supported_capabilities:
> +                test_object.skip = True
> +
> +    @staticmethod
> +    def should_not_be_skipped(test_object: Union[type[TestSuite] | MethodType]) -> bool:
> +        """Figure out whether `test_object` should be skipped.
> +
> +        If `test_object` is a :class:`TestSuite`, its test cases are not checked,
> +        only the class itself.
> +
> +        Args:
> +            test_object: The test suite or test case to be inspected.
> +
> +        Returns:
> +            :data:`True` if the test suite or test case should be skipped, :data:`False` otherwise.
> +        """
> +        return not getattr(test_object, "skip", False)
> +
> +    def __bool__(self) -> bool:
> +        """The truth value is determined by whether the test suite should be run.
> +
> +        Returns:
> +            :data:`False` if the test suite should be skipped, :data:`True` otherwise.
> +        """
> +        found_test_case_to_run = False
> +        for test_case in self.test_cases:
> +            if self.should_not_be_skipped(test_case):
> +                found_test_case_to_run = True
> +                break
> +        return found_test_case_to_run and self.should_not_be_skipped(self.test_suite_class)
> +
>
>  class Result(Enum):
>      """The possible states that a setup, a teardown or a test case may end up in."""
> @@ -170,12 +221,13 @@ def update_setup(self, result: Result, error: Exception | None = None) -> None:
>          self.setup_result.result = result
>          self.setup_result.error = error
>
> -        if result in [Result.BLOCK, Result.ERROR, Result.FAIL]:
> -            self.update_teardown(Result.BLOCK)
> -            self._block_result()
> +        if result != Result.PASS:
> +            result_to_mark = Result.BLOCK if result != Result.SKIP else Result.SKIP
> +            self.update_teardown(result_to_mark)
> +            self._mark_results(result_to_mark)
>
> -    def _block_result(self) -> None:
> -        r"""Mark the result as :attr:`~Result.BLOCK`\ed.
> +    def _mark_results(self, result) -> None:
> +        """Mark the result as well as the child result as `result`.
>
>          The blocking of child results should be done in overloaded methods.
>          """
> @@ -390,11 +442,11 @@ def add_sut_info(self, sut_info: NodeInfo) -> None:
>          self.sut_os_version = sut_info.os_version
>          self.sut_kernel_version = sut_info.kernel_version
>
> -    def _block_result(self) -> None:
> -        r"""Mark the result as :attr:`~Result.BLOCK`\ed."""
> +    def _mark_results(self, result) -> None:
> +        """Mark the result as well as the child result as `result`."""
>          for build_target in self._config.build_targets:
>              child_result = self.add_build_target(build_target)
> -            child_result.update_setup(Result.BLOCK)
> +            child_result.update_setup(result)
>
>
>  class BuildTargetResult(BaseResult):
> @@ -464,11 +516,11 @@ def add_build_target_info(self, versions: BuildTargetInfo) -> None:
>          self.compiler_version = versions.compiler_version
>          self.dpdk_version = versions.dpdk_version
>
> -    def _block_result(self) -> None:
> -        r"""Mark the result as :attr:`~Result.BLOCK`\ed."""
> +    def _mark_results(self, result) -> None:
> +        """Mark the result as well as the child result as `result`."""
>          for test_suite_with_cases in self._test_suites_with_cases:
>              child_result = self.add_test_suite(test_suite_with_cases)
> -            child_result.update_setup(Result.BLOCK)
> +            child_result.update_setup(result)
>
>
>  class TestSuiteResult(BaseResult):
> @@ -508,11 +560,11 @@ def add_test_case(self, test_case_name: str) -> "TestCaseResult":
>          self.child_results.append(result)
>          return result
>
> -    def _block_result(self) -> None:
> -        r"""Mark the result as :attr:`~Result.BLOCK`\ed."""
> +    def _mark_results(self, result) -> None:
> +        """Mark the result as well as the child result as `result`."""
>          for test_case_method in self._test_suite_with_cases.test_cases:
>              child_result = self.add_test_case(test_case_method.__name__)
> -            child_result.update_setup(Result.BLOCK)
> +            child_result.update_setup(result)
>
>
>  class TestCaseResult(BaseResult, FixtureResult):
> @@ -566,9 +618,9 @@ def add_stats(self, statistics: "Statistics") -> None:
>          """
>          statistics += self.result
>
> -    def _block_result(self) -> None:
> -        r"""Mark the result as :attr:`~Result.BLOCK`\ed."""
> -        self.update(Result.BLOCK)
> +    def _mark_results(self, result) -> None:
> +        r"""Mark the result as `result`."""
> +        self.update(result)
>
>      def __bool__(self) -> bool:
>          """The test case passed only if setup, teardown and the test case itself passed."""
> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
> index 9c3b516002..07cdd294b9 100644
> --- a/dts/framework/test_suite.py
> +++ b/dts/framework/test_suite.py
> @@ -13,6 +13,7 @@
>      * Test case verification.
>  """
>
> +from collections.abc import Callable
>  from ipaddress import IPv4Interface, IPv6Interface, ip_interface
>  from typing import ClassVar, Union
>
> @@ -20,6 +21,8 @@
>  from scapy.layers.l2 import Ether  # type: ignore[import]
>  from scapy.packet import Packet, Padding  # type: ignore[import]
>
> +from framework.remote_session import NicCapability
> +
>  from .exception import TestCaseVerifyError
>  from .logger import DTSLogger, get_dts_logger
>  from .testbed_model import Port, PortLink, SutNode, TGNode
> @@ -62,6 +65,7 @@ class TestSuite(object):
>      #: Whether the test suite is blocking. A failure of a blocking test suite
>      #: will block the execution of all subsequent test suites in the current build target.
>      is_blocking: ClassVar[bool] = False
> +    skip: bool
>      _logger: DTSLogger
>      _port_links: list[PortLink]
>      _sut_port_ingress: Port
> @@ -89,6 +93,7 @@ def __init__(
>          """
>          self.sut_node = sut_node
>          self.tg_node = tg_node
> +        self.skip = False
>          self._logger = get_dts_logger(self.__class__.__name__)
>          self._port_links = []
>          self._process_links()
> @@ -360,3 +365,23 @@ def _verify_l3_packet(self, received_packet: IP, expected_packet: IP) -> bool:
>          if received_packet.src != expected_packet.src or received_packet.dst != expected_packet.dst:
>              return False
>          return True
> +
> +
> +def requires(capability: NicCapability) -> Callable:
> +    """A decorator that marks the decorated test case or test suite as one to be skipped.

I think there might be an error here. Are you trying to say "A
decorator that marks the test case/test suite as having additional
requirements" ?

> +
> +    Args:
> +        The capability that's required by the decorated test case or test suite.
> +
> +    Returns:
> +        The decorated function.
> +    """
> +
> +    def add_req_capa(func) -> Callable:
> +        if hasattr(func, "req_capa"):
> +            func.req_capa.append(capability)
> +        else:
> +            func.req_capa = [capability]
> +        return func
> +
> +    return add_req_capa
> diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/testbed_model/sut_node.py
> index 97aa26d419..1fb536735d 100644
> --- a/dts/framework/testbed_model/sut_node.py
> +++ b/dts/framework/testbed_model/sut_node.py
> @@ -15,7 +15,7 @@
>  import tarfile
>  import time
>  from pathlib import PurePath
> -from typing import Type
> +from typing import Iterable, Type
>
>  from framework.config import (
>      BuildTargetConfiguration,
> @@ -23,7 +23,7 @@
>      NodeInfo,
>      SutNodeConfiguration,
>  )
> -from framework.remote_session import CommandResult
> +from framework.remote_session import CommandResult, NicCapability, TestPmdShell
>  from framework.settings import SETTINGS
>  from framework.utils import MesonArgs
>
> @@ -228,6 +228,27 @@ def get_build_target_info(self) -> BuildTargetInfo:
>      def _guess_dpdk_remote_dir(self) -> PurePath:
>          return self.main_session.guess_dpdk_remote_dir(self._remote_tmp_dir)
>
> +    def get_supported_capabilities(
> +        self, capabilities: Iterable[NicCapability]
> +    ) -> set[NicCapability]:
> +        """Get the supported capabilities of the current NIC from `capabilities`.

I wonder if it might be more readable to change the 'capabilities'
variable to 'required_capabilities' or something along those lines.
Although I do understand why you have selected 'capabilities' in the
first place if the capabilities being passed in may not necessarily be
required capabilities 100% of the time.

> +
> +        Args:
> +            capabilities: The capabilities to verify.
> +
> +        Returns:
> +            The set of supported capabilities of the current NIC.
> +        """
> +        supported_capas: set[NicCapability] = set()
> +        unsupported_capas: set[NicCapability] = set()
> +        self._logger.debug(f"Checking which capabilities from {capabilities} NIC are supported.")
> +        testpmd_shell = self.create_interactive_shell(TestPmdShell, privileged=True)
> +        for capability in capabilities:
> +            if capability not in supported_capas or capability not in unsupported_capas:
> +                capability.value(testpmd_shell, supported_capas, unsupported_capas)
> +        del testpmd_shell
> +        return supported_capas
> +
>      def _set_up_build_target(self, build_target_config: BuildTargetConfiguration) -> None:
>          """Setup DPDK on the SUT node.
>
> diff --git a/dts/tests/TestSuite_hello_world.py b/dts/tests/TestSuite_hello_world.py
> index fd7ff1534d..31b1564582 100644
> --- a/dts/tests/TestSuite_hello_world.py
> +++ b/dts/tests/TestSuite_hello_world.py
> @@ -7,7 +7,8 @@
>  No other EAL parameters apart from cores are used.
>  """
>
> -from framework.test_suite import TestSuite
> +from framework.remote_session import NicCapability
> +from framework.test_suite import TestSuite, requires
>  from framework.testbed_model import (
>      LogicalCoreCount,
>      LogicalCoreCountFilter,
> @@ -26,6 +27,7 @@ def set_up_suite(self) -> None:
>          """
>          self.app_helloworld_path = self.sut_node.build_dpdk_app("helloworld")
>
> +    @requires(NicCapability.scattered_rx)
>      def test_hello_world_single_core(self) -> None:
>          """Single core test case.
>
> --
> 2.34.1
>

The above comments are basically just nitpicks, but if nothing else, I
figured I'd bring it to your attention.