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

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

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