[v3,04/12] dts: add mechanism to skip test cases or suites
Checks
Commit Message
If a test case is not relevant to the testing environment (such as when
a NIC doesn't support a tested feature), the framework should skip it.
The mechanism is a skeleton without actual logic that would set a test
case or suite to be skipped.
The mechanism uses a protocol to extend test suites and test cases with
additional attributes that track whether the test case or suite should
be skipped the reason for skipping it.
Also update the results module with the new SKIP result.
Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
---
dts/framework/runner.py | 34 +++++++---
dts/framework/test_result.py | 77 ++++++++++++++---------
dts/framework/test_suite.py | 7 ++-
dts/framework/testbed_model/capability.py | 28 +++++++++
4 files changed, 109 insertions(+), 37 deletions(-)
create mode 100644 dts/framework/testbed_model/capability.py
Comments
On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš
<juraj.linkes@pantheon.tech> wrote:
<snip>
> --- a/dts/framework/test_result.py
> +++ b/dts/framework/test_result.py
> @@ -75,6 +75,20 @@ def create_config(self) -> TestSuiteConfig:
> test_cases=[test_case.__name__ for test_case in self.test_cases],
> )
>
> + @property
> + def skip(self) -> bool:
> + """Skip the test suite if all test cases or the suite itself are to be skipped.
> +
> + Returns:
> + :data:`True` if the test suite should be skipped, :data:`False` otherwise.
> + """
> + all_test_cases_skipped = True
> + for test_case in self.test_cases:
> + if not test_case.skip:
> + all_test_cases_skipped = False
> + break
You could also potentially implement this using the built-in `all()`
function. It would become a simple one-liner like
`all_test_cases_skipped = all(test_case.skip for test_case in
self.test_cases)`. That's probably short enough to even just put in
the return statement though if you wanted to.
> + return all_test_cases_skipped or self.test_suite_class.skip
> +
>
> class Result(Enum):
> """The possible states that a setup, a teardown or a test case may end up in."""
> @@ -86,12 +100,12 @@ class Result(Enum):
> #:
> ERROR = auto()
> #:
> - SKIP = auto()
> - #:
> BLOCK = auto()
> + #:
> + SKIP = auto()
>
> def __bool__(self) -> bool:
> - """Only PASS is True."""
> + """Only :attr:`PASS` is True."""
> return self is self.PASS
>
>
> @@ -169,12 +183,13 @@ def update_setup(self, result: Result, error: Exception | None = None) -> None:
> self.setup_result.result = result
> self.setup_result.error = error
>
> - if result in [Result.BLOCK, Result.ERROR, Result.FAIL]:
> - self.update_teardown(Result.BLOCK)
> - self._block_result()
> + if result != Result.PASS:
> + result_to_mark = Result.BLOCK if result != Result.SKIP else 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:
Is it worth adding the type annotation for `result` here and to the
other places where this is implemented? I guess it doesn't matter that
much since it is a private method.
> + """Mark the result as well as the child result as `result`.
Are these methods even marking their own result or only their
children? It seems like it's only really updating the children
recursively and its result would have already been updated before this
was called.
>
> The blocking of child results should be done in overloaded methods.
> """
<snip>
>
On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš <juraj.linkes@pantheon.tech>
wrote:
> If a test case is not relevant to the testing environment (such as when
> a NIC doesn't support a tested feature), the framework should skip it.
> The mechanism is a skeleton without actual logic that would set a test
> case or suite to be skipped.
>
> The mechanism uses a protocol to extend test suites and test cases with
> additional attributes that track whether the test case or suite should
> be skipped the reason for skipping it.
>
> Also update the results module with the new SKIP result.
>
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
>
Reviewed-by: Dean Marx <dmarx@iol.unh.edu>
On 26. 8. 2024 18:52, Jeremy Spewock wrote:
> On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš
> <juraj.linkes@pantheon.tech> wrote:
> <snip>
>> --- a/dts/framework/test_result.py
>> +++ b/dts/framework/test_result.py
>> @@ -75,6 +75,20 @@ def create_config(self) -> TestSuiteConfig:
>> test_cases=[test_case.__name__ for test_case in self.test_cases],
>> )
>>
>> + @property
>> + def skip(self) -> bool:
>> + """Skip the test suite if all test cases or the suite itself are to be skipped.
>> +
>> + Returns:
>> + :data:`True` if the test suite should be skipped, :data:`False` otherwise.
>> + """
>> + all_test_cases_skipped = True
>> + for test_case in self.test_cases:
>> + if not test_case.skip:
>> + all_test_cases_skipped = False
>> + break
>
> You could also potentially implement this using the built-in `all()`
> function. It would become a simple one-liner like
> `all_test_cases_skipped = all(test_case.skip for test_case in
> self.test_cases)`. That's probably short enough to even just put in
> the return statement though if you wanted to.
>
Good catch, I'll use any() here.
>> + return all_test_cases_skipped or self.test_suite_class.skip
>> +
>>
>> class Result(Enum):
>> """The possible states that a setup, a teardown or a test case may end up in."""
>> @@ -86,12 +100,12 @@ class Result(Enum):
>> #:
>> ERROR = auto()
>> #:
>> - SKIP = auto()
>> - #:
>> BLOCK = auto()
>> + #:
>> + SKIP = auto()
>>
>> def __bool__(self) -> bool:
>> - """Only PASS is True."""
>> + """Only :attr:`PASS` is True."""
>> return self is self.PASS
>>
>>
>> @@ -169,12 +183,13 @@ def update_setup(self, result: Result, error: Exception | None = None) -> None:
>> self.setup_result.result = result
>> self.setup_result.error = error
>>
>> - if result in [Result.BLOCK, Result.ERROR, Result.FAIL]:
>> - self.update_teardown(Result.BLOCK)
>> - self._block_result()
>> + if result != Result.PASS:
>> + result_to_mark = Result.BLOCK if result != Result.SKIP else 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:
>
> Is it worth adding the type annotation for `result` here and to the
> other places where this is implemented? I guess it doesn't matter that
> much since it is a private method.
>
I didn't add it precisely because it's a private method and it's pretty
self explanatory.
>> + """Mark the result as well as the child result as `result`.
>
> Are these methods even marking their own result or only their
> children? It seems like it's only really updating the children
> recursively and its result would have already been updated before this
> was called.
>
It's supposed to be just their result which is actually the result of
the children in all but the TestCaseResult classes. Conceptually, each
results level should contains these:
1. the result of setup
2. the result of teardown
3. the result of the level itself (outside of setup and teardown)
The result of the level itself is what's supposed to be set here. The
thing is we're making the child results for non-test cases and the
result of the test cases for test cases. Maybe I only need to update the
docstring.
>>
>> The blocking of child results should be done in overloaded methods.
>> """
> <snip>
>>
On Thu, Sep 5, 2024 at 5:23 AM Juraj Linkeš <juraj.linkes@pantheon.tech> wrote:
<snip>
> >> + def _mark_results(self, result) -> None:
> >
> > Is it worth adding the type annotation for `result` here and to the
> > other places where this is implemented? I guess it doesn't matter that
> > much since it is a private method.
> >
>
> I didn't add it precisely because it's a private method and it's pretty
> self explanatory.
Makes sense.
>
> >> + """Mark the result as well as the child result as `result`.
> >
> > Are these methods even marking their own result or only their
> > children? It seems like it's only really updating the children
> > recursively and its result would have already been updated before this
> > was called.
> >
>
> It's supposed to be just their result which is actually the result of
> the children in all but the TestCaseResult classes. Conceptually, each
Right, of course. Sorry, I was thinking too literally about this
isolated method, with the context that that's how the result of the
level itself is decided, this makes way more sense.
> results level should contains these:
> 1. the result of setup
> 2. the result of teardown
> 3. the result of the level itself (outside of setup and teardown)
>
> The result of the level itself is what's supposed to be set here. The
> thing is we're making the child results for non-test cases and the
> result of the test cases for test cases. Maybe I only need to update the
> docstring.
You probably don't even need to update the doc-string, this seems more
like a mistake on my part :).
>
> >>
> >> The blocking of child results should be done in overloaded methods.
> >> """
> > <snip>
> >>
>
@@ -477,7 +477,20 @@ def _run_test_suites(
for test_suite_with_cases in test_suites_with_cases:
test_suite_result = build_target_result.add_test_suite(test_suite_with_cases)
try:
- self._run_test_suite(sut_node, tg_node, test_suite_result, test_suite_with_cases)
+ if not test_suite_with_cases.skip:
+ 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__}'. Reason: "
+ f"{test_suite_with_cases.test_suite_class.skip_reason}"
+ )
+ test_suite_result.update_setup(Result.SKIP)
except BlockingTestSuiteError as e:
self._logger.exception(
f"An error occurred within {test_suite_with_cases.test_suite_class.__name__}. "
@@ -576,14 +589,21 @@ 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
- self._run_test_case(test_suite, test_case, test_case_result)
- while not test_case_result and attempt_nr < all_attempts:
- attempt_nr += 1
+ if not test_case.skip:
+ self._run_test_case(test_suite, test_case, test_case_result)
+ while not test_case_result and attempt_nr < all_attempts:
+ attempt_nr += 1
+ self._logger.info(
+ f"Re-running FAILED test case '{test_case_name}'. "
+ f"Attempt number {attempt_nr} out of {all_attempts}."
+ )
+ self._run_test_case(test_suite, test_case, test_case_result)
+ else:
self._logger.info(
- f"Re-running FAILED test case '{test_case_name}'. "
- f"Attempt number {attempt_nr} out of {all_attempts}."
+ f"Test case execution SKIPPED: {test_case_name}. Reason: "
+ f"{test_case.skip_reason}"
)
- self._run_test_case(test_suite, test_case, test_case_result)
+ test_case_result.update_setup(Result.SKIP)
def _run_test_case(
self,
@@ -75,6 +75,20 @@ def create_config(self) -> TestSuiteConfig:
test_cases=[test_case.__name__ for test_case in self.test_cases],
)
+ @property
+ def skip(self) -> bool:
+ """Skip the test suite if all test cases or the suite itself are to be skipped.
+
+ Returns:
+ :data:`True` if the test suite should be skipped, :data:`False` otherwise.
+ """
+ all_test_cases_skipped = True
+ for test_case in self.test_cases:
+ if not test_case.skip:
+ all_test_cases_skipped = False
+ break
+ return all_test_cases_skipped or self.test_suite_class.skip
+
class Result(Enum):
"""The possible states that a setup, a teardown or a test case may end up in."""
@@ -86,12 +100,12 @@ class Result(Enum):
#:
ERROR = auto()
#:
- SKIP = auto()
- #:
BLOCK = auto()
+ #:
+ SKIP = auto()
def __bool__(self) -> bool:
- """Only PASS is True."""
+ """Only :attr:`PASS` is True."""
return self is self.PASS
@@ -169,12 +183,13 @@ def update_setup(self, result: Result, error: Exception | None = None) -> None:
self.setup_result.result = result
self.setup_result.error = error
- if result in [Result.BLOCK, Result.ERROR, Result.FAIL]:
- self.update_teardown(Result.BLOCK)
- self._block_result()
+ if result != Result.PASS:
+ result_to_mark = Result.BLOCK if result != Result.SKIP else 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 methods.
"""
@@ -391,11 +406,11 @@ def add_sut_info(self, sut_info: NodeInfo) -> None:
self.sut_os_version = sut_info.os_version
self.sut_kernel_version = 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 = self.add_build_target(build_target)
- child_result.update_setup(Result.BLOCK)
+ child_result.update_setup(result)
class BuildTargetResult(BaseResult):
@@ -465,11 +480,11 @@ def add_build_target_info(self, versions: BuildTargetInfo) -> None:
self.compiler_version = versions.compiler_version
self.dpdk_version = 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 = self.add_test_suite(test_suite_with_cases)
- child_result.update_setup(Result.BLOCK)
+ child_result.update_setup(result)
class TestSuiteResult(BaseResult):
@@ -509,11 +524,11 @@ def add_test_case(self, test_case_name: str) -> "TestCaseResult":
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 = self.add_test_case(test_case_method.__name__)
- child_result.update_setup(Result.BLOCK)
+ child_result.update_setup(result)
class TestCaseResult(BaseResult, FixtureResult):
@@ -567,9 +582,9 @@ def add_stats(self, statistics: "Statistics") -> None:
"""
statistics += 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 case itself passed."""
@@ -583,7 +598,8 @@ class Statistics(dict):
The data are stored in the following keys:
- * **PASS RATE** (:class:`int`) -- The FAIL/PASS ratio of all test cases.
+ * **PASS RATE** (:class:`int`) -- The :attr:`~Result.FAIL`/:attr:`~Result.PASS` ratio
+ of all test cases.
* **DPDK VERSION** (:class:`str`) -- The tested DPDK version.
"""
@@ -600,22 +616,27 @@ def __init__(self, dpdk_version: str | None):
self["DPDK VERSION"] = dpdk_version
def __iadd__(self, other: Result) -> "Statistics":
- """Add a Result to the final count.
+ """Add a :class:`Result` to the final count.
+
+ :attr:`~Result.SKIP` is not taken into account
Example:
- stats: Statistics = Statistics() # empty Statistics
- stats += Result.PASS # add a Result to `stats`
+ stats: Statistics = Statistics() # empty :class:`Statistics`
+ stats += Result.PASS # add a :class:`Result` to `stats`
Args:
- other: The Result to add to this statistics object.
+ other: The :class:`Result` to add to this statistics object.
Returns:
The modified statistics object.
"""
self[other.name] += 1
- self["PASS RATE"] = (
- float(self[Result.PASS.name]) * 100 / sum(self[result.name] for result in Result)
- )
+ if other != Result.SKIP:
+ self["PASS RATE"] = (
+ float(self[Result.PASS.name])
+ * 100
+ / sum([self[result.name] for result in Result if result != Result.SKIP])
+ )
return self
def __str__(self) -> str:
@@ -23,6 +23,7 @@
from scapy.layers.l2 import Ether # type: ignore[import-untyped]
from scapy.packet import Packet, Padding # type: ignore[import-untyped]
+from framework.testbed_model.capability import TestProtocol
from framework.testbed_model.port import Port, PortLink
from framework.testbed_model.sut_node import SutNode
from framework.testbed_model.tg_node import TGNode
@@ -35,7 +36,7 @@
from .utils import get_packet_summaries
-class TestSuite:
+class TestSuite(TestProtocol):
"""The base class with building blocks needed by most test cases.
* Test suite setup/cleanup methods to override,
@@ -445,7 +446,7 @@ class TestCaseType(Enum):
PERFORMANCE = auto()
-class TestCase(Protocol[TestSuiteMethodType]):
+class TestCase(TestProtocol, Protocol[TestSuiteMethodType]):
"""Definition of the test case type for static type checking purposes.
The type is applied to test case functions through a decorator, which casts the decorated
@@ -476,6 +477,8 @@ def make_decorator(
def _decorator(func: TestSuiteMethodType) -> type[TestCase]:
test_case = cast(type[TestCase], func)
+ test_case.skip = cls.skip
+ test_case.skip_reason = cls.skip_reason
test_case.test_type = test_case_type
return test_case
new file mode 100644
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: BSD-3-Clause
+# Copyright(c) 2024 PANTHEON.tech s.r.o.
+
+"""Testbed capabilities.
+
+This module provides a protocol that defines the common attributes of test cases and suites.
+"""
+
+from collections.abc import Sequence
+from typing import ClassVar, Protocol
+
+
+class TestProtocol(Protocol):
+ """Common test suite and test case attributes."""
+
+ #: Whether to skip the test case or suite.
+ skip: ClassVar[bool] = False
+ #: The reason for skipping the test case or suite.
+ skip_reason: ClassVar[str] = ""
+
+ @classmethod
+ def get_test_cases(cls, test_case_sublist: Sequence[str] | None = None) -> tuple[set, set]:
+ """Get test cases. Should be implemented by subclasses containing test cases.
+
+ Raises:
+ NotImplementedError: The subclass does not implement the method.
+ """
+ raise NotImplementedError()