Re: [PATCH v3 4/4] rv/reactors: add KUnit tests for reactor registration and dispatch

[Nam Cao <namcao@linutronix.de>]

wen.yang@linux.dev writes:
> +/*
> + * Use a fixed-size array so sizeof() gives the exact byte count at
> + * compile time.
> + */

There is no need for a comment. This is obvious from the code itself.

> +static const char long_reactor_name[] = "kunit_reactor_name_too_long_xxx_";
> +_Static_assert(sizeof(long_reactor_name) - 1 >= MAX_RV_REACTOR_NAME_SIZE,
> +	       "long_reactor_name must be at least MAX_RV_REACTOR_NAME_SIZE chars");
> +
> +static void test_name_too_long(struct kunit *test)
> +{
> +	static struct rv_reactor long_reactor = {
> +		.name = long_reactor_name,
> +	};
> +
> +	KUNIT_EXPECT_EQ(test, rv_register_reactor(&long_reactor), -EINVAL);
> +}
> +
> +static struct kunit_case rv_reactor_registration_cases[] = {
> +	KUNIT_CASE(test_double_register),
> +	KUNIT_CASE(test_name_too_long),
> +	{}
> +};

I am not sure about the usefulness of these test cases.
rv_register_reactor() is not an user API that we have to prepare
for corner case usage. We can expect its users to be sane since the
users are us.

But well, if Gabriele wants to keep them..

> +
> +static struct kunit_suite rv_reactor_registration_suite = {
> +	.name		= "rv_reactor_registration",
> +	.test_cases	= rv_reactor_registration_cases,
> +};
> +
> +static atomic_t react_call_count;

Do we really need atomic_t? Does int work?

> +
> +__printf(1, 0) static void mock_react(const char *msg, va_list args)
> +{
> +	atomic_inc(&react_call_count);
> +	/*
> +	 * Hold the CPU for 5 ms so a timer interrupt is likely to fire
> +	 * inside rv_react()'s lockdep context, exercising the LD_WAIT_SPIN
> +	 * constraint.  mdelay() is a calibrated busy-wait with no scheduler
> +	 * interaction.
> +	 */

The comment above mdelay()'s definition already explains what it does.

> +	mdelay(5);
> +}
> +
> +static void test_react_no_callback(struct kunit *test)
> +{
> +	struct rv_monitor monitor = {
> +		.name = "kunit_null_react",
> +	};
> +
> +	atomic_set(&react_call_count, 0);
> +	rv_react(&monitor, "no callback");
> +
> +	/*
> +	 * The only possible failure in this test case is a kernel panic.
> +	 * NULL react guard: callback must NOT have been invoked
> +	 */

Obvious comment.

> +	KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 0);
> +}
> +
> +static void test_react_callback_invoked(struct kunit *test)
> +{
> +	struct rv_monitor monitor = {
> +		.name	= "kunit_dispatch_monitor",
> +		.react	= mock_react,
> +	};
> +
> +	atomic_set(&react_call_count, 0);
> +	rv_react(&monitor, "callback invocation test");
> +	KUNIT_EXPECT_EQ(test, atomic_read(&react_call_count), 1);
> +}

So the test calls rv_react(), and validates that the reactor is called?
Honestly I am not sure how useful that is. Especially since Gabriele
already made the selftest which validates that the reactor is invoked.

Nam