Re: [PATCH 1/5] dts: add TestSuiteSpec class and discovery

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

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 <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> ---
>   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