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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87cz4qfynm.fsf@minerva.mail-host-address-is-not-set>
Date:   Thu, 30 Mar 2023 10:09:49 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     linux-kernel@...r.kernel.org,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Maxime Ripard <maxime@...no.tech>,
        linux-kselftest@...r.kernel.org,
        Enric Balletbo i Serra <eballetb@...hat.com>,
        David Gow <davidgow@...gle.com>, kunit-dev@...glegroups.com,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-input@...r.kernel.org
Subject: Re: [PATCH] Input: Add KUnit tests for some of the input core
 helper functions

Daniel Latypov <dlatypov@...gle.com> writes:

Hello Daniel,

Thanks a lot for your feedback!

> On Wed, Mar 29, 2023 at 2:23 AM Javier Martinez Canillas
> <javierm@...hat.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