Re: [PATCH v3 03/12] dts: add test case decorators

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

On 26. 8. 2024 18:50, Jeremy Spewock wrote:
> On Wed, Aug 21, 2024 at 10:53 AM Juraj Linkeš
> <juraj.linkes@pantheon.tech> wrote:
> <snip>
>>   class DTSRunner:
>> @@ -232,9 +231,9 @@ def _get_test_suites_with_cases(
>>
>>           for test_suite_config in test_suite_configs:
>>               test_suite_class = self._get_test_suite_class(test_suite_config.test_suite)
>> -            test_cases = []
>> -            func_test_cases, perf_test_cases = self._filter_test_cases(
>> -                test_suite_class, test_suite_config.test_cases
>> +            test_cases: list[type[TestCase]] = []
> 
> If TestCase is just a class, why is the `type[]` in the annotation
> required? Are these not specific instances of the TestCase class? I
> figured they would need to be in order for you to run the specific
> test case methods. Maybe this has something to do with the class being
> a Protocol?
> 

The *_test decorators return type[TestCase]. The functions (test 
methods) are cast to type[TestCase] (which kinda makes them subclasses 
of TestCase).

This was a suggestion from Luca and I took it as as. Maybe the functions 
could be cast as instances of TestCase, but I didn't try that.

>> +            func_test_cases, perf_test_cases = test_suite_class.get_test_cases(
>> +                test_suite_config.test_cases
>>               )
>>               if func:
>>                   test_cases.extend(func_test_cases)
>> @@ -309,57 +308,6 @@ def is_test_suite(object) -> bool:
>>               f"Couldn't find any valid test suites in {test_suite_module.__name__}."
>>           )
>>
> <snip>
>> @@ -120,6 +123,68 @@ def _process_links(self) -> None:
>>                   ):
>>                       self._port_links.append(PortLink(sut_port=sut_port, tg_port=tg_port))
>>
>> +    @classmethod
>> +    def get_test_cases(
>> +        cls, test_case_sublist: Sequence[str] | None = None
>> +    ) -> tuple[set[type["TestCase"]], set[type["TestCase"]]]:
>> +        """Filter `test_case_subset` from this class.
>> +
>> +        Test cases are regular (or bound) methods decorated with :func:`func_test`
>> +        or :func:`perf_test`.
>> +
>> +        Args:
>> +            test_case_sublist: Test case names to filter from this class.
>> +                If empty or :data:`None`, return all test cases.
>> +
>> +        Returns:
>> +            The filtered test case functions. This method returns functions as opposed to methods,
>> +            as methods are bound to instances and this method only has access to the class.
>> +
>> +        Raises:
>> +            ConfigurationError: If a test case from `test_case_subset` is not found.
>> +        """
>> +
> <snip>
>> +        for test_case_name, test_case_function in inspect.getmembers(cls, is_test_case):
>> +            if test_case_name in test_case_sublist_copy:
>> +                # if test_case_sublist_copy is non-empty, remove the found test case
>> +                # so that we can look at the remainder at the end
>> +                test_case_sublist_copy.remove(test_case_name)
>> +            elif test_case_sublist:
>> +                # if the original list is not empty (meaning we're filtering test cases),
>> +                # we're dealing with a test case we would've
> 
> I think this part of the comment about "we're dealing with a test case
> we would've removed in the other branch"  confused me a little bit. It
> could just be a me thing, but I think this would have been more clear
> for me if it was something more like "The original list is not empty
> (meaning we're filtering test cases). Since we didn't remove this test
> case in the other branch, it doesn't match the filter and we don't
> want to run it."
> 

We should remove any confusion. I'll change it - your wording sound good.

>> +                # removed in the other branch; since we didn't, we don't want to run it
>> +                continue
>> +
>> +            match test_case_function.test_type:
>> +                case TestCaseType.PERFORMANCE:
>> +                    perf_test_cases.add(test_case_function)
>> +                case TestCaseType.FUNCTIONAL:
>> +                    func_test_cases.add(test_case_function)
>> +
>> +        if test_case_sublist_copy:
>> +            raise ConfigurationError(
>> +                f"Test cases {test_case_sublist_copy} not found among functions of {cls.__name__}."
>> +            )
>> +
>> +        return func_test_cases, perf_test_cases
>> +
> <snip>
>> 2.34.1
>>