Hi Juraj,
Here's my review. Excuse me for the unordinary format, but I thought
it would have just been easier to convey my suggestions through code.
Apart from the smaller suggestions, the most important one I think is
that we should make sure to enforce type checking (and hinting).
Overall I like your approach, but I think it'd be better to initialise
all the required variables per test case, so we can access them
directly without doing checks everytime. The easiest approach I can see
to do this though, is to decorate all the test cases, for example
through @test. It'd be a somewhat important change as it changes the
test writing API, but it brings some improvements while making the
system more resilient.
The comments in the code are part of the review and may refer to
either your code or mine. The diff is in working order, so you could
test the functionality if you wished.
Best regards,
Luca
---
diff --git a/dts/framework/remote_session/__init__.py
b/dts/framework/remote_session/__init__.py
index f18a9f2259..d4dfed3a58 100644
--- a/dts/framework/remote_session/__init__.py
+++ b/dts/framework/remote_session/__init__.py
@@ -22,6 +22,9 @@
from .python_shell import PythonShell
from .remote_session import CommandResult, RemoteSession
from .ssh_session import SSHSession
+
+# in my testpmd params series these imports are removed as they promote eager
module loading,
+# significantly increasing the chances of circular dependencies
from .testpmd_shell import NicCapability, TestPmdShell
diff --git a/dts/framework/remote_session/testpmd_shell.py b/dts/framework/remote_session/testpmd_shell.py
index f6783af621..2b87e2e5c8 100644
--- a/dts/framework/remote_session/testpmd_shell.py
+++ b/dts/framework/remote_session/testpmd_shell.py
@@ -16,7 +16,6 @@
"""
import time
-from collections.abc import MutableSet
from enum import Enum, auto
from functools import partial
from pathlib import PurePath
@@ -232,9 +231,8 @@ def close(self) -> None:
self.send_command("quit", "")
return super().close()
- def get_capas_rxq(
- self, supported_capabilities: MutableSet, unsupported_capabilities:
MutableSet
- ) -> None:
+ # the built-in `set` is a mutable set. Is there an advantage to using
MutableSet?
+ def get_capas_rxq(self, supported_capabilities: set,
unsupported_capabilities: set) -> None:
"""Get all rxq capabilities and divide them into supported and
unsupported.
Args:
@@ -243,6 +241,7 @@ def get_capas_rxq(
not supported will be stored.
"""
self._logger.debug("Getting rxq capabilities.")
+ # this assumes that the used ports are all the same. Could this be of
concern?
command = "show rxq info 0 0"
rxq_info = self.send_command(command)
for line in rxq_info.split("\n"):
@@ -270,4 +269,6 @@ class NicCapability(Enum):
`unsupported_capabilities` based on their support.
"""
+ # partial is just a high-order function that pre-fills the arguments... but we have no arguments
+ # here? Was this intentional?
scattered_rx = partial(TestPmdShell.get_capas_rxq)
diff --git a/dts/framework/runner.py b/dts/framework/runner.py
index 7407ea30b8..db02735ee9 100644
--- a/dts/framework/runner.py
+++ b/dts/framework/runner.py
@@ -20,12 +20,13 @@
import importlib
import inspect
import os
-import re
import sys
from pathlib import Path
from types import MethodType
from typing import Iterable, Sequence
+from framework.remote_session.testpmd_shell import NicCapability
+
from .config import (
BuildTargetConfiguration,
Configuration,
@@ -50,7 +51,7 @@
TestSuiteResult,
TestSuiteWithCases,
)
-from .test_suite import TestSuite
+from .test_suite import TestCase, TestCaseType, TestSuite
from .testbed_model import SutNode, TGNode
@@ -305,7 +306,7 @@ def is_test_suite(object) -> bool:
def _filter_test_cases(
self, test_suite_class: type[TestSuite], test_cases_to_run:
Sequence[str]
- ) -> tuple[list[MethodType], list[MethodType]]:
+ ) -> tuple[list[type[TestCase]], list[type[TestCase]]]:
"""Filter `test_cases_to_run` from `test_suite_class`.
There are two rounds of filtering if `test_cases_to_run` is not empty.
@@ -327,30 +328,28 @@ def _filter_test_cases(
"""
func_test_cases = []
perf_test_cases = []
- name_method_tuples = inspect.getmembers(test_suite_class,
inspect.isfunction)
+ # By introducing the TestCase class this could be simplified.
+ # Also adding separation of concerns, delegating the auto discovery of
test cases to the
+ # test suite class.
if test_cases_to_run:
- name_method_tuples = [
- (name, method) for name, method in name_method_tuples if name
in test_cases_to_run
- ]
- if len(name_method_tuples) < len(test_cases_to_run):
+ test_cases = test_suite_class.get_test_cases(lambda t: t.__name__
in test_cases_to_run)
+ if len(test_cases) < len(test_cases_to_run):
missing_test_cases = set(test_cases_to_run) - {
- name for name, _ in name_method_tuples
+ test_case.__name__ for test_case in test_cases
}
raise ConfigurationError(
f"Test cases {missing_test_cases} not found among methods "
f"of {test_suite_class.__name__}."
)
+ else:
+ test_cases = test_suite_class.get_test_cases()
- for test_case_name, test_case_method in name_method_tuples:
- if re.match(self._func_test_case_regex, test_case_name):
- func_test_cases.append(test_case_method)
- elif re.match(self._perf_test_case_regex, test_case_name):
- perf_test_cases.append(test_case_method)
- elif test_cases_to_run:
- raise ConfigurationError(
- f"Method '{test_case_name}' matches neither "
- f"a functional nor a performance test case name."
- )
+ for test_case in test_cases:
+ match test_case.type:
+ case TestCaseType.PERFORMANCE:
+ perf_test_cases.append(test_case)
+ case TestCaseType.FUNCTIONAL:
+ func_test_cases.append(test_case)
return func_test_cases, perf_test_cases
@@ -489,6 +488,17 @@ def _run_build_target(
self._logger.exception("Build target teardown failed.")
build_target_result.update_teardown(Result.FAIL, e)
+ def _get_supported_required_capabilities(
+ self, sut_node: SutNode, test_suites_with_cases:
Iterable[TestSuiteWithCases]
+ ) -> set[NicCapability]:
+ required_capabilities = set()
+ for test_suite_with_cases in test_suites_with_cases:
+
required_capabilities.update(test_suite_with_cases.req_capabilities)
+ self._logger.debug(f"Found required capabilities:
{required_capabilities}")
+ if required_capabilities:
+ return sut_node.get_supported_capabilities(required_capabilities)
+ return set()
+
def _run_test_suites(
self,
sut_node: SutNode,
@@ -518,18 +528,17 @@ def _run_test_suites(
test_suites_with_cases: The test suites with test cases to run.
"""
end_build_target = False
- required_capabilities = set()
- supported_capabilities = set()
- for test_suite_with_cases in test_suites_with_cases:
-
required_capabilities.update(test_suite_with_cases.req_capabilities)
- self._logger.debug(f"Found required capabilities:
{required_capabilities}")
- if required_capabilities:
- supported_capabilities =
sut_node.get_supported_capabilities(required_capabilities)
+ # extract this logic
+ supported_capabilities = self._get_supported_required_capabilities(
+ sut_node, test_suites_with_cases
+ )
for test_suite_with_cases in test_suites_with_cases:
test_suite_result =
build_target_result.add_test_suite(test_suite_with_cases)
+ # not against this but perhaps something more explanatory like
mark_skip_to_unsupported?
test_suite_with_cases.mark_skip(supported_capabilities)
try:
- if test_suite_with_cases:
+ # is_any_supported is more self-explanatory than an ambiguous
boolean check
+ if test_suite_with_cases.is_any_supported():
self._run_test_suite(
sut_node,
tg_node,
@@ -619,7 +628,7 @@ def _run_test_suite(
def _execute_test_suite(
self,
test_suite: TestSuite,
- test_cases: Iterable[MethodType],
+ test_cases: Iterable[type[TestCase]],
test_suite_result: TestSuiteResult,
) -> None:
"""Execute all `test_cases` in `test_suite`.
@@ -640,7 +649,7 @@ def _execute_test_suite(
test_case_result = test_suite_result.add_test_case(test_case_name)
all_attempts = SETTINGS.re_run + 1
attempt_nr = 1
- if TestSuiteWithCases.should_not_be_skipped(test_case_method):
+ if not test_case_method.skip:
self._run_test_case(test_suite, test_case_method,
test_case_result)
while not test_case_result and attempt_nr < all_attempts:
attempt_nr += 1
@@ -656,7 +665,7 @@ def _execute_test_suite(
def _run_test_case(
self,
test_suite: TestSuite,
- test_case_method: MethodType,
+ test_case_method: type[TestCase],
test_case_result: TestCaseResult,
) -> None:
"""Setup, execute and teardown `test_case_method` from `test_suite`.
@@ -702,7 +711,7 @@ def _run_test_case(
def _execute_test_case(
self,
test_suite: TestSuite,
- test_case_method: MethodType,
+ test_case_method: type[TestCase],
test_case_result: TestCaseResult,
) -> None:
"""Execute `test_case_method` from `test_suite`, record the result and
handle failures.
@@ -716,7 +725,8 @@ def _execute_test_case(
test_case_name = test_case_method.__name__
try:
self._logger.info(f"Starting test case execution:
{test_case_name}")
- test_case_method(test_suite)
+ # Explicit method binding is now required, otherwise mypy complains
+ MethodType(test_case_method, test_suite)()
test_case_result.update(Result.PASS)
self._logger.info(f"Test case execution PASSED: {test_case_name}")
diff --git a/dts/framework/test_result.py b/dts/framework/test_result.py
index 26c58a8606..82b2dc17b8 100644
--- a/dts/framework/test_result.py
+++ b/dts/framework/test_result.py
@@ -24,10 +24,9 @@
"""
import os.path
-from collections.abc import MutableSequence, MutableSet
+from collections.abc import MutableSequence
from dataclasses import dataclass, field
from enum import Enum, auto
-from types import MethodType
from typing import Union
from framework.remote_session import NicCapability
@@ -46,7 +45,7 @@
from .exception import DTSError, ErrorSeverity
from .logger import DTSLogger
from .settings import SETTINGS
-from .test_suite import TestSuite
+from .test_suite import TestCase, TestSuite
@dataclass(slots=True, frozen=True)
@@ -65,7 +64,7 @@ class is to hold a subset of test cases (which could be all
test cases) because
"""
test_suite_class: type[TestSuite]
- test_cases: list[MethodType]
+ test_cases: list[type[TestCase]]
req_capabilities: set[NicCapability] = field(default_factory=set,
init=False)
def __post_init__(self):
@@ -86,7 +85,7 @@ def create_config(self) -> TestSuiteConfig:
test_cases=[test_case.__name__ for test_case in self.test_cases],
)
- def mark_skip(self, supported_capabilities: MutableSet[NicCapability]) -> None:
+ def mark_skip(self, supported_capabilities: set[NicCapability]) -> None:
"""Mark the test suite and test cases to be skipped.
The mark is applied is object to be skipped requires any capabilities and at least one of
@@ -95,26 +94,15 @@ def mark_skip(self, supported_capabilities:
MutableSet[NicCapability]) -> None:
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_capabilities:
+ for test_object in [self.test_suite_class, *self.test_cases]:
+ # mypy picks up that both TestSuite and TestCase implement
RequiresCapabilities and is
+ # happy. We could explicitly type hint the list for readability if
we preferred.
+ if test_object.required_capabilities - supported_capabilities:
+ # the latest version of mypy complains about this in the
original code because
+ # test_object can be distinct classes. The added protocol
solves this
test_object.skip = True
- @staticmethod
- def should_not_be_skipped(test_object: Union[type[TestSuite] | MethodType])
-> 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 skipped,
:data:`False` otherwise.
- """
- return not getattr(test_object, "skip", False)
-
- def __bool__(self) -> bool:
+ def is_any_supported(self) -> bool:
"""The truth value is determined by whether the test suite should be
run.
Returns:
@@ -122,10 +110,12 @@ def __bool__(self) -> bool:
"""
found_test_case_to_run = False
for test_case in self.test_cases:
- if self.should_not_be_skipped(test_case):
+ # mypy and the TestCase decorators now ensure that the attributes
always exist
+ # so the above static method is no longer needed
+ if not test_case.skip:
found_test_case_to_run = True
break
- return found_test_case_to_run and
self.should_not_be_skipped(self.test_suite_class)
+ return found_test_case_to_run and not self.test_suite_class.skip
class Result(Enum):
diff --git a/dts/framework/test_suite.py b/dts/framework/test_suite.py
index 07cdd294b9..d03f8db712 100644
--- a/dts/framework/test_suite.py
+++ b/dts/framework/test_suite.py
@@ -13,9 +13,10 @@
* Test case verification.
"""
-from collections.abc import Callable
+from enum import Enum, auto
+import inspect
from ipaddress import IPv4Interface, IPv6Interface, ip_interface
-from typing import ClassVar, Union
+from typing import Callable, ClassVar, Protocol, TypeVar, Union, cast
from scapy.layers.inet import IP # type: ignore[import]
from scapy.layers.l2 import Ether # type: ignore[import]
@@ -30,7 +31,14 @@
from .utils import get_packet_summaries
-class TestSuite(object):
+# a protocol/interface for common fields. The defaults are correctly picked up
+# by child concrete classes.
+class RequiresCapabilities(Protocol):
+ skip: ClassVar[bool] = False
+ required_capabilities: ClassVar[set[NicCapability]] = set()
+
+
+class TestSuite(RequiresCapabilities):
"""The base class with building blocks needed by most test cases.
* Test suite setup/cleanup methods to override,
@@ -65,7 +73,6 @@ 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 current
build target.
is_blocking: ClassVar[bool] = False
- skip: bool
_logger: DTSLogger
_port_links: list[PortLink]
_sut_port_ingress: Port
@@ -93,7 +100,7 @@ def __init__(
"""
self.sut_node = sut_node
self.tg_node = tg_node
- self.skip = False
+
self._logger = get_dts_logger(self.__class__.__name__)
self._port_links = []
self._process_links()
@@ -110,6 +117,23 @@ def __init__(
self._tg_ip_address_egress = ip_interface("192.168.100.3/24")
self._tg_ip_address_ingress = ip_interface("192.168.101.3/24")
+ # move the discovery of the test cases to TestSuite for separation of concerns
+ @classmethod
+ def get_test_cases(
+ cls, filter: Callable[[type["TestCase"]], bool] | None = None
+ ) -> set[type["TestCase"]]:
+ test_cases = set()
+ for _, method in inspect.getmembers(cls, inspect.isfunction):
+ # at runtime the function remains a function with custom
attributes. An ininstance check
+ # unfortunately wouldn't work. Therefore force and test for the
presence of TestCaseType
+ test_case = cast(type[TestCase], method)
+ try:
+ if isinstance(test_case.type, TestCaseType) and (not filter or
filter(test_case)):
+ test_cases.add(test_case)
+ except AttributeError:
+ pass
+ return test_cases
+
def _process_links(self) -> None:
"""Construct links between SUT and TG ports."""
for sut_port in self.sut_node.ports:
@@ -367,8 +391,46 @@ def _verify_l3_packet(self, received_packet: IP,
expected_packet: IP) -> bool:
return True
-def requires(capability: NicCapability) -> Callable:
- """A decorator that marks the decorated test case or test suite as one to
be skipped.
+# generic type for a method of an instance of TestSuite
+M = TypeVar("M", bound=Callable[[TestSuite], None])
+
+
+class TestCaseType(Enum):
+ FUNCTIONAL = auto()
+ PERFORMANCE = auto()
+
+
+# the protocol here is merely for static type checking and hinting.
+# the defaults don't get applied to functions like it does with inheriting
+# classes. The cast here is necessary as we are forcing mypy to understand
+# we want to treat the function as TestCase. When all the attributes are
correctly
+# initialised this is fine.
+class TestCase(RequiresCapabilities, Protocol[M]):
+ type: ClassVar[TestCaseType]
+ __call__: M # necessary for mypy so that it can treat this class as the
function it's shadowing
+
+ @staticmethod
+ def make_decorator(test_case_type: TestCaseType):
+ def _decorator(func: M) -> type[TestCase]:
+ test_case = cast(type[TestCase], func)
+ test_case.skip = False
+ test_case.required_capabilities = set()
+ test_case.type = test_case_type
+ return test_case
+
+ return _decorator
+
+
+# this now requires to tag all test cases with the following decorators and
the advantage of:
+# - a cleaner auto discovery
+# - enforcing the TestCase type
+# - initialising all the required attributes for test cases
+test = TestCase.make_decorator(TestCaseType.FUNCTIONAL)
+perf_test = TestCase.make_decorator(TestCaseType.PERFORMANCE)
+
+
+def requires(*capabilities: NicCapability):
+ """A decorator that marks the decorated test case or test suite as
skippable.
Args:
The capability that's required by the decorated test case or test
suite.
@@ -377,11 +439,8 @@ def requires(capability: NicCapability) -> Callable:
The decorated function.
"""
- def add_req_capa(func) -> Callable:
- if hasattr(func, "req_capa"):
- func.req_capa.append(capability)
- else:
- func.req_capa = [capability]
- return func
+ def add_req_capa(test_case: type[TestCase]) -> type[TestCase]:
+ test_case.required_capabilities.update(capabilities)
+ return test_case
return add_req_capa
diff --git a/dts/tests/TestSuite_hello_world.py
b/dts/tests/TestSuite_hello_world.py
index 31b1564582..61533665f8 100644
--- a/dts/tests/TestSuite_hello_world.py
+++ b/dts/tests/TestSuite_hello_world.py
@@ -8,7 +8,7 @@
"""
from framework.remote_session import NicCapability
-from framework.test_suite import TestSuite, requires
+from framework.test_suite import TestSuite, requires, test
from framework.testbed_model import (
LogicalCoreCount,
LogicalCoreCountFilter,
@@ -28,7 +28,8 @@ def set_up_suite(self) -> None:
self.app_helloworld_path = self.sut_node.build_dpdk_app("helloworld")
@requires(NicCapability.scattered_rx)
- def test_hello_world_single_core(self) -> None:
+ @test
+ def hello_world_single_core(self) -> None:
"""Single core test case.
Steps:
@@ -47,7 +48,8 @@ def test_hello_world_single_core(self) -> None:
f"helloworld didn't start on lcore{lcores[0]}",
)
- def test_hello_world_all_cores(self) -> None:
+ @test
+ def hello_world_all_cores(self) -> None:
"""All cores test case.
Steps:
