On Thu, Oct 5, 2023 at 1:26 PM Mattias Rönnblom <hof...@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
