diff mbox series

[1/7] HID: uclogic: KUnit best practices and naming conventions

Message ID 20220710175043.192901-2-jose.exposito89@gmail.com (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series XP-PEN Deco Pro S support (for-5.20/uclogic) | expand

Commit Message

José Expósito July 10, 2022, 5:50 p.m. UTC 
The KUnit documentation [1] suggests allowing build tests as a module.

In addition, it is recommended [2] to use snake case names for
kunit_suite and test cases.

Change the Kconfig entry from bool to tristate and stick to the naming
conventions to avoid style issues with future tests.

Link: https://docs.kernel.org/dev-tools/kunit/style.html#test-kconfig-entries  [1]
Link: https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html  [2]
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/hid/Kconfig                  |  2 +-
 drivers/hid/Makefile                 |  3 ++-
 drivers/hid/hid-uclogic-rdesc-test.c | 22 +++++++++++-----------
 3 files changed, 14 insertions(+), 13 deletions(-)

Comments

Daniel Latypov July 11, 2022, 2:41 p.m. UTC | #1 
On Sun, Jul 10, 2022 at 10:51 AM José Expósito
<jose.exposito89@gmail.com> wrote:
>
> The KUnit documentation [1] suggests allowing build tests as a module.
>
> In addition, it is recommended [2] to use snake case names for
> kunit_suite and test cases.

Test parameters don't fall under "test cases", though I see how that
can be construed as such.
I don't think anyone has stated any preference to standardize the naming there.

We currently have parameterized tests using spaces and punctuation, e.g.
ok 7 - binfmt_elf
    # Subtest: ext4_inode_test
    1..1
        # Subtest: inode_test_xtimestamp_decoding
        ok 1 - 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits
        ok 2 - 1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits
...
    ok 1 - mctp_test_fragment
        # Subtest: mctp_test_rx_input
        ok 1 - {1,a,8,0}
        ok 2 - {1,a,9,0}
        ok 3 - {2,a,8,0}

So I think the old names were more conventional.

>
> Change the Kconfig entry from bool to tristate and stick to the naming
> conventions to avoid style issues with future tests.
>
> Link: https://docs.kernel.org/dev-tools/kunit/style.html#test-kconfig-entries  [1]
> Link: https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html  [2]
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>

Everything else (renaming the suite and switching to tristate) sounds
good to me though.

Acked-by: Daniel Latypov <dlatypov@google.com>
José Expósito July 11, 2022, 4:16 p.m. UTC | #2 
Hi Daniel,

On Mon, Jul 11, 2022 at 07:41:53AM -0700, Daniel Latypov wrote:
> On Sun, Jul 10, 2022 at 10:51 AM José Expósito
> <jose.exposito89@gmail.com> wrote:
> >
> > The KUnit documentation [1] suggests allowing build tests as a module.
> >
> > In addition, it is recommended [2] to use snake case names for
> > kunit_suite and test cases.
> 
> Test parameters don't fall under "test cases", though I see how that
> can be construed as such.
> I don't think anyone has stated any preference to standardize the naming there.
> 
> We currently have parameterized tests using spaces and punctuation, e.g.
> ok 7 - binfmt_elf
>     # Subtest: ext4_inode_test
>     1..1
>         # Subtest: inode_test_xtimestamp_decoding
>         ok 1 - 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits
>         ok 2 - 1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits
> ...
>     ok 1 - mctp_test_fragment
>         # Subtest: mctp_test_rx_input
>         ok 1 - {1,a,8,0}
>         ok 2 - {1,a,9,0}
>         ok 3 - {2,a,8,0}
> 
> So I think the old names were more conventional.

I changed the names to be consistent with other tests I'm working on
present in "gpu/drm/tests/drm_format_helper_test.c".

My first version there used full sentences for the test cases, but, if
I remember correctly, it was suggested to use snake case.

I don't have a strong preference about using one approach or the other.
If there is not a rule, I'd prefer to be consistent with the work I'm
doing in the DRM subsystem to avoid mixing notation or refactoring
there, but I'm open to change it.

> > Change the Kconfig entry from bool to tristate and stick to the naming
> > conventions to avoid style issues with future tests.
> >
> > Link: https://docs.kernel.org/dev-tools/kunit/style.html#test-kconfig-entries  [1]
> > Link: https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html  [2]
> > Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> 
> Everything else (renaming the suite and switching to tristate) sounds
> good to me though.
> 
> Acked-by: Daniel Latypov <dlatypov@google.com>

Thanks a lot for your review! I'll wait a couple of days before
sending v2 with the Acked-by tag just in case you or somebody else
wants to add more comments.

Best wishes,
Jose
diff mbox series

Patch

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 6ce92830b5d1..36a17958493f 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1307,7 +1307,7 @@  config HID_MCP2221
 	will be called hid-mcp2221.ko.
 
 config HID_KUNIT_TEST
-	bool "KUnit tests for HID" if !KUNIT_ALL_TESTS
+	tristate "KUnit tests for HID" if !KUNIT_ALL_TESTS
 	depends on KUNIT=y
 	depends on HID_UCLOGIC
 	default KUNIT_ALL_TESTS
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index b0bef8098139..82d8fd97d96c 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -144,8 +144,9 @@  obj-$(CONFIG_HID_WIIMOTE)	+= hid-wiimote.o
 obj-$(CONFIG_HID_SENSOR_HUB)	+= hid-sensor-hub.o
 obj-$(CONFIG_HID_SENSOR_CUSTOM_SENSOR)	+= hid-sensor-custom.o
 
-obj-$(CONFIG_HID_KUNIT_TEST)	+= hid-uclogic-rdesc.o \
+hid-uclogic-test-objs		:= hid-uclogic-rdesc.o \
 				   hid-uclogic-rdesc-test.o
+obj-$(CONFIG_HID_KUNIT_TEST)	+= hid-uclogic-test.o
 
 obj-$(CONFIG_USB_HID)		+= usbhid/
 obj-$(CONFIG_USB_MOUSE)		+= usbhid/
diff --git a/drivers/hid/hid-uclogic-rdesc-test.c b/drivers/hid/hid-uclogic-rdesc-test.c
index ebebffef5f8a..3971a0854c3e 100644
--- a/drivers/hid/hid-uclogic-rdesc-test.c
+++ b/drivers/hid/hid-uclogic-rdesc-test.c
@@ -97,7 +97,7 @@  static const __u8 template_params_none[] = {
 
 static struct uclogic_template_case uclogic_template_cases[] = {
 	{
-		.name = "Empty template",
+		.name = "empty_template",
 		.template = template_empty,
 		.template_size = sizeof(template_empty),
 		.param_list = params_pen_all,
@@ -105,7 +105,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = template_empty,
 	},
 	{
-		.name = "Template smaller than the placeholder",
+		.name = "template_smaller_than_the_placeholder",
 		.template = template_small,
 		.template_size = sizeof(template_small),
 		.param_list = params_pen_all,
@@ -113,7 +113,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = template_small,
 	},
 	{
-		.name = "No placeholder",
+		.name = "no_placeholder",
 		.template = template_no_ph,
 		.template_size = sizeof(template_no_ph),
 		.param_list = params_pen_all,
@@ -121,7 +121,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = template_no_ph,
 	},
 	{
-		.name = "Pen placeholder at the end, without ID",
+		.name = "pen_placeholder_at_the_end_without_id",
 		.template = template_pen_ph_end,
 		.template_size = sizeof(template_pen_ph_end),
 		.param_list = params_pen_all,
@@ -129,7 +129,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = template_pen_ph_end,
 	},
 	{
-		.name = "Frame button placeholder at the end, without ID",
+		.name = "frame_button_placeholder_at_the_end_without_id",
 		.template = template_btn_ph_end,
 		.template_size = sizeof(template_btn_ph_end),
 		.param_list = params_frame_all,
@@ -137,7 +137,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = template_btn_ph_end,
 	},
 	{
-		.name = "All params present in the pen template",
+		.name = "all_params_present_in_the_pen_template",
 		.template = template_pen_all_params,
 		.template_size = sizeof(template_pen_all_params),
 		.param_list = params_pen_all,
@@ -145,7 +145,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = expected_pen_all_params,
 	},
 	{
-		.name = "All params present in the frame template",
+		.name = "all_params_present_in_the_frame_template",
 		.template = template_frame_all_params,
 		.template_size = sizeof(template_frame_all_params),
 		.param_list = params_frame_all,
@@ -153,7 +153,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = expected_frame_all_params,
 	},
 	{
-		.name = "Some params present in the pen template (complete param list)",
+		.name = "some_params_present_in_the_pen_template_with_complete_param_list",
 		.template = template_pen_some_params,
 		.template_size = sizeof(template_pen_some_params),
 		.param_list = params_pen_all,
@@ -161,7 +161,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = expected_pen_some_params,
 	},
 	{
-		.name = "Some params present in the pen template (incomplete param list)",
+		.name = "some_params_present_in_the_pen_template_with_incomplete_param_list",
 		.template = template_pen_some_params,
 		.template_size = sizeof(template_pen_some_params),
 		.param_list = params_pen_some,
@@ -169,7 +169,7 @@  static struct uclogic_template_case uclogic_template_cases[] = {
 		.expected = expected_pen_some_params,
 	},
 	{
-		.name = "No params present in the template",
+		.name = "no_params_present_in_the_template",
 		.template = template_params_none,
 		.template_size = sizeof(template_params_none),
 		.param_list = params_pen_some,
@@ -208,7 +208,7 @@  static struct kunit_case hid_uclogic_rdesc_test_cases[] = {
 };
 
 static struct kunit_suite hid_uclogic_rdesc_test_suite = {
-	.name = "hid-uclogic-rdesc-test",
+	.name = "hid_uclogic_rdesc_test",
 	.test_cases = hid_uclogic_rdesc_test_cases,
 };