Re: [PATCH v3 04/12] dts: add mechanism to skip test cases or suites

[Jeremy Spewock <jspewock@iol.unh.edu>]

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>
>