* [RFC PATCH v1] dts: skip test cases based on capabilities
@ 2024-03-01 15:54 Juraj Linkeš
2024-04-11 8:48 ` [RFC PATCH v2] " Juraj Linkeš
0 siblings, 1 reply; 14+ messages in thread
From: Juraj Linkeš @ 2024-03-01 15:54 UTC (permalink / raw)
To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
Luca.Vizzarro, npratte
Cc: dev, Juraj Linkeš
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@pantheon.tech>
---
dts/framework/remote_session/__init__.py | 2 +-
dts/framework/remote_session/testpmd_shell.py | 48 ++++++++++
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, 209 insertions(+), 31 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 0184cc2e71..71379d54b8 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -15,6 +15,9 @@
testpmd_shell.close()
"""
+from collections.abc import MutableSet
+from enum import Enum
+from functools import partial
from pathlib import PurePath
from typing import Callable, ClassVar
@@ -82,3 +85,48 @@ def get_devices(self) -> list[TestPmdDevice]:
if "device name:" in line.lower():
dev_list.append(TestPmdDevice(line))
return dev_list
+
+ 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.
+ """
+ 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)
+
+ def close(self) -> None:
+ """Send the quit command to close testpmd."""
+ self.send_command("quit", "Bye...")
+ super().close()
+
+
+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.
+ 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
+ them is not among `capabilities`.
+
+ 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 1957ea7328..1d53db092f 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
@@ -61,6 +64,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
@@ -88,6 +92,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()
@@ -349,3 +354,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.
+
+ 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 c4acea38d1..943836a051 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
@@ -223,6 +223,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`.
+
+ 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH v2] dts: skip test cases based on capabilities
2024-03-01 15:54 [RFC PATCH v1] dts: skip test cases based on capabilities Juraj Linkeš
@ 2024-04-11 8:48 ` Juraj Linkeš
2024-05-21 15:47 ` Luca Vizzarro
` (4 more replies)
0 siblings, 5 replies; 14+ messages in thread
From: Juraj Linkeš @ 2024-04-11 8:48 UTC (permalink / raw)
To: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
Luca.Vizzarro, npratte
Cc: dev, Juraj Linkeš
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@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.
+ """
+ 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.
+ 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
+ them is not among `capabilities`.
+
+ 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.
+
+ 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`.
+
+ 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-04-11 8:48 ` [RFC PATCH v2] " Juraj Linkeš
@ 2024-05-21 15:47 ` Luca Vizzarro
2024-05-22 14:58 ` Luca Vizzarro
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Luca Vizzarro @ 2024-05-21 15:47 UTC (permalink / raw)
To: Juraj Linkeš,
thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
npratte
Cc: dev
On 11/04/2024 09:48, Juraj Linkeš wrote:
> + 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.
> + """
> + 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)
It doesn't look like this works in normal condition. I've noticed that
this appears as "on" if I set --max-pkt-len=9000 on the E810-C.
Otherwise it's off... and with Jeremy's patch based on this, the
pmd_buffer_scatter test gets skipped when it's supported.
Apart from this, everything else seems to work as expected. I'll send a
review of the code as soon as possible.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-04-11 8:48 ` [RFC PATCH v2] " Juraj Linkeš
2024-05-21 15:47 ` Luca Vizzarro
@ 2024-05-22 14:58 ` Luca Vizzarro
2024-06-07 13:13 ` Juraj Linkeš
2024-05-24 20:51 ` Nicholas Pratte
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Luca Vizzarro @ 2024-05-22 14:58 UTC (permalink / raw)
To: Juraj Linkeš,
thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
npratte
Cc: dev
Hi Juraj,
Here's my review. Excuse me for the unordinary format, but I thought
it would have just been easier to convey my suggestions through code.
Apart from the smaller suggestions, the most important one I think is
that we should make sure to enforce type checking (and hinting).
Overall I like your approach, but I think it'd be better to initialise
all the required variables per test case, so we can access them
directly without doing checks everytime. The easiest approach I can see
to do this though, is to decorate all the test cases, for example
through @test. It'd be a somewhat important change as it changes the
test writing API, but it brings some improvements while making the
system more resilient.
The comments in the code are part of the review and may refer to
either your code or mine. The diff is in working order, so you could
test the functionality if you wished.
Best regards,
Luca
---
diff --git a/dts/framework/remote_session/__init__.py b/dts/framework/remote_session/__init__.py
index f18a9f2259..d4dfed3a58 100644
--- a/dts/framework/remote_session/__init__.py
+++ b/dts/framework/remote_session/__init__.py
@@ -22,6 +22,9 @@
from .python_shell import PythonShell
from .remote_session import CommandResult, RemoteSession
from .ssh_session import SSHSession
+
+# in my testpmd params series these imports are removed as they promote eager module loading,
+# significantly increasing the chances of circular dependencies
from .testpmd_shell import NicCapability, TestPmdShell
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index f6783af621..2b87e2e5c8 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -16,7 +16,6 @@
"""
import time
-from collections.abc import MutableSet
from enum import Enum, auto
from functools import partial
from pathlib import PurePath
@@ -232,9 +231,8 @@ def close(self) -> None:
self.send_command("quit", "")
return super().close()
- def get_capas_rxq(
- self, supported_capabilities: MutableSet, unsupported_capabilities: MutableSet
- ) -> None:
+ # the built-in `set` is a mutable set. Is there an advantage to using MutableSet?
+ def get_capas_rxq(self, supported_capabilities: set, unsupported_capabilities: set) -> None:
"""Get all rxq capabilities and divide them into supported and unsupported.
Args:
@@ -243,6 +241,7 @@ def get_capas_rxq(
not supported will be stored.
"""
self._logger.debug("Getting rxq capabilities.")
+ # this assumes that the used ports are all the same. Could this be of concern?
command = "show rxq info 0 0"
rxq_info = self.send_command(command)
for line in rxq_info.split("\n"):
@@ -270,4 +269,6 @@ class NicCapability(Enum):
`unsupported_capabilities` based on their support.
"""
+ # partial is just a high-order function that pre-fills the arguments... but we have no arguments
+ # here? Was this intentional?
scattered_rx = partial(TestPmdShell.get_capas_rxq)
diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index 7407ea30b8..db02735ee9 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -20,12 +20,13 @@
import importlib
import inspect
import os
-import re
import sys
from pathlib import Path
from types import MethodType
from typing import Iterable, Sequence
+from framework.remote_session.testpmd_shell import NicCapability
+
from .config import (
BuildTargetConfiguration,
Configuration,
@@ -50,7 +51,7 @@
TestSuiteResult,
TestSuiteWithCases,
)
-from .test_suite import TestSuite
+from .test_suite import TestCase, TestCaseType, TestSuite
from .testbed_model import SutNode, TGNode
@@ -305,7 +306,7 @@ def is_test_suite(object) -> bool:
def _filter_test_cases(
self, test_suite_class: type[TestSuite], test_cases_to_run: Sequence[str]
- ) -> tuple[list[MethodType], list[MethodType]]:
+ ) -> tuple[list[type[TestCase]], list[type[TestCase]]]:
"""Filter `test_cases_to_run` from `test_suite_class`.
There are two rounds of filtering if `test_cases_to_run` is not empty.
@@ -327,30 +328,28 @@ def _filter_test_cases(
"""
func_test_cases = []
perf_test_cases = []
- name_method_tuples = inspect.getmembers(test_suite_class, inspect.isfunction)
+ # By introducing the TestCase class this could be simplified.
+ # Also adding separation of concerns, delegating the auto discovery of test cases to the
+ # test suite class.
if test_cases_to_run:
- name_method_tuples = [
- (name, method) for name, method in name_method_tuples if name in test_cases_to_run
- ]
- if len(name_method_tuples) < len(test_cases_to_run):
+ test_cases = test_suite_class.get_test_cases(lambda t: t.__name__ in test_cases_to_run)
+ if len(test_cases) < len(test_cases_to_run):
missing_test_cases = set(test_cases_to_run) - {
- name for name, _ in name_method_tuples
+ test_case.__name__ for test_case in test_cases
}
raise ConfigurationError(
f"Test cases {missing_test_cases} not found among methods "
f"of {test_suite_class.__name__}."
)
+ else:
+ test_cases = test_suite_class.get_test_cases()
- for test_case_name, test_case_method in name_method_tuples:
- if re.match(self._func_test_case_regex, test_case_name):
- func_test_cases.append(test_case_method)
- elif re.match(self._perf_test_case_regex, test_case_name):
- perf_test_cases.append(test_case_method)
- elif test_cases_to_run:
- raise ConfigurationError(
- f"Method '{test_case_name}' matches neither "
- f"a functional nor a performance test case name."
- )
+ for test_case in test_cases:
+ match test_case.type:
+ case TestCaseType.PERFORMANCE:
+ perf_test_cases.append(test_case)
+ case TestCaseType.FUNCTIONAL:
+ func_test_cases.append(test_case)
return func_test_cases, perf_test_cases
@@ -489,6 +488,17 @@ def _run_build_target(
self._logger.exception("Build target teardown failed.")
build_target_result.update_teardown(Result.FAIL, e)
+ def _get_supported_required_capabilities(
+ self, sut_node: SutNode, test_suites_with_cases: Iterable[TestSuiteWithCases]
+ ) -> set[NicCapability]:
+ required_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:
+ return sut_node.get_supported_capabilities(required_capabilities)
+ return set()
+
def _run_test_suites(
self,
sut_node: SutNode,
@@ -518,18 +528,17 @@ 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)
+ # extract this logic
+ supported_capabilities = self._get_supported_required_capabilities(
+ sut_node, test_suites_with_cases
+ )
for test_suite_with_cases in test_suites_with_cases:
test_suite_result = build_target_result.add_test_suite(test_suite_with_cases)
+ # not against this but perhaps something more explanatory like mark_skip_to_unsupported?
test_suite_with_cases.mark_skip(supported_capabilities)
try:
- if test_suite_with_cases:
+ # is_any_supported is more self-explanatory than an ambiguous boolean check
+ if test_suite_with_cases.is_any_supported():
self._run_test_suite(
sut_node,
tg_node,
@@ -619,7 +628,7 @@ def _run_test_suite(
def _execute_test_suite(
self,
test_suite: TestSuite,
- test_cases: Iterable[MethodType],
+ test_cases: Iterable[type[TestCase]],
test_suite_result: TestSuiteResult,
) -> None:
"""Execute all `test_cases` in `test_suite`.
@@ -640,7 +649,7 @@ 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
- if TestSuiteWithCases.should_not_be_skipped(test_case_method):
+ if not test_case_method.skip:
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
@@ -656,7 +665,7 @@ def _execute_test_suite(
def _run_test_case(
self,
test_suite: TestSuite,
- test_case_method: MethodType,
+ test_case_method: type[TestCase],
test_case_result: TestCaseResult,
) -> None:
"""Setup, execute and teardown `test_case_method` from `test_suite`.
@@ -702,7 +711,7 @@ def _run_test_case(
def _execute_test_case(
self,
test_suite: TestSuite,
- test_case_method: MethodType,
+ test_case_method: type[TestCase],
test_case_result: TestCaseResult,
) -> None:
"""Execute `test_case_method` from `test_suite`, record the result and handle failures.
@@ -716,7 +725,8 @@ def _execute_test_case(
test_case_name = test_case_method.__name__
try:
self._logger.info(f"Starting test case execution: {test_case_name}")
- test_case_method(test_suite)
+ # Explicit method binding is now required, otherwise mypy complains
+ MethodType(test_case_method, test_suite)()
test_case_result.update(Result.PASS)
self._logger.info(f"Test case execution PASSED: {test_case_name}")
diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
index 26c58a8606..82b2dc17b8 100644
--- a/dts/framework/test_result.py
+++ b/dts/framework/test_result.py
@@ -24,10 +24,9 @@
"""
import os.path
-from collections.abc import MutableSequence, MutableSet
+from collections.abc import MutableSequence
from dataclasses import dataclass, field
from enum import Enum, auto
-from types import MethodType
from typing import Union
from framework.remote_session import NicCapability
@@ -46,7 +45,7 @@
from .exception import DTSError, ErrorSeverity
from .logger import DTSLogger
from .settings import SETTINGS
-from .test_suite import TestSuite
+from .test_suite import TestCase, TestSuite
@dataclass(slots=True, frozen=True)
@@ -65,7 +64,7 @@ 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]
+ test_cases: list[type[TestCase]]
req_capabilities: set[NicCapability] = field(default_factory=set, init=False)
def __post_init__(self):
@@ -86,7 +85,7 @@ 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:
+ def mark_skip(self, supported_capabilities: set[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
@@ -95,26 +94,15 @@ def mark_skip(self, supported_capabilities: MutableSet[NicCapability]) -> None:
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:
+ for test_object in [self.test_suite_class, *self.test_cases]:
+ # mypy picks up that both TestSuite and TestCase implement RequiresCapabilities and is
+ # happy. We could explicitly type hint the list for readability if we preferred.
+ if test_object.required_capabilities - supported_capabilities:
+ # the latest version of mypy complains about this in the original code because
+ # test_object can be distinct classes. The added protocol solves this
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:
+ def is_any_supported(self) -> bool:
"""The truth value is determined by whether the test suite should be run.
Returns:
@@ -122,10 +110,12 @@ def __bool__(self) -> bool:
"""
found_test_case_to_run = False
for test_case in self.test_cases:
- if self.should_not_be_skipped(test_case):
+ # mypy and the TestCase decorators now ensure that the attributes always exist
+ # so the above static method is no longer needed
+ if not test_case.skip:
found_test_case_to_run = True
break
- return found_test_case_to_run and self.should_not_be_skipped(self.test_suite_class)
+ return found_test_case_to_run and not self.test_suite_class.skip
class Result(Enum):
diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 07cdd294b9..d03f8db712 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -13,9 +13,10 @@
* Test case verification.
"""
-from collections.abc import Callable
+from enum import Enum, auto
+import inspect
from ipaddress import IPv4Interface, IPv6Interface, ip_interface
-from typing import ClassVar, Union
+from typing import Callable, ClassVar, Protocol, TypeVar, Union, cast
from scapy.layers.inet import IP # type: ignore[import]
from scapy.layers.l2 import Ether # type: ignore[import]
@@ -30,7 +31,14 @@
from .utils import get_packet_summaries
-class TestSuite(object):
+# a protocol/interface for common fields. The defaults are correctly picked up
+# by child concrete classes.
+class RequiresCapabilities(Protocol):
+ skip: ClassVar[bool] = False
+ required_capabilities: ClassVar[set[NicCapability]] = set()
+
+
+class TestSuite(RequiresCapabilities):
"""The base class with building blocks needed by most test cases.
* Test suite setup/cleanup methods to override,
@@ -65,7 +73,6 @@ 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
@@ -93,7 +100,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()
@@ -110,6 +117,23 @@ def __init__(
self._tg_ip_address_egress = ip_interface("192.168.100.3/24")
self._tg_ip_address_ingress = ip_interface("192.168.101.3/24")
+ # move the discovery of the test cases to TestSuite for separation of concerns
+ @classmethod
+ def get_test_cases(
+ cls, filter: Callable[[type["TestCase"]], bool] | None = None
+ ) -> set[type["TestCase"]]:
+ test_cases = set()
+ for _, method in inspect.getmembers(cls, inspect.isfunction):
+ # at runtime the function remains a function with custom attributes. An ininstance check
+ # unfortunately wouldn't work. Therefore force and test for the presence of TestCaseType
+ test_case = cast(type[TestCase], method)
+ try:
+ if isinstance(test_case.type, TestCaseType) and (not filter or filter(test_case)):
+ test_cases.add(test_case)
+ except AttributeError:
+ pass
+ return test_cases
+
def _process_links(self) -> None:
"""Construct links between SUT and TG ports."""
for sut_port in self.sut_node.ports:
@@ -367,8 +391,46 @@ def _verify_l3_packet(self, received_packet: IP, expected_packet: IP) -> bool:
return True
-def requires(capability: NicCapability) -> Callable:
- """A decorator that marks the decorated test case or test suite as one to be skipped.
+# generic type for a method of an instance of TestSuite
+M = TypeVar("M", bound=Callable[[TestSuite], None])
+
+
+class TestCaseType(Enum):
+ FUNCTIONAL = auto()
+ PERFORMANCE = auto()
+
+
+# the protocol here is merely for static type checking and hinting.
+# the defaults don't get applied to functions like it does with inheriting
+# classes. The cast here is necessary as we are forcing mypy to understand
+# we want to treat the function as TestCase. When all the attributes are correctly
+# initialised this is fine.
+class TestCase(RequiresCapabilities, Protocol[M]):
+ type: ClassVar[TestCaseType]
+ __call__: M # necessary for mypy so that it can treat this class as the function it's shadowing
+
+ @staticmethod
+ def make_decorator(test_case_type: TestCaseType):
+ def _decorator(func: M) -> type[TestCase]:
+ test_case = cast(type[TestCase], func)
+ test_case.skip = False
+ test_case.required_capabilities = set()
+ test_case.type = test_case_type
+ return test_case
+
+ return _decorator
+
+
+# this now requires to tag all test cases with the following decorators and the advantage of:
+# - a cleaner auto discovery
+# - enforcing the TestCase type
+# - initialising all the required attributes for test cases
+test = TestCase.make_decorator(TestCaseType.FUNCTIONAL)
+perf_test = TestCase.make_decorator(TestCaseType.PERFORMANCE)
+
+
+def requires(*capabilities: NicCapability):
+ """A decorator that marks the decorated test case or test suite as skippable.
Args:
The capability that's required by the decorated test case or test suite.
@@ -377,11 +439,8 @@ def requires(capability: NicCapability) -> Callable:
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
+ def add_req_capa(test_case: type[TestCase]) -> type[TestCase]:
+ test_case.required_capabilities.update(capabilities)
+ return test_case
return add_req_capa
diff --git a/dts/tests/TestSuite_hello_world.py b/dts/tests/TestSuite_hello_world.py
index 31b1564582..61533665f8 100644
--- a/dts/tests/TestSuite_hello_world.py
+++ b/dts/tests/TestSuite_hello_world.py
@@ -8,7 +8,7 @@
"""
from framework.remote_session import NicCapability
-from framework.test_suite import TestSuite, requires
+from framework.test_suite import TestSuite, requires, test
from framework.testbed_model import (
LogicalCoreCount,
LogicalCoreCountFilter,
@@ -28,7 +28,8 @@ 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:
+ @test
+ def hello_world_single_core(self) -> None:
"""Single core test case.
Steps:
@@ -47,7 +48,8 @@ def test_hello_world_single_core(self) -> None:
f"helloworld didn't start on lcore{lcores[0]}",
)
- def test_hello_world_all_cores(self) -> None:
+ @test
+ def hello_world_all_cores(self) -> None:
"""All cores test case.
Steps:
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-04-11 8:48 ` [RFC PATCH v2] " Juraj Linkeš
2024-05-21 15:47 ` Luca Vizzarro
2024-05-22 14:58 ` Luca Vizzarro
@ 2024-05-24 20:51 ` Nicholas Pratte
2024-05-31 16:44 ` Luca Vizzarro
2024-06-03 14:40 ` Nicholas Pratte
4 siblings, 0 replies; 14+ messages in thread
From: Nicholas Pratte @ 2024-05-24 20:51 UTC (permalink / raw)
To: Juraj Linkeš
Cc: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
Luca.Vizzarro, dev
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@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@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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-04-11 8:48 ` [RFC PATCH v2] " Juraj Linkeš
` (2 preceding siblings ...)
2024-05-24 20:51 ` Nicholas Pratte
@ 2024-05-31 16:44 ` Luca Vizzarro
2024-06-05 13:55 ` Patrick Robb
2024-06-03 14:40 ` Nicholas Pratte
4 siblings, 1 reply; 14+ messages in thread
From: Luca Vizzarro @ 2024-05-31 16:44 UTC (permalink / raw)
To: Juraj Linkeš,
thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
npratte
Cc: dev
Hi again Juraj,
sorry for yet another comment!
On 11/04/2024 09:48, Juraj Linkeš wrote:
> + 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.
> + """
> + self._logger.debug("Getting rxq capabilities.")
> + command = "show rxq info 0 0"
In my testing of Jeremy's patches which depend on this one ("Add second
scatter test case"), I've discovered that the Intel E810-C NIC I am
using to test does not automatically show "RX scattered packets: on".
But I've noticed it does if the MTU is set to something big like 9000.
I've tested a change of this by adding:
self.set_port_mtu(0, 9000)
> + rxq_info = self.send_command(command)
self.set_port_mtu(1, 9000)
And it seems to work alright. I've also tested this specific change with
Mellanox NICs and it didn't seem to affect them at all. No errors or
problems and they still showed "RX scattered packets: off" as expected.
The `set_port_mtu` method comes from Jeremy's patch...
> + 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)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-04-11 8:48 ` [RFC PATCH v2] " Juraj Linkeš
` (3 preceding siblings ...)
2024-05-31 16:44 ` Luca Vizzarro
@ 2024-06-03 14:40 ` Nicholas Pratte
2024-06-07 13:20 ` Juraj Linkeš
4 siblings, 1 reply; 14+ messages in thread
From: Nicholas Pratte @ 2024-06-03 14:40 UTC (permalink / raw)
To: Juraj Linkeš
Cc: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
Luca.Vizzarro, dev
I was able to use this implementation on the in-development
jumboframes suite, setting restrictions on the required link speeds of
NICs and using this as a requirement to run all test cases. While the
framework you've developed is intuitive for true/false capabilities,
this may not be the case for other device capabilities such as link
speed, where perhaps someone might want to support a certain range of
speeds (I also acknowledge that this may be a needless feature). I
personally found implementing this to be a head-scratcher, and I
ultimately ended up implementing this using a lower bound link speed
instead of accepting a range of speeds. The reason for me implementing
this at all is because of some complications within old DTS's
jumboframes implementation. In old DTS, the test suite would check for
1GB NICs within certain test cases and modify the MTU lengths because
of some inconsistent logic. You can see what I am referring to in the
link below, take a look at test_jumboframes_bigger_jumbo, if you are
interested.
https://git.dpdk.org/tools/dts/tree/tests/TestSuite_jumboframes.py
A solution to this problem is to set a restriction on the speed of
NICs for the test suite, but whether or not this is a viable solution
may require further discussion. This issue is its own conversation,
but I'm bringing it up in this thread since we may run into
requirements issues like this in the future, but I'm not so sure what
the rest of you guys think, or if you guys think it is a viable
concern at all.
On Thu, Apr 11, 2024 at 4:48 AM Juraj Linkeš <juraj.linkes@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@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.
> + """
> + 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.
> + 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
> + them is not among `capabilities`.
> +
> + 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.
> +
> + 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`.
> +
> + 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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-05-31 16:44 ` Luca Vizzarro
@ 2024-06-05 13:55 ` Patrick Robb
2024-06-06 13:36 ` Jeremy Spewock
0 siblings, 1 reply; 14+ messages in thread
From: Patrick Robb @ 2024-06-05 13:55 UTC (permalink / raw)
To: jspewock
Cc: Juraj Linkeš,
thomas, Honnappa.Nagarahalli, paul.szczepanek, npratte, dev,
Luca Vizzarro
[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]
On Fri, May 31, 2024 at 12:44 PM Luca Vizzarro <Luca.Vizzarro@arm.com>
wrote:
>
> In my testing of Jeremy's patches which depend on this one ("Add second
> scatter test case"), I've discovered that the Intel E810-C NIC I am
> using to test does not automatically show "RX scattered packets: on".
> But I've noticed it does if the MTU is set to something big like 9000.
>
> I've tested a change of this by adding:
>
> self.set_port_mtu(0, 9000)
> > + rxq_info = self.send_command(command)
> self.set_port_mtu(1, 9000)
>
> And it seems to work alright. I've also tested this specific change with
> Mellanox NICs and it didn't seem to affect them at all. No errors or
> problems and they still showed "RX scattered packets: off" as expected.
>
> The `set_port_mtu` method comes from Jeremy's patch...
>
>
>
Hi Jeremy,
Sounds like Luca's way ahead of me here, but I wanted to note that I did
run from the capabilities patch + Jeremy's new Scatter patch, across these
NICs:
Mellanox CX5
Broadcom 57414 25G
Broadcom P2100G
Intel XL710 40G
And in call cases scatter_mbuf_2048 skips,
and scatter_mbuf_2048_with_offload runs.
The 2nd case passed in all cases, excluding the XL710 where it errors with
"Test pmd failed to set fwd mode to mac." I can double check that to ensure
there was no setup error on my part, but I think the more interesting part
is the skip on the non-offload testcase, as I recall Jeremy saying that the
XL710 was expected to natively support scatter and run the first testcase.
I can do a rerun, adding in the MTU modifier, and see if the same
adjustment happens as with the E810 as Luca describes.
[-- Attachment #2: Type: text/html, Size: 2305 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-06-05 13:55 ` Patrick Robb
@ 2024-06-06 13:36 ` Jeremy Spewock
0 siblings, 0 replies; 14+ messages in thread
From: Jeremy Spewock @ 2024-06-06 13:36 UTC (permalink / raw)
To: Patrick Robb
Cc: Juraj Linkeš,
thomas, Honnappa.Nagarahalli, paul.szczepanek, npratte, dev,
Luca Vizzarro
On Wed, Jun 5, 2024 at 9:55 AM Patrick Robb <probb@iol.unh.edu> wrote:
>
>
>
> On Fri, May 31, 2024 at 12:44 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>>
>>
>> In my testing of Jeremy's patches which depend on this one ("Add second
>> scatter test case"), I've discovered that the Intel E810-C NIC I am
>> using to test does not automatically show "RX scattered packets: on".
>> But I've noticed it does if the MTU is set to something big like 9000.
>>
>> I've tested a change of this by adding:
>>
>> self.set_port_mtu(0, 9000)
>> > + rxq_info = self.send_command(command)
>> self.set_port_mtu(1, 9000)
>>
>> And it seems to work alright. I've also tested this specific change with
>> Mellanox NICs and it didn't seem to affect them at all. No errors or
>> problems and they still showed "RX scattered packets: off" as expected.
>>
>> The `set_port_mtu` method comes from Jeremy's patch...
>>
>>
>
> Hi Jeremy,
>
> Sounds like Luca's way ahead of me here, but I wanted to note that I did run from the capabilities patch + Jeremy's new Scatter patch, across these NICs:
>
> Mellanox CX5
> Broadcom 57414 25G
> Broadcom P2100G
> Intel XL710 40G
>
> And in call cases scatter_mbuf_2048 skips, and scatter_mbuf_2048_with_offload runs.
>
> The 2nd case passed in all cases, excluding the XL710 where it errors with "Test pmd failed to set fwd mode to mac." I can double check that to ensure there was no setup error on my part, but I think the more interesting part is the skip on the non-offload testcase, as I recall Jeremy saying that the XL710 was expected to natively support scatter and run the first testcase.
The "failing to set forwarding mode" is strange, I assume this is
likely because of another issue that Luca noted which was some Intel
NICs taking longer to start testpmd and the timeout not being long
enough to allow for proper startup. If this is the case it would have
essentially "poisoned" the testpmd output buffer and caused all of the
verification steps of subsequent commands to fail. This should be
fixed in the new series of my patch that increases this timeout to 5
seconds.
>
> I can do a rerun, adding in the MTU modifier, and see if the same adjustment happens as with the E810 as Luca describes.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-05-22 14:58 ` Luca Vizzarro
@ 2024-06-07 13:13 ` Juraj Linkeš
2024-06-11 9:51 ` Luca Vizzarro
0 siblings, 1 reply; 14+ messages in thread
From: Juraj Linkeš @ 2024-06-07 13:13 UTC (permalink / raw)
To: Luca Vizzarro, thomas, Honnappa.Nagarahalli, jspewock, probb,
paul.szczepanek, npratte
Cc: dev
On 22. 5. 2024 16:58, Luca Vizzarro wrote:
> Hi Juraj,
>
> Here's my review. Excuse me for the unordinary format, but I thought
> it would have just been easier to convey my suggestions through code.
> Apart from the smaller suggestions, the most important one I think is
> that we should make sure to enforce type checking (and hinting).
> Overall I like your approach, but I think it'd be better to initialise
> all the required variables per test case, so we can access them
> directly without doing checks everytime. The easiest approach I can see
> to do this though, is to decorate all the test cases, for example
> through @test. It'd be a somewhat important change as it changes the
> test writing API, but it brings some improvements while making the
> system more resilient.
>
The format is a bit wonky, but I was able to figure it out. I answered
some of the questions I found.
I like the idea of decorating the test cases. I was thinking of
something similar in the past and now it makes sense to implement it.
I'll definitely use some (or all :-)) of the suggestions.
> The comments in the code are part of the review and may refer to
> either your code or mine. The diff is in working order, so you could
> test the functionality if you wished.
>
> Best regards,
> Luca
>
> ---
> diff --git a/dts/framework/remote_session/__init__.py
> b/dts/framework/remote_session/__init__.py
> index f18a9f2259..d4dfed3a58 100644
> --- a/dts/framework/remote_session/__init__.py
> +++ b/dts/framework/remote_session/__init__.py
> @@ -22,6 +22,9 @@
> from .python_shell import PythonShell
> from .remote_session import CommandResult, RemoteSession
> from .ssh_session import SSHSession
> +
> +# in my testpmd params series these imports are removed as they promote
> eager module loading,
> +# significantly increasing the chances of circular dependencies
> from .testpmd_shell import NicCapability, TestPmdShell
>
>
> diff --git a/dts/framework/remote_session/testpmd_shell.py
> b/dts/framework/remote_session/testpmd_shell.py
> index f6783af621..2b87e2e5c8 100644
> --- a/dts/framework/remote_session/testpmd_shell.py
> +++ b/dts/framework/remote_session/testpmd_shell.py
> @@ -16,7 +16,6 @@
> """
>
> import time
> -from collections.abc import MutableSet
> from enum import Enum, auto
> from functools import partial
> from pathlib import PurePath
> @@ -232,9 +231,8 @@ def close(self) -> None:
> self.send_command("quit", "")
> return super().close()
>
> - def get_capas_rxq(
> - self, supported_capabilities: MutableSet,
> unsupported_capabilities: MutableSet
> - ) -> None:
> + # the built-in `set` is a mutable set. Is there an advantage to
> using MutableSet?
From what I can tell, it's best practice to be as broad as possible
with input types. set is just one class, MutableSet could be any class
that's a mutable set.
> + def get_capas_rxq(self, supported_capabilities: set,
> unsupported_capabilities: set) -> None:
> """Get all rxq capabilities and divide them into supported and
> unsupported.
>
> Args:
> @@ -243,6 +241,7 @@ def get_capas_rxq(
> not supported will be stored.
> """
> self._logger.debug("Getting rxq capabilities.")
> + # this assumes that the used ports are all the same. Could this
> be of concern?
Our current assumption is one NIC per one execution, so this should be
safe, at least for now.
> command = "show rxq info 0 0"
> rxq_info = self.send_command(command)
> for line in rxq_info.split("\n"):
> @@ -270,4 +269,6 @@ class NicCapability(Enum):
> `unsupported_capabilities` based on their support.
> """
>
> + # partial is just a high-order function that pre-fills the
> arguments... but we have no arguments
> + # here? Was this intentional?
It's necessary because of the interaction between Enums and functions.
Without partial, accessing NicCapability.scattered_rx returns the
function instead of the enum.
> scattered_rx = partial(TestPmdShell.get_capas_rxq)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-06-03 14:40 ` Nicholas Pratte
@ 2024-06-07 13:20 ` Juraj Linkeš
0 siblings, 0 replies; 14+ messages in thread
From: Juraj Linkeš @ 2024-06-07 13:20 UTC (permalink / raw)
To: Nicholas Pratte
Cc: thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
Luca.Vizzarro, dev
On 3. 6. 2024 16:40, Nicholas Pratte wrote:
> I was able to use this implementation on the in-development
> jumboframes suite, setting restrictions on the required link speeds of
> NICs and using this as a requirement to run all test cases. While the
> framework you've developed is intuitive for true/false capabilities,
> this may not be the case for other device capabilities such as link
> speed, where perhaps someone might want to support a certain range of
> speeds (I also acknowledge that this may be a needless feature).
Would using the same decorator multiple times work? In any case, I will
think about this.
And also, thanks for the comments in the other thread, I'll incorporate
those.
> I
> personally found implementing this to be a head-scratcher, and I
> ultimately ended up implementing this using a lower bound link speed
> instead of accepting a range of speeds. The reason for me implementing
> this at all is because of some complications within old DTS's
> jumboframes implementation. In old DTS, the test suite would check for
> 1GB NICs within certain test cases and modify the MTU lengths because
> of some inconsistent logic. You can see what I am referring to in the
> link below, take a look at test_jumboframes_bigger_jumbo, if you are
> interested.
>
> https://git.dpdk.org/tools/dts/tree/tests/TestSuite_jumboframes.py
>
> A solution to this problem is to set a restriction on the speed of
> NICs for the test suite, but whether or not this is a viable solution
> may require further discussion. This issue is its own conversation,
> but I'm bringing it up in this thread since we may run into
> requirements issues like this in the future, but I'm not so sure what
> the rest of you guys think, or if you guys think it is a viable
> concern at all.
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-06-07 13:13 ` Juraj Linkeš
@ 2024-06-11 9:51 ` Luca Vizzarro
2024-06-12 9:15 ` Juraj Linkeš
0 siblings, 1 reply; 14+ messages in thread
From: Luca Vizzarro @ 2024-06-11 9:51 UTC (permalink / raw)
To: Juraj Linkeš,
thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
npratte
Cc: dev
While working on my blocklist patch, I've just realised I forgot to add
another comment. I think it would be ideal to make capabilities a
generic class, and NicCapability a child of this. When collecting
capabilities we could group these by the final class, and let this final
class create the environment to test the support. For example:
class Capability(ABC):
@staticmethod
@abstractmethod
def test_environment(node, capabilities):
"""Overridable"
class NicCapability(Capability):
def test_environment(node, capabilities):
shell = TestPmdShell(node)
# test capabilities against shell capabilities
Another thing that I don't remember if I pointed out, please let's use
complete names: required_capabilities instead of req_capas. I kept
forgetting what it meant. req commonly could mean "request". If you want
to use a widely used short version for capability, that's "cap",
although in a completely different context/meaning (hardware capabilities).
On 07/06/2024 14:13, Juraj Linkeš wrote:
>> - def get_capas_rxq(
>> - self, supported_capabilities: MutableSet,
>> unsupported_capabilities: MutableSet
>> - ) -> None:
>> + # the built-in `set` is a mutable set. Is there an advantage to
>> using MutableSet?
>
> From what I can tell, it's best practice to be as broad as possible
> with input types. set is just one class, MutableSet could be any class
> that's a mutable set.
Oh, yes! Great thinking. Didn't consider the usage of custom set
classes. Although, not sure if it'll ever be needed.
>> command = "show rxq info 0 0"
>> rxq_info = self.send_command(command)
>> for line in rxq_info.split("\n"):
>> @@ -270,4 +269,6 @@ class NicCapability(Enum):
>> `unsupported_capabilities` based on their support.
>> """
>>
>> + # partial is just a high-order function that pre-fills the
>> arguments... but we have no arguments
>> + # here? Was this intentional?
>
> It's necessary because of the interaction between Enums and functions.
> Without partial, accessing NicCapability.scattered_rx returns the
> function instead of the enum.
Oh interesting. Tested now and I see that it's not making it an enum
entry when done this way. I wonder why is this.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-06-11 9:51 ` Luca Vizzarro
@ 2024-06-12 9:15 ` Juraj Linkeš
2024-06-17 15:07 ` Luca Vizzarro
0 siblings, 1 reply; 14+ messages in thread
From: Juraj Linkeš @ 2024-06-12 9:15 UTC (permalink / raw)
To: Luca Vizzarro, thomas, Honnappa.Nagarahalli, jspewock, probb,
paul.szczepanek, npratte
Cc: dev
On 11. 6. 2024 11:51, Luca Vizzarro wrote:
> While working on my blocklist patch, I've just realised I forgot to add
> another comment. I think it would be ideal to make capabilities a
> generic class, and NicCapability a child of this. When collecting
> capabilities we could group these by the final class, and let this final
> class create the environment to test the support. For example:
>
I'm trying to wrap my head around this:
1. What do you mean group these by the final class?
2. What is this addressing or improving?
> class Capability(ABC):
> @staticmethod
> @abstractmethod
> def test_environment(node, capabilities):
> """Overridable"
>
> class NicCapability(Capability):
> def test_environment(node, capabilities):
> shell = TestPmdShell(node)
> # test capabilities against shell capabilities
>
I'm looking at this and I don't even know what to replace with this.
> Another thing that I don't remember if I pointed out, please let's use
> complete names: required_capabilities instead of req_capas. I kept
> forgetting what it meant. req commonly could mean "request". If you want
> to use a widely used short version for capability, that's "cap",
> although in a completely different context/meaning (hardware capabilities).
>
No problem.
> On 07/06/2024 14:13, Juraj Linkeš wrote:
>
>>> - def get_capas_rxq(
>>> - self, supported_capabilities: MutableSet,
>>> unsupported_capabilities: MutableSet
>>> - ) -> None:
>>> + # the built-in `set` is a mutable set. Is there an advantage to
>>> using MutableSet?
>>
>> From what I can tell, it's best practice to be as broad as possible
>> with input types. set is just one class, MutableSet could be any class
>> that's a mutable set.
>
> Oh, yes! Great thinking. Didn't consider the usage of custom set
> classes. Although, not sure if it'll ever be needed.
>
>>> command = "show rxq info 0 0"
>>> rxq_info = self.send_command(command)
>>> for line in rxq_info.split("\n"):
>>> @@ -270,4 +269,6 @@ class NicCapability(Enum):
>>> `unsupported_capabilities` based on their support.
>>> """
>>>
>>> + # partial is just a high-order function that pre-fills the
>>> arguments... but we have no arguments
>>> + # here? Was this intentional?
>>
>> It's necessary because of the interaction between Enums and functions.
>> Without partial, accessing NicCapability.scattered_rx returns the
>> function instead of the enum.
>
> Oh interesting. Tested now and I see that it's not making it an enum
> entry when done this way. I wonder why is this.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH v2] dts: skip test cases based on capabilities
2024-06-12 9:15 ` Juraj Linkeš
@ 2024-06-17 15:07 ` Luca Vizzarro
0 siblings, 0 replies; 14+ messages in thread
From: Luca Vizzarro @ 2024-06-17 15:07 UTC (permalink / raw)
To: Juraj Linkeš,
thomas, Honnappa.Nagarahalli, jspewock, probb, paul.szczepanek,
npratte
Cc: dev
On 12/06/2024 10:15, Juraj Linkeš wrote:
> On 11. 6. 2024 11:51, Luca Vizzarro wrote:
>> While working on my blocklist patch, I've just realised I forgot to
>> add another comment. I think it would be ideal to make capabilities a
>> generic class, and NicCapability a child of this. When collecting
>> capabilities we could group these by the final class, and let this
>> final class create the environment to test the support. For example:
>>
>
> I'm trying to wrap my head around this:
> 1. What do you mean group these by the final class?
The ultimate child of Capability, so NicCapability.
> 2. What is this addressing or improving?
It renders requirements more generic than just having to do with
testpmd. For example for the blocklist we could have a different kind of
"capability" and we could do:
@requires(TestbedCapability.min_port(2))
class TestSuite_blocklist(TestSuite):
Given the specific test requires a minimum of 2 ports. Albeit redundant
for DTS as we are always assuming minimum 2 ports right now, but it's an
example.
>> class Capability(ABC):
>> @staticmethod
>> @abstractmethod
>> def test_environment(node, capabilities):
>> """Overridable"
>>
>> class NicCapability(Capability):
>> def test_environment(node, capabilities):
>> shell = TestPmdShell(node)
>> # test capabilities against shell capabilities
>>
>
> I'm looking at this and I don't even know what to replace with this.
Replace with what? The intention of the original message was just not to
rely on a very concrete "NicCapability", but provide an abstract
Capability instead.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-06-17 15:07 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-01 15:54 [RFC PATCH v1] dts: skip test cases based on capabilities Juraj Linkeš
2024-04-11 8:48 ` [RFC PATCH v2] " Juraj Linkeš
2024-05-21 15:47 ` Luca Vizzarro
2024-05-22 14:58 ` Luca Vizzarro
2024-06-07 13:13 ` Juraj Linkeš
2024-06-11 9:51 ` Luca Vizzarro
2024-06-12 9:15 ` Juraj Linkeš
2024-06-17 15:07 ` Luca Vizzarro
2024-05-24 20:51 ` Nicholas Pratte
2024-05-31 16:44 ` Luca Vizzarro
2024-06-05 13:55 ` Patrick Robb
2024-06-06 13:36 ` Jeremy Spewock
2024-06-03 14:40 ` Nicholas Pratte
2024-06-07 13:20 ` Juraj Linkeš
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).