From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7A46844147; Mon, 3 Jun 2024 16:41:03 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 13E4F42E1C; Mon, 3 Jun 2024 16:41:03 +0200 (CEST) Received: from mail-lj1-f179.google.com (mail-lj1-f179.google.com [209.85.208.179]) by mails.dpdk.org (Postfix) with ESMTP id 05B3942E02 for ; Mon, 3 Jun 2024 16:41:00 +0200 (CEST) Received: by mail-lj1-f179.google.com with SMTP id 38308e7fff4ca-2eab062bbdeso1836421fa.1 for ; Mon, 03 Jun 2024 07:41:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1717425660; x=1718030460; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=R1rWEKQPN4mP3lafBgQ4+zQrdvAzqSwwJjKcs4IA/5o=; b=Bas7eu5faRewpmSh7Zbd1hV3p+9DXGnzqHp+wv2ijKJ0dXSgL8xnoBmhSHcpBXR2W/ e97I8tE+kYbwYeoMxfReDUyqeNSZjZvCrYYazpH/KGeUR7O4aX06rIDcXXfQ6L2TntZO ICVtlSi453aYpx/RuhcjhvO1F08K7q7+6B5iA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717425660; x=1718030460; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=R1rWEKQPN4mP3lafBgQ4+zQrdvAzqSwwJjKcs4IA/5o=; b=DepYHt9hy1O1AAQ/qy7c4cH2kwNVBUK50EyEfxdBSoQhFSVGlXrYvUUi9kPBpYgIlV g6I2P8Lo9ZOocrcc4hKgaUYScG6wtRSAXBY+8joIVlaITOeawSdX3APXMXBoNcBZciHa sRKJPrVBFxvmQYDhRl2+28TmQdGGYLqur5ypRCWArlGBetVDaiSBk6cjNlAODbKwapWJ xgesQZWqEEu7sM3u4vE/8KMA3Q9bUM+YPrpqRjjqAfuQwudMvITDf7L55xd8v2gl2ARZ vWifCS9Q3Jb45cY+CNY4f/J4UAoxItzmBgc69kcNuQwYIDL5BC5Zwm9qVOXJagm2EMCP E9Fw== X-Forwarded-Encrypted: i=1; AJvYcCUhxq44LAKJLYbAjdAKpzU9xh7buZ+j8ZeQJzyMA2/Roev0Q6FhDNFzTH6+k5THbE/uKdhlQX75YREBre8= X-Gm-Message-State: AOJu0YwgbI7Gl0NCPOBtTYhT9A+kIbBU0I0oIcWyQiIJPAHHqOKG5NOt FaN6NK5JBvcDnfNpkRDrgV0xzCsdVb9yjMc8myNur47KJHqI757D7thm8ewJc/8OpNkQWnFwbC1 zCSzIA3R7PYe/PabAIowjx3yikBpatuxybVnkug== X-Google-Smtp-Source: AGHT+IH4XT3aO1ubZEMKQp7UKRaA2VRyrVyRUnJkwsZmIH/X4xsprH15ues4WlqziIydAkqom+LBDcTByAYJvjfrJMM= X-Received: by 2002:a2e:984b:0:b0:2e9:8aa1:af89 with SMTP id 38308e7fff4ca-2ea95207270mr50572941fa.3.1717425660038; Mon, 03 Jun 2024 07:41:00 -0700 (PDT) MIME-Version: 1.0 References: <20240301155416.96960-1-juraj.linkes@pantheon.tech> <20240411084829.64984-1-juraj.linkes@pantheon.tech> In-Reply-To: <20240411084829.64984-1-juraj.linkes@pantheon.tech> From: Nicholas Pratte Date: Mon, 3 Jun 2024 10:40:47 -0400 Message-ID: Subject: Re: [RFC PATCH v2] dts: skip test cases based on capabilities To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, jspewock@iol.unh.edu, probb@iol.unh.edu, paul.szczepanek@arm.com, Luca.Vizzarro@arm.com, dev@dpdk.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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=E2=80=AFAM Juraj Linke=C5=A1 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=C5=A1 > --- > 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/rem= ote_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/framewor= k/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_capabiliti= es: MutableSet > + ) -> None: > + """Get all rxq capabilities and divide them into supported and u= nsupported. > + > + Args: > + supported_capabilities: A set where capabilities which are s= upported will be stored. > + unsupported_capabilities: A set where capabilities which are > + not supported will be stored. > + """ > + self._logger.debug("Getting rxq capabilities.") > + command =3D "show rxq info 0 0" > + rxq_info =3D self.send_command(command) > + for line in rxq_info.split("\n"): > + bare_line =3D line.strip() > + if bare_line.startswith("RX scattered packets:"): > + if bare_line.endswith("on"): > + supported_capabilities.add(NicCapability.scattered_r= x) > + else: > + unsupported_capabilities.add(NicCapability.scattered= _rx) > + > + > +class NicCapability(Enum): > + """A mapping between capability names and the associated :class:`Tes= tPmdShell` 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_capabil= ities: MutableSet) -> None > + > + The function must execute the testpmd command from which the capabil= ity support can be obtained. > + If multiple capabilities can be obtained from the same testpmd comma= nd, each should be obtained > + in one function. These capabilities should then be added to `support= ed_capabilities` or > + `unsupported_capabilities` based on their support. > + """ > + > + scattered_rx =3D 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 be= en built on the SUT node. > The current build target thus corresponds to the current DPDK bu= ild present on the SUT node. > > + Before running any suites, the method determines whether they sh= ould 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 ski= pped, 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, t= he 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 r= un. > """ > end_build_target =3D False > + required_capabilities =3D set() > + supported_capabilities =3D set() > + for test_suite_with_cases in test_suites_with_cases: > + required_capabilities.update(test_suite_with_cases.req_capab= ilities) > + self._logger.debug(f"Found required capabilities: {required_capa= bilities}") > + if required_capabilities: > + supported_capabilities =3D sut_node.get_supported_capabiliti= es(required_capabilities) > for test_suite_with_cases in test_suites_with_cases: > test_suite_result =3D build_target_result.add_test_suite(tes= t_suite_with_cases) > + test_suite_with_cases.mark_skip(supported_capabilities) > try: > - self._run_test_suite(sut_node, tg_node, test_suite_resul= t, 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.te= st_suite_class.__name__}. " > @@ -614,14 +640,18 @@ def _execute_test_suite( > test_case_result =3D test_suite_result.add_test_case(test_ca= se_name) > all_attempts =3D SETTINGS.re_run + 1 > attempt_nr =3D 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 +=3D 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_c= ase_result) > + while not test_case_result and attempt_nr < all_attempts= : > + attempt_nr +=3D 1 > + self._logger.info( > + f"Re-running FAILED test case '{test_case_name}'= . " > + f"Attempt number {attempt_nr} out of {all_attemp= ts}." > + ) > + self._run_test_case(test_suite, test_case_method, te= st_case_result) > + else: > + self._logger.info(f"Test case execution SKIPPED: {test_c= ase_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 b= e all test cases) because > > test_suite_class: type[TestSuite] > test_cases: list[MethodType] > + req_capabilities: set[NicCapability] =3D field(default_factory=3Dset= , init=3DFalse) > + > + def __post_init__(self): > + """Gather the required capabilities of the test suite and all te= st cases.""" > + for test_object in [self.test_suite_class] + self.test_cases: > + test_object.skip =3D 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 suit= e with test cases. > @@ -76,6 +86,47 @@ def create_config(self) -> TestSuiteConfig: > test_cases=3D[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 capabil= ities 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_cap= abilities: > + test_object.skip =3D True > + > + @staticmethod > + def should_not_be_skipped(test_object: Union[type[TestSuite] | Metho= dType]) -> 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 skippe= d, :data:`False` otherwise. > + """ > + return not getattr(test_object, "skip", False) > + > + def __bool__(self) -> bool: > + """The truth value is determined by whether the test suite shoul= d be run. > + > + Returns: > + :data:`False` if the test suite should be skipped, :data:`Tr= ue` otherwise. > + """ > + found_test_case_to_run =3D False > + for test_case in self.test_cases: > + if self.should_not_be_skipped(test_case): > + found_test_case_to_run =3D True > + break > + return found_test_case_to_run and self.should_not_be_skipped(sel= f.test_suite_class) > + > > class Result(Enum): > """The possible states that a setup, a teardown or a test case may e= nd up in.""" > @@ -170,12 +221,13 @@ def update_setup(self, result: Result, error: Excep= tion | None =3D None) -> None: > self.setup_result.result =3D result > self.setup_result.error =3D error > > - if result in [Result.BLOCK, Result.ERROR, Result.FAIL]: > - self.update_teardown(Result.BLOCK) > - self._block_result() > + if result !=3D Result.PASS: > + result_to_mark =3D Result.BLOCK if result !=3D Result.SKIP e= lse 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 metho= ds. > """ > @@ -390,11 +442,11 @@ def add_sut_info(self, sut_info: NodeInfo) -> None: > self.sut_os_version =3D sut_info.os_version > self.sut_kernel_version =3D 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 =3D 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: BuildTarg= etInfo) -> None: > self.compiler_version =3D versions.compiler_version > self.dpdk_version =3D 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 =3D 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) -> "Te= stCaseResult": > 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 =3D 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 +=3D 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 cas= e 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 cur= rent build target. > is_blocking: ClassVar[bool] =3D False > + skip: bool > _logger: DTSLogger > _port_links: list[PortLink] > _sut_port_ingress: Port > @@ -89,6 +93,7 @@ def __init__( > """ > self.sut_node =3D sut_node > self.tg_node =3D tg_node > + self.skip =3D False > self._logger =3D get_dts_logger(self.__class__.__name__) > self._port_links =3D [] > self._process_links() > @@ -360,3 +365,23 @@ def _verify_l3_packet(self, received_packet: IP, exp= ected_packet: IP) -> bool: > if received_packet.src !=3D expected_packet.src or received_pack= et.dst !=3D expected_packet.dst: > return False > return True > + > + > +def requires(capability: NicCapability) -> Callable: > + """A decorator that marks the decorated test case or test suite as o= ne to be skipped. > + > + Args: > + The capability that's required by the decorated test case or tes= t 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 =3D [capability] > + return func > + > + return add_req_capa > diff --git a/dts/framework/testbed_model/sut_node.py b/dts/framework/test= bed_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, TestP= mdShell > 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 `capab= ilities`. > + > + Args: > + capabilities: The capabilities to verify. > + > + Returns: > + The set of supported capabilities of the current NIC. > + """ > + supported_capas: set[NicCapability] =3D set() > + unsupported_capas: set[NicCapability] =3D set() > + self._logger.debug(f"Checking which capabilities from {capabilit= ies} NIC are supported.") > + testpmd_shell =3D self.create_interactive_shell(TestPmdShell, pr= ivileged=3DTrue) > + for capability in capabilities: > + if capability not in supported_capas or capability not in un= supported_capas: > + capability.value(testpmd_shell, supported_capas, unsuppo= rted_capas) > + del testpmd_shell > + return supported_capas > + > def _set_up_build_target(self, build_target_config: BuildTargetConfi= guration) -> None: > """Setup DPDK on the SUT node. > > diff --git a/dts/tests/TestSuite_hello_world.py b/dts/tests/TestSuite_hel= lo_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 =3D self.sut_node.build_dpdk_app("hello= world") > > + @requires(NicCapability.scattered_rx) > def test_hello_world_single_core(self) -> None: > """Single core test case. > > -- > 2.34.1 >