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 1DF06459AE; Mon, 16 Sep 2024 15:01:00 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E94344025F; Mon, 16 Sep 2024 15:00:59 +0200 (CEST) Received: from mail-lj1-f171.google.com (mail-lj1-f171.google.com [209.85.208.171]) by mails.dpdk.org (Postfix) with ESMTP id C68A340150 for ; Mon, 16 Sep 2024 15:00:57 +0200 (CEST) Received: by mail-lj1-f171.google.com with SMTP id 38308e7fff4ca-2f78b28ddb6so36497731fa.0 for ; Mon, 16 Sep 2024 06:00:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1726491657; x=1727096457; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=9Or06FujlLi+MwohpPZ14ijt7ciwMIKzBa1TakIlNB8=; b=fSQSzy7X4TEhRZoTftJtaCAwj9ph5buyt8rybh3ZoHZju8UOekq7bc/C97lnTICAWF lH3HmNwzC4YPxvJl/Wy/lSDF4wV0A7K/HTvTJgl9NtQ+YKr7etZG7k9SGWyywhKvG4sp G/iPuSNQk8BJpmrvfcj0uaEr/i9m3taHsz88PplnOY6SrmX+Zpg02NBnOa50gFgHJ8jW ic8IGir++CV2qeU8xLTruSn6assxL2ALNMC3kcoHfQ81xnRzPRUwQclop+scVO0IqAVA ZjEgDy6LfiJzcIIOoUbOuldaPOXIFWjzg0ZNN/i6a/DcQTzdEjQQ7sYyY2OrEiJogrVy cKdg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1726491657; x=1727096457; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=9Or06FujlLi+MwohpPZ14ijt7ciwMIKzBa1TakIlNB8=; b=P1dlFUrNZJcDJMHKgUxDyBwk+KQmMEIqCpqrk57RgRDax/z9eWrvtaltSk0hjkB6tS cHVD7ggG7ZiPN6LtvB8kfJHx802D/9MXhA03i7gd+WJSTcWBzCosJ4KMSeXwkiGD5R+Y I98IH0XZaZTS6FL/E46YdBZ/iHHrdHNpxzOX+2VzBjbTPbRVlwbsolBJDU0OMIUSp5nu uYUm9TqXlJaoU6/a/LWUYQ9c8GE+db5Yo9YMt6XItaWcrIpk8xAID+K0jgSguqBgXek8 Cr8Q0wmoS3Nht0Hv+e5jTC1wdc1bmUPrl1QYNf9MBxc+HOqJR2ozzlXBtpTuN2lECSXP jNCQ== X-Forwarded-Encrypted: i=1; AJvYcCXvQCK/WEPHxo3+EIXJCbnHpkCszCtTmrHxuPTqXykc0OoHPhjo4h2lQQURP5hj3hea9mo=@dpdk.org X-Gm-Message-State: AOJu0YzrfcefX3b9GbEP/D3bL8TMBEaBymTnj5mLOrHwTH5SVtUeJL71 algp+ye+ox/2gz5SUocgzpmZSYhrNEl/RdXFHVCTjxN0F/YeTqlwDmbXJQDe/a4= X-Google-Smtp-Source: AGHT+IHQlxLrlvIdyLfHqZmIBqXowoE3XVjy8dxze0JDeiJiwhtwJoadRBXtZ8I7k0vGlNcMpI6DNg== X-Received: by 2002:a2e:be0b:0:b0:2ef:24a0:c176 with SMTP id 38308e7fff4ca-2f787f31805mr73344261fa.28.1726491656697; Mon, 16 Sep 2024 06:00:56 -0700 (PDT) Received: from [192.168.200.22] ([84.245.121.62]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5c42bb492a1sm2569664a12.8.2024.09.16.06.00.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 16 Sep 2024 06:00:56 -0700 (PDT) Message-ID: <0bfa524b-ac38-4fa5-937f-51efcfdf6459@pantheon.tech> Date: Mon, 16 Sep 2024 15:00:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/5] dts: add TestSuiteSpec class and discovery To: Luca Vizzarro , dev@dpdk.org Cc: Honnappa Nagarahalli , Paul Szczepanek References: <20240822163941.1390326-1-luca.vizzarro@arm.com> <20240822163941.1390326-2-luca.vizzarro@arm.com> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <20240822163941.1390326-2-luca.vizzarro@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 There are some elements which seem to be present in https://patches.dpdk.org/project/dpdk/patch/20240821145315.97974-4-juraj.linkes@pantheon.tech/, which is an attempt at decorating test cases (buzgilla 1460) as part of the capabilities series. Looks like we could create a separate patch with 1460 and this patch in it on which both capabilities and this series would depend. What do you think? Certainly makes sense to have decorating tests cases separate from capabilities. On 22. 8. 2024 18:39, Luca Vizzarro wrote: > Currently there is a lack of a definition which identifies all the test > suites available to test. This change intends to simplify the process to > discover all the test suites and idenfity them. > > Signed-off-by: Luca Vizzarro > Reviewed-by: Paul Szczepanek > --- > dts/framework/test_suite.py | 182 +++++++++++++++++++++++++++++++++++- > 1 file changed, 181 insertions(+), 1 deletion(-) > > diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py > index 694b2eba65..972968b036 100644 > --- a/dts/framework/test_suite.py > +++ b/dts/framework/test_suite.py > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: BSD-3-Clause > # Copyright(c) 2010-2014 Intel Corporation > # Copyright(c) 2023 PANTHEON.tech s.r.o. > +# Copyright(c) 2024 Arm Limited > > """Features common to all test suites. > > @@ -13,12 +14,22 @@ > * Test case verification. > """ > > +import inspect > +import re > +from dataclasses import dataclass > +from enum import Enum, auto > +from functools import cached_property > +from importlib import import_module > from ipaddress import IPv4Interface, IPv6Interface, ip_interface > -from typing import ClassVar, Union > +from pkgutil import iter_modules > +from types import FunctionType, ModuleType > +from typing import ClassVar, NamedTuple, Union > > +from pydantic.alias_generators import to_pascal This is using pydantic, but it's only added in the subsequent patch. > from scapy.layers.inet import IP # type: ignore[import-untyped] > from scapy.layers.l2 import Ether # type: ignore[import-untyped] > from scapy.packet import Packet, Padding # type: ignore[import-untyped] > +from typing_extensions import Self > > from framework.testbed_model.port import Port, PortLink > from framework.testbed_model.sut_node import SutNode > @@ -365,3 +376,172 @@ 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 > + > + > +class TestCaseVariant(Enum): > + """Enum representing the variant of the test case.""" > + > + #: > + FUNCTIONAL = auto() > + #: > + PERFORMANCE = auto() > + > + > +class TestCase(NamedTuple): > + """Tuple representing a test case.""" > + > + #: The name of the test case without prefix > + name: str > + #: The reference to the function > + function_type: FunctionType I had to read almost the whole patch to understand what this is. It's not the type of a function, it's the function object, which is what the docstring says, but I glossed over that. This should be just function or maybe function_obj. > + #: The test case variant > + variant: TestCaseVariant > + > + > +@dataclass > +class TestSuiteSpec: > + """A class defining the specification of a test suite. > + > + Apart from defining all the specs of a test suite, a helper function :meth:`discover_all` is > + provided to automatically discover all the available test suites. > + We should probably document the assumption that there's only one TestCase class in a test case module. > + Attributes: > + module_name: The name of the test suite's module. > + """ > + > + #: > + TEST_SUITES_PACKAGE_NAME = "tests" Formally speaking, the tests dir doesn't have an __init__.py file in it, so it isn't a package, but the name is fine. > + #: > + TEST_SUITE_MODULE_PREFIX = "TestSuite_" > + #: > + TEST_SUITE_CLASS_PREFIX = "Test" > + #: > + TEST_CASE_METHOD_PREFIX = "test_" > + #: > + FUNC_TEST_CASE_REGEX = r"test_(?!perf_)" > + #: > + PERF_TEST_CASE_REGEX = r"test_perf_" > + These are common to all test suites, so they should be class variables. I'm also wondering whether these should be documented in the module level docstring. It makes sense that we document there what a subclass is supposed to look like (and where it's supposed to be located by default). If we do this, we may need to move parts of the class's docstring as well. > + module_name: str > + > + @cached_property Nice touch, we are using our own implementation of this elsewhere, so maybe we should create a ticket to update those to use @cached_property instead. > + def name(self) -> str: TestSuiteSpec.name really sound the name of a TestSuite, so I'd rename this to module_name. > + """The name of the test suite's module.""" > + return self.module_name[len(self.TEST_SUITE_MODULE_PREFIX) :] > + > + @cached_property > + def module_type(self) -> ModuleType: This isn't a module type, just an instance of the module object, right? Could be named just module. > + """A reference to the test suite's module.""" > + return import_module(f"{self.TEST_SUITES_PACKAGE_NAME}.{self.module_name}") > + > + @cached_property > + def class_name(self) -> str: > + """The name of the test suite's class.""" > + return f"{self.TEST_SUITE_CLASS_PREFIX}{to_pascal(self.name)}" > + > + @cached_property > + def class_type(self) -> type[TestSuite]: Class type would be the type of the class, but this is just the class, right? Could be named just class. > + """A reference to the test suite's class.""" > + > + def is_test_suite(obj) -> bool: > + """Check whether `obj` is a :class:`TestSuite`. > + > + The `obj` is a subclass of :class:`TestSuite`, but not :class:`TestSuite` itself. > + > + Args: > + obj: The object to be checked. > + > + Returns: > + :data:`True` if `obj` is a subclass of `TestSuite`. > + """ > + try: > + if issubclass(obj, TestSuite) and obj is not TestSuite: > + return True > + except TypeError: > + return False > + return False > + > + for class_name, class_type in inspect.getmembers(self.module_type, is_test_suite): > + if class_name == self.class_name: > + return class_type > + > + raise Exception("class not found in eligible test module") This should be a DTS error, maybe InternalError? This doesn't seem like ConfigurationError. It should also say which module and be a proper sentence (capital first letter, end with a dot). > + > + @cached_property > + def test_cases(self) -> list[TestCase]: > + """A list of all the available test cases.""" > + test_cases = [] > + > + functions = inspect.getmembers(self.class_type, inspect.isfunction) > + for fn_name, fn_type in functions: fn_obj instead of fn_type. The type suffix used in the whole module is very confusing. > + if prefix := re.match(self.FUNC_TEST_CASE_REGEX, fn_name): > + variant = TestCaseVariant.FUNCTIONAL > + elif prefix := re.match(self.PERF_TEST_CASE_REGEX, fn_name): > + variant = TestCaseVariant.PERFORMANCE > + else: > + continue > + > + name = fn_name[len(prefix.group(0)) :] Do we actually want to strip the prefix? It could be confusing if it appears in logs. > + test_cases.append(TestCase(name, fn_type, variant)) > + > + return test_cases > + > + @classmethod > + def discover_all( > + cls, package_name: str | None = None, module_prefix: str | None = None > + ) -> list[Self]: > + """Discover all the test suites. > + > + The test suites are discovered in the provided `package_name`. The full module name, > + expected under that package, is prefixed with `module_prefix`. > + The module name is a standard filename with words separated with underscores. > + For each module found, search for a :class:`TestSuite` class which starts > + with `self.TEST_SUITE_CLASS_PREFIX`, continuing with the module name in PascalCase. `self.TEST_SUITE_CLASS_PREFIX` -> :attr:`~TestSuiteSpec.TEST_SUITE_CLASS_PREFIX` > + > + The PascalCase convention applies to abbreviations, acronyms, initialisms and so on:: > + > + OS -> Os > + TCP -> Tcp > + > + Args: > + package_name: The name of the package where to find the test suites, if none is set the I'd separate this into two sentences, with the second one reworded a bit: If :data:`None`, the :attr:`~TestSuiteSpec.TEST_SUITES_PACKAGE_NAME` constant is used. > + constant :attr:`~TestSuiteSpec.TEST_SUITES_PACKAGE_NAME` is used instead. > + module_prefix: The name prefix defining the test suite module, if none is set the Same here. > + constant :attr:`~TestSuiteSpec.TEST_SUITE_MODULE_PREFIX` is used instead. > + > + Returns: > + A list containing all the discovered test suites. > + """ > + if package_name is None: > + package_name = cls.TEST_SUITES_PACKAGE_NAME > + if module_prefix is None: > + module_prefix = cls.TEST_SUITE_MODULE_PREFIX > + > + test_suites = [] > + > + test_suites_pkg = import_module(package_name) > + for _, module_name, is_pkg in iter_modules(test_suites_pkg.__path__): > + if not module_name.startswith(module_prefix) or is_pkg: > + continue > + > + test_suite = cls(module_name) > + try: > + if test_suite.class_type: > + test_suites.append(test_suite) > + except Exception: > + pass It may be beneficial to log a warning that we found a {module_prefix} test suite module without any actual valid test suites. > + > + return test_suites > + > + > +AVAILABLE_TEST_SUITES: list[TestSuiteSpec] = TestSuiteSpec.discover_all() > +"""Constant to store all the available, discovered and imported test suites. > + > +The test suites should be gathered from this list to avoid importing more than once. > +""" We could store this in TestSuiteSpec itself. This would allow us to move the find_by_name function into it and also not import everything at once, but only what's needed if it hadn't been imported before, but maybe we don't want to do that since we lose the verification aspect. I'm just not a fan of code being executed when we import a module, since we didn't call anything, it just sorta happened. Looks like this is used when parsing configuration, so we could do the full scan using @cached_property and that way it'll be the best of both worlds. > + > + > +def find_by_name(name: str) -> TestSuiteSpec | None: It should be clearer from the name/args/docstring that we're trying to find the test suite by module name. > + """Find a requested test suite by name from the available ones.""" > + test_suites = filter(lambda t: t.name == name, AVAILABLE_TEST_SUITES) A list comprehension would be easier to understand I think (mostly because it would remove the question of why do it this way instead of list comprehension): test_suite_specs = [test_suite_spec for test_suite_spec in AVAILABLE_TEST_SUITES if test_suite_spec.name == name] > + return next(test_suites, None) And then test_suite_specs[0] if test_suite_specs else None