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 9BE0D440B6; Fri, 24 May 2024 22:52:06 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 20CB9402ED; Fri, 24 May 2024 22:52:06 +0200 (CEST) Received: from mail-lf1-f50.google.com (mail-lf1-f50.google.com [209.85.167.50]) by mails.dpdk.org (Postfix) with ESMTP id D83174026B for ; Fri, 24 May 2024 22:52:04 +0200 (CEST) Received: by mail-lf1-f50.google.com with SMTP id 2adb3069b0e04-5295f9b4f86so112415e87.2 for ; Fri, 24 May 2024 13:52:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iol.unh.edu; s=unh-iol; t=1716583924; x=1717188724; 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=sUq9W2X8O44uuSBlvD82m6nSviR/9OpArLv0UijXpLA=; b=PNxRpGubSOOlAtocyuezhkBiFS6IEXhQpPVKYZmeY4sKspeFBahDDtsjkKijjDbzRO C4T3DsWHXdaa9kk/scdviSk/rvqJnykhK2BDG68EkQapeDpI5mAsArv1bPCzaE/j+OaP f7m2FGXhLSs1QhTvxcuwPYfML/aJhAN5VJ6yY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716583924; x=1717188724; 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=sUq9W2X8O44uuSBlvD82m6nSviR/9OpArLv0UijXpLA=; b=IuPGnEhXXHvIO8ki5L4VWO+qcfF+/f1mIG+2qT5xshpe+rySoPs8CBENtqS4rHfclI 0uPZtaEGzqou3tNJK3X+JsjREencqS1DLWW0sNBXUO8fw6bx5hH1Rn0VwsJrVBcTgjR5 YepYTx3ooNuTy3YEUmg9X8b5IxiKYyc8F0Gzx+3W+MpZRr5Brn5xFP/ADZhJwanQhMUb A399DMaZOggdm1NI2mnZchzZXDYjFSe8RCFTUtNgykDDyL90lY2MxNFNBrZbLFbv0Td3 P/pq6SNzTEDD6CRJMwscwE4iaE8sAkCY/ZBRHc0YXen660wFvh1QacSIaPAIFHVoK/uE 94yw== X-Forwarded-Encrypted: i=1; AJvYcCXCPB5XmqLLnpSZicAUls5FXfhorrbA/eGu+FbK5dxPmod1Y0E6/EBf3jDNt0tKWOK079DpxFMURQSZ2ik= X-Gm-Message-State: AOJu0Yy3mpHkPi+f32RiUsyK9BOQwOHaAE781BCHn73xCptQ8CnCUnYF rrmSUGgceBgi5NlzNsjEbscAIKX4phJE79sORHkN4v4mr8sneHkWw1x70fTRiqR5cLQYx1VsgnL 1hB8kxW0Zq+P4rbg2eG9S6+8VbeGGmGxWlOOPC6ltElW376bv X-Google-Smtp-Source: AGHT+IE5/xNbmvgfU3e1rLYQW3dULrvd99M6dCQ8zFKiPK6/Z0ZSxq26oYmCUtSVifsv4/HXLv3y4mGqLi8m17SnSmE= X-Received: by 2002:a2e:2e06:0:b0:2e2:18c2:9c8b with SMTP id 38308e7fff4ca-2e95af3f76cmr17241011fa.0.1716583924022; Fri, 24 May 2024 13:52:04 -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: Fri, 24 May 2024 16:51:53 -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 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=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. 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 =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. "Which capability supported can be obtained." I think there was tense error here. > + 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 "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_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. 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 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`. 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] =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 > The above comments are basically just nitpicks, but if nothing else, I figured I'd bring it to your attention.