Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220621200926.257002-1-maira.canal@usp.br> <20220621200926.257002-5-maira.canal@usp.br>
In-Reply-To: <20220621200926.257002-5-maira.canal@usp.br>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Wed, 22 Jun 2022 10:06:42 -0700
Message-ID: <CAGS_qxrynX=q7ZVmFSm-HO-3r3wGVnXMXVMejM3-ONvyJUrrPg@mail.gmail.com>
Subject: Re: [PATCH v2 4/9] drm: selftest: convert drm_format selftest to KUnit
To:     =?UTF-8?B?TWHDrXJhIENhbmFs?= <maira.canal@usp.br>
Cc:     Isabella Basso <isabbasso@riseup.net>, magalilemes00@gmail.com,
        tales.aparecida@gmail.com, mwen@igalia.com, andrealmeid@riseup.net,
        siqueirajordao@riseup.net, Trevor Woerner <twoerner@gmail.com>,
        leandro.ribeiro@collabora.com, n@nfraprado.net,
        Daniel Vetter <daniel@ffwll.ch>,
        Shuah Khan <skhan@linuxfoundation.org>,
        David Airlie <airlied@linux.ie>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        michal.winiarski@intel.com,
        Javier Martinez Canillas <javierm@redhat.com>,
        =?UTF-8?B?Sm9zw6kgRXhww7NzaXRv?= <jose.exposito89@gmail.com>,
        David Gow <davidgow@google.com>, brendanhiggins@google.com,
        kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Jun 21, 2022 at 1:10 PM Ma=C3=ADra Canal <maira.canal@usp.br> wrote=
:
>
> Considering the current adoption of the KUnit framework, convert the
> DRM format selftest to the KUnit API.
>
> Tested-by: David Gow <davidgow@google.com>
> Signed-off-by: Ma=C3=ADra Canal <maira.canal@usp.br>

Acked-by: Daniel Latypov <dlatypov@google.com>

Overall looks good from the KUnit side, just a few general suggestions belo=
w.

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 funct=
ions.
(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 =3D 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 =3D 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 =3D=3D 0, but
        foo =3D=3D 2

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

> +
> +       /* Test 1 plane format */
> +       info =3D 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.