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.lin...@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.lin...@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.
