Daniel Latypov <dlatypov@xxxxxxxxxx> writes:
Hello Daniel,
Thanks a lot for your feedback!
> On Wed, Mar 29, 2023 at 2:23 AM Javier Martinez Canillas
> <javierm@xxxxxxxxxx> 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
