Re: [PATCH v2 4/9] drm: selftest: convert drm_format selftest to KUnit

[Daniel Latypov]

On Tue, Jun 21, 2022 at 1:10 PM Maíra Canal <maira.canal@xxxxxx> wrote:
>
> Considering the current adoption of the KUnit framework, convert the
> DRM format selftest to the KUnit API.
>
> Tested-by: David Gow <davidgow@xxxxxxxxxx>
> Signed-off-by: Maíra Canal <maira.canal@xxxxxx>

Acked-by: Daniel Latypov <dlatypov@xxxxxxxxxx>

Overall looks good from the KUnit side, just a few general suggestions below.

FYI, the warning email from kernel-test-robot is basically saying that
the compiler is not optimizing away the temporary variables internally
created in KUNIT_EXPECT_*.
So having too many KUNIT_EXPECT_.* in a single function is the trigger.
The main workaround you'd have is to split up the test into more test functions.
(I don't know if that's actually worth doing)

> +static void igt_check_drm_format_block_width(struct kunit *test)
> +{
> +       const struct drm_format_info *info = NULL;
> +
> +       /* Test invalid arguments */
> +       KUNIT_EXPECT_FALSE(test, drm_format_info_block_width(info, 0));
> +       KUNIT_EXPECT_FALSE(test, drm_format_info_block_width(info, -1));
> +       KUNIT_EXPECT_FALSE(test, drm_format_info_block_width(info, 1));

Hmm, I think one of these two would be clearer here:
KUNIT_EXPECT_EQ(test, drm_format_info_block_width(info, 0), 0);
KUNIT_EXPECT_EQ(test, 0, drm_format_info_block_width(info, 0));

I think this helps test readability (giving hints about the types) and
gives better error messages, more on that below.

The problem with using the boolean expectations is that given
  int foo = 2;
  KUNIT_EXPECT_FALSE(test, foo);
KUnit will only print out
    Expected foo to be false, but is true

Using EXPECT_EQ(foo, 0), we'd get
    Expected foo == 0, but
        foo == 2

Knowing exactly what the offending return value was can help debug
test failures a bit faster.

> +
> +       /* Test 1 plane format */
> +       info = drm_format_info(DRM_FORMAT_XRGB4444);
> +       KUNIT_EXPECT_TRUE(test, info);

FYI, you can now instead write
  KUNIT_EXPECT_NOT_NULL(test, info);
this new macro was merged into 5.19-rc1.