| Message ID | 20230517153145.513095-1-dangel101@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] Input: tests: add test to cover all input_grab_device() function | expand |
Hi Dana, On Wed, May 17, 2023 at 06:31:45PM +0300, Dana Elfassy wrote: > Currently input_grab_device() isn't covered by any tests > Thus, adding a test to cover the cases: > 1. The device is grabbed successfully > 2. Trying to grab a device that is already grabbed by another input > handle > > Signed-off-by: Dana Elfassy <dangel101@gmail.com> > Tested-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > --- > > Changes in v2: > - Use input_put_device() to decrement the refcount increased by get(). > - Remove unnecessary struct input_handle test_handle variable. So this tests something different than what patch description states. You are testing that there is no "recursive" grabbing happening (an API could be designed to allow the same handle grab device several times). This is a good and useful test, but you do want to also use 2nd separate handle to see that it gets -EBUSY as well. And ideally we should have another test verifying that the 2nd handle can successfully grab the device once the first handle releases it. Thanks.
Dmitry Torokhov <dmitry.torokhov@gmail.com> writes: Hello Dmitry, > Hi Dana, > > On Wed, May 17, 2023 at 06:31:45PM +0300, Dana Elfassy wrote: >> Currently input_grab_device() isn't covered by any tests >> Thus, adding a test to cover the cases: >> 1. The device is grabbed successfully >> 2. Trying to grab a device that is already grabbed by another input >> handle >> >> Signed-off-by: Dana Elfassy <dangel101@gmail.com> >> Tested-by: Javier Martinez Canillas <javierm@redhat.com> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >> --- >> >> Changes in v2: >> - Use input_put_device() to decrement the refcount increased by get(). >> - Remove unnecessary struct input_handle test_handle variable. > > So this tests something different than what patch description states. > You are testing that there is no "recursive" grabbing happening (an API > could be designed to allow the same handle grab device several times). > This is a good and useful test, but you do want to also use 2nd separate > handle to see that it gets -EBUSY as well. And ideally we should have That was my fault since v1 had two different handles but since it wasn't releasing it, didn't add any value really so I asked Dana to just drop it. > another test verifying that the 2nd handle can successfully grab the > device once the first handle releases it. > That's the correct approach indeed and would make the test more useful.
Dana Elfassy <delfassy@redhat.com> writes: Hello Dana, > Currently input_grab_device() isn't covered by any tests > Thus, adding a test to cover the cases: > 1. The device is grabbed successfully > 2. Trying to grab a device that is already grabbed by another input > handle > > Signed-off-by: Dana Elfassy <dangel101@gmail.com> > Tested-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > --- [...] > + > + handle.dev = input_get_device(input_dev); > + handle.name = dev_name(&input_dev->dev); > + handle.handler = &handler; > + res = input_grab_device(&handle); Another thing I noticed is that your test will try to grab the same input_handle twice. So you need to remove the line above I believe. > + KUNIT_ASSERT_TRUE(test, input_grab_device(&handle)); > +
diff --git a/drivers/input/tests/input_test.c b/drivers/input/tests/input_test.c index 25bbf51b5c87..cea0167a74d2 100644 --- a/drivers/input/tests/input_test.c +++ b/drivers/input/tests/input_test.c @@ -124,10 +124,33 @@ static void input_test_match_device_id(struct kunit *test) KUNIT_ASSERT_FALSE(test, input_match_device_id(input_dev, &id)); } +static void input_test_grab(struct kunit *test) +{ + struct input_dev *input_dev = test->priv; + struct input_handler handler; + struct input_handle handle; + struct input_device_id id; + int res; + + handler.name = "handler"; + handler.id_table = &id; + + handle.dev = input_get_device(input_dev); + handle.name = dev_name(&input_dev->dev); + handle.handler = &handler; + res = input_grab_device(&handle); + KUNIT_ASSERT_TRUE(test, input_grab_device(&handle)); + + res = input_grab_device(&handle); + KUNIT_ASSERT_EQ(test, res, -EBUSY); + input_put_device(input_dev); +} + static struct kunit_case input_tests[] = { KUNIT_CASE(input_test_polling), KUNIT_CASE(input_test_timestamp), KUNIT_CASE(input_test_match_device_id), + KUNIT_CASE(input_test_grab), { /* sentinel */ } };