Re: [PATCH] Input: Add KUnit tests for some of the input core helper functions

[Javier Martinez Canillas <javierm@redhat.com>]

Daniel Latypov <dlatypov@google.com> writes:

Hello Daniel,

Thanks a lot for your feedback!

> On Wed, Mar 29, 2023 at 2:23 AM Javier Martinez Canillas
> <javierm@redhat.com> wrote:

[...]

>>
>>   $ ./tools/testing/kunit/kunit.py run \
>>     --kunitconfig=drivers/input/tests/.kunitconfig
>
> Nice!
> A few small suggestions below as someone who has worked on KUnit.
>
> FYI, to save a few keystrokes, you can omit the "/.kunitconfig" and
> just pass the dir, i.e.
>   --kunitconfig=drivers/input/tests
>

Ah, cool. I didn't know that.

[...]

>>  drivers/input/tests/input_test.c | 144 +++++++++++++++++++++++++++++++
>
> I don't see the .kunitconfig in the diff.
> Was it accidentally forgotten or does this patch apply to a tree that
> already has the file?
>
> (it's easy to forget since git will still ignore it by default, IIRC)
>

I did indeed forgot because as you mentioned git add complained and I
missed that needed to force to add it.

[...]

>> +         Say Y here if you want to build the KUnit tests for the input
>> +         subsystem. For more information about KUnit and unit tests in
>> +         general, please refer to the KUnit documentation in
>> +         Documentation/dev-tools/kunit/.
>> +
>> +         If in doubt, say "N".
>
> FYI, I know this is in the style guide, but I'd personally feel free
> to leave out this paragraph.
>
> Having such "advertising" about what KUnit is made more sense when
> less people knew about it.
> It's not known by everyone in the community yet, but we might be
> getting to a point where this turns into repetitive bloat.
>

Ok, I'll drop these.

[...]

>> +
>> +       ret = input_register_device(input_dev);
>> +       KUNIT_ASSERT_EQ(test, ret, 0);
>
> (very unlikely that this matters, but...)
> Hmm, should we call input_free_device() if this fails?
> i.e. something like
>
> ret = ...;
> if (ret) {
>   input_free_device(input_dev);
>   KUNIT_ASSERT_FAILURE(test, "failed to register device: %d", ret);
> }
>

Indeed. I'll do this too.

[...]

>> +
>> +       ret = input_get_poll_interval(input_dev);
>> +       KUNIT_ASSERT_EQ(test, ret, -EINVAL);
>
> minor suggestion: can we inline these? E.g.
>   KUNIT_ASSERT_EQ(test, -EINVAL, input_get_poll_interval(input_dev));
> This way on failure, KUnit can print the function call instead of just `ret`.
>
> Users could always find out what failed by the line #, but including
> it in the output would be a bit nicer.
>
> E.g. w/ KUNIT_EXPECT_EQ(test, 0, ...)
>
>     # example_simple_test: EXPECTATION FAILED at
> lib/kunit/kunit-example-test.c:29
>     Expected 0 == input_get_poll_interval(input_dev), but
>         input_get_poll_interval(input_dev) == 42 (0x2a)
>
> verus
>
>     # example_simple_test: EXPECTATION FAILED at
> lib/kunit/kunit-example-test.c:28
>     Expected ret == 0, but
>         ret == 42 (0x2a)
>

Great suggestion. I'll change too, it would also get rid of the ret variable.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat