Re: [PATCH v2 1/2] string: Convert selftest to KUnit

[Andy Shevchenko]

On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> Convert test_string.c to KUnit so it can be easily run with everything
> else.

Have you run it?

..

>         if (i < 256)
> -               return (i << 24) | (j << 16) | k | 0x8000;
> -       return 0;
> +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);

First of all, this special value encodes the problematic patterns, so
you missed proper messaging.
Second, the returned value has a constant, how do you expect 0 to be
equal to something (guaranteed not to be 0)?

This needs a good rethink of what you should do in the KUnit approach.

..

> +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);

Ditto.

..

> +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);

Ditto.

..

>         for (i = 0; i < strlen(test_string) + 1; i++) {
>                 result = strchr(test_string, test_string[i]);
> -               if (result - test_string != i)
> -                       return i + 'a';
> +               KUNIT_ASSERT_EQ(test, result - test_string, i);

In a similar way, all returned values are *special*, you really need
to think about them before converting to a simple (and sometimes
wrong) checks)

..

I dunno if KUnit has a fault ejection simulation. It should, in order
to be sure that test cases are fine when they fail.

-- 
With Best Regards,
Andy Shevchenko