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

The input subsystem doesn't currently have any unit tests, let's add a
CONFIG_INPUT_KUNIT_TEST option that builds a test suite to be executed
with the KUnit test infrastructure.

For now, only three tests were added for some of the input core helper
functions that are trivial to test:

  * input_test_polling: set/get poll interval and set-up a poll handler.

  * input_test_timestamp: set/get input event timestamps.

  * input_test_match_device_id: match a device by bus, vendor, product
                                and events that is capable of handling.

But having the minimal KUnit support allows to add more tests and suites
as follow-up changes. The tests can be run with the following command:

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

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

 drivers/input/Kconfig            |  12 +++
 drivers/input/Makefile           |   1 +
 drivers/input/tests/Makefile     |   3 +
 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)

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.

(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);
}

KUNIT_ASSERT_FAILURE() is like KUNIT_ASSERT_*(), but doesn't do any comparison.
It just unconditionally aborts the test after printing the given message.

We could alternatively do
  kunit_err(test, "failed to register device: %d", ret);
  return ret;
but this version won't show a file and line number.

+
+       test->priv = input_dev;
+
+       return 0;
+}
+
+static void input_test_exit(struct kunit *test)
+{
+       struct input_dev *input_dev = test->priv;
+
+       input_unregister_device(input_dev);
+}
+
+static void input_test_poll(struct input_dev *input) { }
+
+static void input_test_polling(struct kunit *test)
+{
+       struct input_dev *input_dev = test->priv;
+       int ret;
+
+       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)

+
+       ret = input_setup_polling(input_dev, input_test_poll);
+       KUNIT_ASSERT_EQ(test, ret, 0);
+
+       input_set_poll_interval(input_dev, POLL_INTERVAL);
+
+       ret = input_get_poll_interval(input_dev);
+       KUNIT_ASSERT_EQ(test, ret, POLL_INTERVAL);
+}
+

Thanks,
Daniel