Daniel Latypov [off-list ref] writes:

Hello Daniel,

Thanks a lot for your feedback!

On Wed, Mar 29, 2023 at 2:23 AM Javier Martinez Canillas
[off-list ref] wrote:

[...]

quoted

  $ ./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.

[...]

quoted

 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.

[...]

quoted

+         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.

[...]

quoted

+
+       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.

[...]

quoted

+
+       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