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

[David Marchand <david.marchand@redhat.com>]

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.

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


-- 
David Marchand