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 DCE5643380; Mon, 20 Nov 2023 17:25:30 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CCA5342E23; Mon, 20 Nov 2023 17:25:30 +0100 (CET) Received: from mail-ej1-f45.google.com (mail-ej1-f45.google.com [209.85.218.45]) by mails.dpdk.org (Postfix) with ESMTP id A614D42E1E for ; Mon, 20 Nov 2023 17:25:29 +0100 (CET) Received: by mail-ej1-f45.google.com with SMTP id a640c23a62f3a-9fffa4c4f43so100735966b.3 for ; Mon, 20 Nov 2023 08:25:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1700497529; x=1701102329; 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=P4dK89nEQXpmNtFoy2EXQkxpEJkUNxmRUSBAFQamn88=; b=mHW8FNz1Pny34UaNNx1uOrdQgqC9kBE6kD34mtyRhBRwonctBrZNb1OFpeWjTl/rA6 PK6GVb50w0guvAqBNvX5pia/OJhAC2s+gz3DeGtbPLVaWlfrU9FIAgj3fMgodPwTT4qB qBLoThqPNnJtp+mXILPPRDGeQQf3CNdbBg8IGQ480d1e1vxvDUM5kUXqNKhw8vuotJwQ GdBye+aEH7yGozWgm4FzqNrqNdU/3r5dBnD4+2sw4HQnz5vCPc6FaP4MAqam4q/cnzFm VV08ssoRw7uSH+bUyYQ6gwZgEzBCPBH7M4jACwfzoSW0/V0ifg7938+PlnoChopaVMNn pI9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700497529; x=1701102329; 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=P4dK89nEQXpmNtFoy2EXQkxpEJkUNxmRUSBAFQamn88=; b=BV7uTonJw4YLoUTIDyLZMyiKOsPvim3rNanKOeTRcIAPHVP+9svszSdwmMfC5WtWeA ukYJeh6tNa2qdHRHvQdKWClGkghArZtSRpT8qJ3Aw79+BMOYep+meQa7ZFs5Rq2sJ3Qd qOhx+zKrXOqVCOfngF2MrqzVg0EnftYfooWLd9WmwHGwy6kUm2CJhM0TFV6JdD3AgqlY EBsvm4vDXhFuy0SRaKkwRV3CDWaUS0StWKuQWkVkXG9lyIiB9eCNKURHDwXqP06Hjjd+ smyTrzVynGONSgtjH7imT4KrCu3FTIjjEdXkorgsmqEhehrj8bztqBmVRniH8GWiEO4b y4OA== X-Gm-Message-State: AOJu0YxbCRXOouw3hCgsd0vR8wb5cN+Z9+K1vBnONyXk9T6mjz/KDWl/ zUYvMKLJ4vRJVvB6os5DyGWwJ7fmZX9JvWSUClj5Xg== X-Google-Smtp-Source: AGHT+IEybAdGsBSD+uHrr3zQDygEsa2hgnwuEr8J/zWIEvAfkZlqzO+MlJ3gEheAxZdlU4FUPqnTTjXShU0RdA7lSH4= X-Received: by 2002:a17:907:a683:b0:9e7:3af8:1fcd with SMTP id vv3-20020a170907a68300b009e73af81fcdmr7868972ejc.76.1700497529179; Mon, 20 Nov 2023 08:25:29 -0800 (PST) MIME-Version: 1.0 References: <20231108125324.191005-23-juraj.linkes@pantheon.tech> <20231115130959.39420-1-juraj.linkes@pantheon.tech> <20231115130959.39420-9-juraj.linkes@pantheon.tech> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Mon, 20 Nov 2023 17:25:18 +0100 Message-ID: Subject: Re: [PATCH v7 08/21] dts: test suite docstring update To: Jeremy Spewock Cc: thomas@monjalon.net, Honnappa.Nagarahalli@arm.com, probb@iol.unh.edu, paul.szczepanek@arm.com, yoan.picchi@foss.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 On Thu, Nov 16, 2023 at 11:16=E2=80=AFPM Jeremy Spewock wrote: > > > > On Wed, Nov 15, 2023 at 8:12=E2=80=AFAM Juraj Linke=C5=A1 wrote: >> >> Format according to the Google format and PEP257, with slight >> deviations. >> >> Signed-off-by: Juraj Linke=C5=A1 >> --- >> dts/framework/test_suite.py | 223 +++++++++++++++++++++++++++--------- >> 1 file changed, 168 insertions(+), 55 deletions(-) >> >> diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py >> index d53553bf34..9e5251ffc6 100644 >> --- a/dts/framework/test_suite.py >> +++ b/dts/framework/test_suite.py >> @@ -2,8 +2,19 @@ >> # Copyright(c) 2010-2014 Intel Corporation >> # Copyright(c) 2023 PANTHEON.tech s.r.o. >> >> -""" >> -Base class for creating DTS test cases. >> +"""Features common to all test suites. >> + >> +The module defines the :class:`TestSuite` class which doesn't contain a= ny test cases, and as such >> +must be extended by subclasses which add test cases. The :class:`TestSu= ite` contains the basics >> +needed by subclasses: >> + >> + * Test suite and test case execution flow, >> + * Testbed (SUT, TG) configuration, >> + * Packet sending and verification, >> + * Test case verification. >> + >> +The module also defines a function, :func:`get_test_suites`, >> +for gathering test suites from a Python module. >> """ >> >> import importlib >> @@ -31,25 +42,44 @@ >> >> >> class TestSuite(object): >> - """ >> - The base TestSuite class provides methods for handling basic flow o= f a test suite: >> - * test case filtering and collection >> - * test suite setup/cleanup >> - * test setup/cleanup >> - * test case execution >> - * error handling and results storage >> - Test cases are implemented by derived classes. Test cases are all m= ethods >> - starting with test_, further divided into performance test cases >> - (starting with test_perf_) and functional test cases (all other tes= t cases). >> - By default, all test cases will be executed. A list of testcase str= names >> - may be specified in conf.yaml or on the command line >> - to filter which test cases to run. >> - The methods named [set_up|tear_down]_[suite|test_case] should be ov= erridden >> - in derived classes if the appropriate suite/test case fixtures are = needed. >> + """The base class with methods for handling the basic flow of a tes= t suite. >> + >> + * Test case filtering and collection, >> + * Test suite setup/cleanup, >> + * Test setup/cleanup, >> + * Test case execution, >> + * Error handling and results storage. >> + >> + Test cases are implemented by subclasses. Test cases are all method= s starting with ``test_``, >> + further divided into performance test cases (starting with ``test_p= erf_``) >> + and functional test cases (all other test cases). >> + >> + By default, all test cases will be executed. A list of testcase nam= es may be specified >> + in the YAML test run configuration file and in the :option:`--test-= cases` command line argument >> + or in the :envvar:`DTS_TESTCASES` environment variable to filter wh= ich test cases to run. >> + The union of both lists will be used. Any unknown test cases from t= he latter lists >> + will be silently ignored. >> + >> + If the :option:`--re-run` command line argument or the :envvar:`DTS= _RERUN` environment variable >> + is set, in case of a test case failure, the test case will be execu= ted again until it passes >> + or it fails that many times in addition of the first failure. >> + >> + The methods named ``[set_up|tear_down]_[suite|test_case]`` should b= e overridden in subclasses >> + if the appropriate test suite/test case fixtures are needed. >> + >> + The test suite is aware of the testbed (the SUT and TG) it's runnin= g on. From this, it can >> + properly choose the IP addresses and other configuration that must = be tailored to the testbed. >> + >> + Attributes: >> + sut_node: The SUT node where the test suite is running. >> + tg_node: The TG node where the test suite is running. >> + is_blocking: Whether the test suite is blocking. A failure of a= blocking test suite >> + will block the execution of all subsequent test suites in t= he current build target. >> """ > > > Should this attribute section instead be comments in the form "#:" becaus= e they are class variables instead of instance ones? > Yes and no. The first two are not class variables, but the last one is, so I'll change is_blocking to ClassVar. Thankfully the resulting generated docs look just fine, the instance variables are listed first, then class variables. >> >> >> sut_node: SutNode >> - is_blocking =3D False >> + tg_node: TGNode >> + is_blocking: bool =3D False >> _logger: DTSLOG >> _test_cases_to_run: list[str] >> _func: bool >> @@ -72,6 +102,19 @@ def __init__( >> func: bool, >> build_target_result: BuildTargetResult, >> ): >> + """Initialize the test suite testbed information and basic conf= iguration. >> + >> + Process what test cases to run, create the associated :class:`T= estSuiteResult`, >> + find links between ports and set up default IP addresses to be = used when configuring them. >> + >> + Args: >> + sut_node: The SUT node where the test suite will run. >> + tg_node: The TG node where the test suite will run. >> + test_cases: The list of test cases to execute. >> + If empty, all test cases will be executed. >> + func: Whether to run functional tests. >> + build_target_result: The build target result this test suit= e is run in. >> + """ >> self.sut_node =3D sut_node >> self.tg_node =3D tg_node >> self._logger =3D getLogger(self.__class__.__name__) >> @@ -95,6 +138,7 @@ def __init__( >> self._tg_ip_address_ingress =3D ip_interface("192.168.101.3/24"= ) >> >> def _process_links(self) -> None: >> + """Construct links between SUT and TG ports.""" >> for sut_port in self.sut_node.ports: >> for tg_port in self.tg_node.ports: >> if (sut_port.identifier, sut_port.peer) =3D=3D ( >> @@ -106,27 +150,42 @@ def _process_links(self) -> None: >> ) >> >> def set_up_suite(self) -> None: >> - """ >> - Set up test fixtures common to all test cases; this is done bef= ore >> - any test case is run. >> + """Set up test fixtures common to all test cases. >> + >> + This is done before any test case has been run. >> """ >> >> def tear_down_suite(self) -> None: >> - """ >> - Tear down the previously created test fixtures common to all te= st cases. >> + """Tear down the previously created test fixtures common to all= test cases. >> + >> + This is done after all test have been run. >> """ >> >> def set_up_test_case(self) -> None: >> - """ >> - Set up test fixtures before each test case. >> + """Set up test fixtures before each test case. >> + >> + This is done before *each* test case. >> """ >> >> def tear_down_test_case(self) -> None: >> - """ >> - Tear down the previously created test fixtures after each test = case. >> + """Tear down the previously created test fixtures after each te= st case. >> + >> + This is done after *each* test case. >> """ >> >> def configure_testbed_ipv4(self, restore: bool =3D False) -> None: >> + """Configure IPv4 addresses on all testbed ports. >> + >> + The configured ports are: >> + >> + * SUT ingress port, >> + * SUT egress port, >> + * TG ingress port, >> + * TG egress port. >> + >> + Args: >> + restore: If :data:`True`, will remove the configuration ins= tead. >> + """ >> delete =3D True if restore else False >> enable =3D False if restore else True >> self._configure_ipv4_forwarding(enable) >> @@ -153,11 +212,13 @@ def _configure_ipv4_forwarding(self, enable: bool)= -> None: >> def send_packet_and_capture( >> self, packet: Packet, duration: float =3D 1 >> ) -> list[Packet]: >> - """ >> - Send a packet through the appropriate interface and >> - receive on the appropriate interface. >> - Modify the packet with l3/l2 addresses corresponding >> - to the testbed and desired traffic. >> + """Send and receive `packet` using the associated TG. >> + >> + Send `packet` through the appropriate interface and receive on = the appropriate interface. >> + Modify the packet with l3/l2 addresses corresponding to the tes= tbed and desired traffic. >> + >> + Returns: >> + A list of received packets. >> """ >> packet =3D self._adjust_addresses(packet) >> return self.tg_node.send_packet_and_capture( >> @@ -165,13 +226,25 @@ def send_packet_and_capture( >> ) >> >> def get_expected_packet(self, packet: Packet) -> Packet: >> + """Inject the proper L2/L3 addresses into `packet`. >> + >> + Args: >> + packet: The packet to modify. >> + >> + Returns: >> + `packet` with injected L2/L3 addresses. >> + """ >> return self._adjust_addresses(packet, expected=3DTrue) >> >> def _adjust_addresses(self, packet: Packet, expected: bool =3D Fals= e) -> Packet: >> - """ >> + """L2 and L3 address additions in both directions. >> + >> Assumptions: >> - Two links between SUT and TG, one link is TG -> SUT, >> - the other SUT -> TG. >> + Two links between SUT and TG, one link is TG -> SUT, the ot= her SUT -> TG. >> + >> + Args: >> + packet: The packet to modify. >> + expected: If :data:`True`, the direction is SUT -> TG, othe= rwise the direction is TG -> SUT. >> """ >> if expected: >> # The packet enters the TG from SUT >> @@ -197,6 +270,19 @@ def _adjust_addresses(self, packet: Packet, expecte= d: bool =3D False) -> Packet: >> return Ether(packet.build()) >> >> def verify(self, condition: bool, failure_description: str) -> None= : >> + """Verify `condition` and handle failures. >> + >> + When `condition` is :data:`False`, raise an exception and log t= he last 10 commands >> + executed on both the SUT and TG. >> + >> + Args: >> + condition: The condition to check. >> + failure_description: A short description of the failure >> + that will be stored in the raised exception. >> + >> + Raises: >> + TestCaseVerifyError: `condition` is :data:`False`. >> + """ >> if not condition: >> self._fail_test_case_verify(failure_description) >> >> @@ -216,6 +302,19 @@ def _fail_test_case_verify(self, failure_descriptio= n: str) -> None: >> def verify_packets( >> self, expected_packet: Packet, received_packets: list[Packet] >> ) -> None: >> + """Verify that `expected_packet` has been received. >> + >> + Go through `received_packets` and check that `expected_packet` = is among them. >> + If not, raise an exception and log the last 10 commands >> + executed on both the SUT and TG. >> + >> + Args: >> + expected_packet: The packet we're expecting to receive. >> + received_packets: The packets where we're looking for `expe= cted_packet`. >> + >> + Raises: >> + TestCaseVerifyError: `expected_packet` is not among `receiv= ed_packets`. >> + """ >> for received_packet in received_packets: >> if self._compare_packets(expected_packet, received_packet): >> break >> @@ -303,10 +402,14 @@ def _verify_l3_packet(self, received_packet: IP, e= xpected_packet: IP) -> bool: >> return True >> >> def run(self) -> None: >> - """ >> - Setup, execute and teardown the whole suite. >> - Suite execution consists of running all test cases scheduled to= be executed. >> - A test cast run consists of setup, execution and teardown of sa= id test case. >> + """Set up, execute and tear down the whole suite. >> + >> + Test suite execution consists of running all test cases schedul= ed to be executed. >> + A test case run consists of setup, execution and teardown of sa= id test case. >> + >> + Record the setup and the teardown and handle failures. >> + >> + The list of scheduled test cases is constructed when creating t= he :class:`TestSuite` object. >> """ >> test_suite_name =3D self.__class__.__name__ >> >> @@ -338,9 +441,7 @@ def run(self) -> None: >> raise BlockingTestSuiteError(test_suite_name) >> >> def _execute_test_suite(self) -> None: >> - """ >> - Execute all test cases scheduled to be executed in this suite. >> - """ >> + """Execute all test cases scheduled to be executed in this suit= e.""" >> if self._func: >> for test_case_method in self._get_functional_test_cases(): >> test_case_name =3D test_case_method.__name__ >> @@ -357,14 +458,18 @@ def _execute_test_suite(self) -> None: >> self._run_test_case(test_case_method, test_case_res= ult) >> >> def _get_functional_test_cases(self) -> list[MethodType]: >> - """ >> - Get all functional test cases. >> + """Get all functional test cases defined in this TestSuite. >> + >> + Returns: >> + The list of functional test cases of this TestSuite. >> """ >> return self._get_test_cases(r"test_(?!perf_)") >> >> def _get_test_cases(self, test_case_regex: str) -> list[MethodType]= : >> - """ >> - Return a list of test cases matching test_case_regex. >> + """Return a list of test cases matching test_case_regex. >> + >> + Returns: >> + The list of test cases matching test_case_regex of this Tes= tSuite. >> """ >> self._logger.debug(f"Searching for test cases in {self.__class_= _.__name__}.") >> filtered_test_cases =3D [] >> @@ -378,9 +483,7 @@ def _get_test_cases(self, test_case_regex: str) -> l= ist[MethodType]: >> return filtered_test_cases >> >> def _should_be_executed(self, test_case_name: str, test_case_regex:= str) -> bool: >> - """ >> - Check whether the test case should be executed. >> - """ >> + """Check whether the test case should be scheduled to be execut= ed.""" >> match =3D bool(re.match(test_case_regex, test_case_name)) >> if self._test_cases_to_run: >> return match and test_case_name in self._test_cases_to_run >> @@ -390,9 +493,9 @@ def _should_be_executed(self, test_case_name: str, t= est_case_regex: str) -> bool >> def _run_test_case( >> self, test_case_method: MethodType, test_case_result: TestCaseR= esult >> ) -> None: >> - """ >> - Setup, execute and teardown a test case in this suite. >> - Exceptions are caught and recorded in logs and results. >> + """Setup, execute and teardown a test case in this suite. >> + >> + Record the result of the setup and the teardown and handle fail= ures. >> """ >> test_case_name =3D test_case_method.__name__ >> >> @@ -427,9 +530,7 @@ def _run_test_case( >> def _execute_test_case( >> self, test_case_method: MethodType, test_case_result: TestCaseR= esult >> ) -> None: >> - """ >> - Execute one test case and handle failures. >> - """ >> + """Execute one test case, record the result and handle failures= .""" >> test_case_name =3D test_case_method.__name__ >> try: >> self._logger.info(f"Starting test case execution: {test_cas= e_name}") >> @@ -452,6 +553,18 @@ def _execute_test_case( >> >> >> def get_test_suites(testsuite_module_path: str) -> list[type[TestSuite]= ]: >> + r"""Find all :class:`TestSuite`\s in a Python module. >> + >> + Args: >> + testsuite_module_path: The path to the Python module. >> + >> + Returns: >> + The list of :class:`TestSuite`\s found within the Python module= . >> + >> + Raises: >> + ConfigurationError: The test suite module was not found. >> + """ >> + >> def is_test_suite(object: Any) -> bool: >> try: >> if issubclass(object, TestSuite) and object is not TestSuit= e: >> -- >> 2.34.1 >>