Re: [PATCH v5 2/3] test: add dispatcher test suite

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2023-10-06 10:52, David Marchand wrote:
> On Thu, Oct 5, 2023 at 1:26 PM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> 
> [snip]
> 
>>>> +#define RETURN_ON_ERROR(rc) \
>>>> +       do {                                    \
>>>> +               if (rc != TEST_SUCCESS)         \
>>>> +                       return rc;              \
>>>> +       } while (0)
>>>
>>> TEST_ASSERT?
>>> This gives context about which part of a test failed.
>>>
>>
>> This macro is used in a situation where the failure has occured and has
>> been reported already.
>>
>> Maybe it would be better to replace the macro instationation with just
>> the if+return statements.
>>
>> RETURN_ON_ERROR(rc);
>>
>> ->
>>
>> if (rc != TEST_SUCCESS)
>>          return rc;
> 
> Yes, this macro does not add much, you can remove it.
> 

OK, will do.

> [snip]
> 
> 
>>>> +       for (i = 0; i < NUM_SERVICE_CORES; i++)
>>>> +               if (app->service_lcores[i] == lcore_id)
>>>> +                       return i;
>>>
>>> This construct is hard to read and prone to error if the code is updated later.
>>>
>>> for () {
>>>     if ()
>>>       return i;
>>> }
>>>
>>>
>>
>> I wouldn't consider that an improvement (rather the opposite).
> 
> Well, I disagree, but it is not enforced in the coding style so I won't insist.
> 
> [snip]
> 
> 
>>>> +static struct unit_test_suite test_suite = {
>>>> +       .suite_name = "Event dispatcher test suite",
>>>> +       .unit_test_cases = {
>>>> +               TEST_CASE_ST(test_setup, test_teardown, test_basic),
>>>> +               TEST_CASE_ST(test_setup, test_teardown, test_drop),
>>>> +               TEST_CASE_ST(test_setup, test_teardown,
>>>> +                            test_many_handler_registrations),
>>>> +               TEST_CASE_ST(test_setup, test_teardown,
>>>> +                            test_many_finalize_registrations),
>>>> +               TEST_CASES_END()
>>>> +       }
>>>> +};
>>>> +
>>>> +static int
>>>> +test_dispatcher(void)
>>>> +{
>>>> +       return unit_test_suite_runner(&test_suite);
>>>> +}
>>>> +
>>>> +REGISTER_TEST_COMMAND(dispatcher_autotest, test_dispatcher);
>>>
>>> We have new macros (see REGISTER_FAST_TEST for example) so a test is
>>> associated to an existing testsuite.
>>> I think this test should be part of the fast-test testsuite, wdyt?
>>>
>>>
>>
>> It needs setup and teardown methods, so I assume a generic test suite
>> woulnd't do.
>>
>> The dispatcher tests do have fairly short run times, so in that sense
>> they should qualify.
> 
> 
> So please use REGISTER_FAST_TEST().
> Thanks.
> 
> 

OK.