[PATCH] lib: Convert UUID runtime test to KUnit

[André Almeida]

Remove custom functions for testing and use KUnit framework. Test cases
and test data remains the same.

Signed-off-by: André Almeida <andre...@collabora.com>
---
lib/Kconfig.debug | 13 +++--
lib/Makefile | 2 +-
lib/test_uuid.c | 131 ++++++++++++++++++----------------------------
3 files changed, 62 insertions(+), 84 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 678c13967580..e8bd574d7a67 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2188,9 +2188,6 @@ config TEST_BITMAP

If unsure, say N.

-config TEST_UUID
- tristate "Test functions located in the uuid module at runtime"
-
config TEST_XARRAY
tristate "Test the XArray code at runtime"

@@ -2429,6 +2426,16 @@ config BITS_TEST

If unsure, say N.

+config TEST_UUID
+ tristate "Unit test for UUID" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds the UUID unit test.
+ Tests parsing functions for UUID/GUID strings.
+
+ If unsure, say N.
+
config TEST_UDELAY
tristate "udelay test driver"
help
diff --git a/lib/Makefile b/lib/Makefile
index 2cc359ec1fdd..6ef3c614409d 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -85,7 +85,6 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
obj-$(CONFIG_TEST_PRINTF) += test_printf.o
obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
obj-$(CONFIG_TEST_STRSCPY) += test_strscpy.o
-obj-$(CONFIG_TEST_UUID) += test_uuid.o
obj-$(CONFIG_TEST_XARRAY) += test_xarray.o
obj-$(CONFIG_TEST_PARMAN) += test_parman.o
obj-$(CONFIG_TEST_KMOD) += test_kmod.o
@@ -354,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
obj-$(CONFIG_BITS_TEST) += test_bits.o
obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
+obj-$(CONFIG_TEST_UUID) += test_uuid.o

obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
diff --git a/lib/test_uuid.c b/lib/test_uuid.c
index cd819c397dc7..45c919b0d724 100644
--- a/lib/test_uuid.c
+++ b/lib/test_uuid.c
@@ -1,21 +1,20 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
/*
- * Test cases for lib/uuid.c module.
+ * Unit tests for lib/uuid.c module.
+ *
+ * Copyright 2016 Andy Shevchenko <andriy.s...@linux.intel.com>
+ * Copyright 2021 André Almeida <andre...@riseup.net>
*/
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/string.h>
+#include <kunit/test.h>
#include <linux/uuid.h>

-struct test_uuid_data {
+struct test_data {
const char *uuid;
guid_t le;
uuid_t be;
};

-static const struct test_uuid_data test_uuid_test_data[] = {
+static const struct test_data correct_data[] = {
{
.uuid = "c33f4995-3701-450e-9fbf-206a2e98e576",
.le = GUID_INIT(0xc33f4995, 0x3701, 0x450e, 0x9f, 0xbf, 0x20, 0x6a, 0x2e, 0x98, 0xe5, 0x76),
@@ -33,101 +32,73 @@ static const struct test_uuid_data test_uuid_test_data[] = {
},
};

-static const char * const test_uuid_wrong_data[] = {
+static const char * const wrong_data[] = {
"c33f4995-3701-450e-9fbf206a2e98e576 ", /* no hyphen(s) */
"64b4371c-77c1-48f9-8221-29f054XX023b", /* invalid character(s) */
"0cb4ddff-a545-4401-9d06-688af53e", /* not enough data */
};

-static unsigned total_tests __initdata;
-static unsigned failed_tests __initdata;
-
-static void __init test_uuid_failed(const char *prefix, bool wrong, bool be,
- const char *data, const char *actual)
+static void uuid_correct_le(struct kunit *test)
{
- pr_err("%s test #%u %s %s data: '%s'\n",
- prefix,
- total_tests,
- wrong ? "passed on wrong" : "failed on",
- be ? "BE" : "LE",
- data);
- if (actual && *actual)
- pr_err("%s test #%u actual data: '%s'\n",
- prefix,
- total_tests,
- actual);
- failed_tests++;
+ guid_t le;
+ const struct test_data *data = (const struct test_data *)(test->param_value);
+
+ KUNIT_ASSERT_EQ(test, guid_parse(data->uuid, &le), 0);
+ KUNIT_EXPECT_TRUE(test, guid_equal(&data->le, &le));
}

-static void __init test_uuid_test(const struct test_uuid_data *data)
+static void uuid_correct_be(struct kunit *test)
{
- guid_t le;
uuid_t be;
- char buf[48];
-
- /* LE */
- total_tests++;
- if (guid_parse(data->uuid, &le))
- test_uuid_failed("conversion", false, false, data->uuid, NULL);
-
- total_tests++;
- if (!guid_equal(&data->le, &le)) {
- sprintf(buf, "%pUl", &le);
- test_uuid_failed("cmp", false, false, data->uuid, buf);
- }
-
- /* BE */
- total_tests++;
- if (uuid_parse(data->uuid, &be))
- test_uuid_failed("conversion", false, true, data->uuid, NULL);
-
- total_tests++;
- if (!uuid_equal(&data->be, &be)) {
- sprintf(buf, "%pUb", &be);
- test_uuid_failed("cmp", false, true, data->uuid, buf);
- }
+ const struct test_data *data = (const struct test_data *)(test->param_value);
+
+ KUNIT_ASSERT_EQ(test, uuid_parse(data->uuid, &be), 0);
+ KUNIT_EXPECT_TRUE(test, uuid_equal(&data->be, &be));
}

-static void __init test_uuid_wrong(const char *data)
+static void uuid_wrong_le(struct kunit *test)
{
guid_t le;
- uuid_t be;
-
- /* LE */
- total_tests++;
- if (!guid_parse(data, &le))
- test_uuid_failed("negative", true, false, data, NULL);
+ const char *data = (const char *)(test->param_value);

- /* BE */
- total_tests++;
- if (!uuid_parse(data, &be))
- test_uuid_failed("negative", true, true, data, NULL);
+ KUNIT_ASSERT_NE(test, guid_parse(data, &le), 0);
}

-static int __init test_uuid_init(void)
+static void uuid_wrong_be(struct kunit *test)
{
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(test_uuid_test_data); i++)
- test_uuid_test(&test_uuid_test_data[i]);
-
- for (i = 0; i < ARRAY_SIZE(test_uuid_wrong_data); i++)
- test_uuid_wrong(test_uuid_wrong_data[i]);
+ uuid_t be;
+ const char *data = (const char *)(test->param_value);

- if (failed_tests == 0)
- pr_info("all %u tests passed\n", total_tests);
- else
- pr_err("failed %u out of %u tests\n", failed_tests, total_tests);
+ KUNIT_ASSERT_NE(test, uuid_parse(data, &be), 0);
+}

- return failed_tests ? -EINVAL : 0;
+static void case_to_desc_correct(const struct test_data *t, char *desc)
+{
+ strcpy(desc, t->uuid);
}
-module_init(test_uuid_init);

-static void __exit test_uuid_exit(void)
+KUNIT_ARRAY_PARAM(correct, correct_data, case_to_desc_correct);
+
+static void case_to_desc_wrong(const char * const *s, char *desc)
{
- /* do nothing */
+ strcpy(desc, *s);
}
-module_exit(test_uuid_exit);
+
+KUNIT_ARRAY_PARAM(wrong, wrong_data, case_to_desc_wrong);
+
+static struct kunit_case uuid_test_cases[] = {
+ KUNIT_CASE_PARAM(uuid_correct_be, correct_gen_params),
+ KUNIT_CASE_PARAM(uuid_correct_le, correct_gen_params),
+ KUNIT_CASE_PARAM(uuid_wrong_be, wrong_gen_params),
+ KUNIT_CASE_PARAM(uuid_wrong_le, wrong_gen_params),
+ {}
+};
+
+static struct kunit_suite uuid_test_suite = {
+ .name = "uuid-test",
+ .test_cases = uuid_test_cases,
+};
+kunit_test_suite(uuid_test_suite);

MODULE_AUTHOR("Andy Shevchenko <andriy.s...@linux.intel.com>");
MODULE_LICENSE("Dual BSD/GPL");
--
2.31.1

[David Gow]

On Sun, Jun 6, 2021 at 5:52 AM André Almeida <andre...@collabora.com> wrote:
>
> Remove custom functions for testing and use KUnit framework. Test cases
> and test data remains the same.
>
> Signed-off-by: André Almeida <andre...@collabora.com>
> ---

Thanks! It's always exciting to see more tests using KUnit.

Note that the names here (filename, suite name, and Kconfig entry
name) don't match what we usually recommend for KUnit tests:
https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html

Given that this is an existing test, it is definitely okay to keep the
old names if you think it'd break something, but if there's no issue
it may be worth renaming them. The test suite name (which is new
anyway) ideally shouldn't end in "-test": just "uuid" is best.

I know there are quite a few existing tests which don't adhere to
these perfectly yet, but ideally new ones will if it's convenient.

Otherwise, this looks great. I've run it here, and it worked well and
picked up on any deliberate errors I introduced.

So this is
Tested-by: David Gow <davi...@google.com>

Cheers,
-- David

[André Almeida]

Às 21:54 de 05/06/21, David Gow escreveu:

Thank you for the feedback :) I'll submit a v2 applying your suggestions.

>
> Cheers,
> -- David
>

[Andy Shevchenko]

On Sun, Jun 6, 2021 at 12:53 AM André Almeida <andre...@collabora.com> wrote:
>
> Remove custom functions for testing and use KUnit framework. Test cases
> and test data remains the same.

Can you provide the output in OK and non-OK runs before and after your patch?

--
With Best Regards,
Andy Shevchenko

[Daniel Latypov]

Random question: this moves the config option down.
Is the intent to keep all the KUnit-based tests together?

I personally think it would be fine to leave it where it was, makes
`git blame` a bit more useful.

You could make use of the _MSG variants, fyi.
See https://www.kernel.org/doc/html/latest/dev-tools/kunit/tips.html#customizing-error-messages

I'd actually written up a version of this patch but never ended up
sending it out.
(I prefer how you've more properly split up the test cases and used
parameterized testing.)

Here's how I'd converted the test case using those _MSG variants:
/* LE */
KUNIT_EXPECT_FALSE_MSG(test, guid_parse(data->uuid, &le),
"LE: failed to parse '%s'", data->uuid);

KUNIT_EXPECT_TRUE_MSG(test, guid_equal(&data->le, &le),
"LE: '%s' should be equal to %pUl", data->uuid, &le);

/* BE */
KUNIT_EXPECT_FALSE_MSG(test, uuid_parse(data->uuid, &be),
"BE: failed to parse '%s'", data->uuid);

KUNIT_EXPECT_TRUE_MSG(test, uuid_equal(&data->be, &be),
"BE: '%s' should be equal to %pUl", data->uuid, &be);

Example failure output:
# test_uuid: EXPECTATION FAILED at lib/test_uuid.c:77
Expected uuid_equal(&data->be, &be) to be true, but is false

BE: 'c33f4995-3701-450e-9fbf-206a2e98e576' should be equal to
95493fc3-0137-0e45-9fbf-206a2e98e576
not ok 1 - test_uuid

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20210605215215.171165-1-andrealmeid%40collabora.com.

[André Almeida]

Às 22:02 de 07/06/21, Daniel Latypov escreveu:

Yes.

> I personally think it would be fine to leave it where it was, makes
> `git blame` a bit more useful.
>

No problem for me, I'll change that for v2.

Awesome, I really wasn't aware of those _MSG variants, I'll use them for
v2, thanks!

[André Almeida]

Hi Andy,

Às 14:32 de 06/06/21, Andy Shevchenko escreveu:

> On Sun, Jun 6, 2021 at 12:53 AM André Almeida <andre...@collabora.com> wrote:
>>
>> Remove custom functions for testing and use KUnit framework. Test cases
>> and test data remains the same.
>
> Can you provide the output in OK and non-OK runs before and after your patch?
>

I'll add the output in my v2 cover letter.

[André Almeida]

Hi,

This patch converts existing UUID runtime test to use KUnit framework.

Below, there's a comparison between the old output format and the new
one. Keep in mind that even if KUnit seems very verbose, this is the
corner case where _every_ test has failed.

* This is how the current output looks like in success:

test_uuid: all 18 tests passed

* And when it fails:

test_uuid: conversion test #1 failed on LE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #2 failed on LE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #2 actual data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: conversion test #3 failed on BE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #4 failed on BE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #4 actual data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: conversion test #5 failed on LE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #6 failed on LE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #6 actual data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: conversion test #7 failed on BE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #8 failed on BE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #8 actual data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: conversion test #9 failed on LE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #10 failed on LE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #10 actual data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: conversion test #11 failed on BE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #12 failed on BE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #12 actual data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: negative test #13 passed on wrong LE data: 'c33f4995-3701-450e-9fbf206a2e98e576 '
test_uuid: negative test #14 passed on wrong BE data: 'c33f4995-3701-450e-9fbf206a2e98e576 '
test_uuid: negative test #15 passed on wrong LE data: '64b4371c-77c1-48f9-8221-29f054XX023b'
test_uuid: negative test #16 passed on wrong BE data: '64b4371c-77c1-48f9-8221-29f054XX023b'
test_uuid: negative test #17 passed on wrong LE data: '0cb4ddff-a545-4401-9d06-688af53e'
test_uuid: negative test #18 passed on wrong BE data: '0cb4ddff-a545-4401-9d06-688af53e'
test_uuid: failed 18 out of 18 tests


* Now, here's how it looks like with KUnit:

======== [PASSED] uuid ========
[PASSED] uuid_correct_be
[PASSED] uuid_correct_le
[PASSED] uuid_wrong_be
[PASSED] uuid_wrong_le

* And if every test fail with KUnit:

======== [FAILED] uuid ========
[FAILED] uuid_correct_be
# uuid_correct_be: ASSERTION FAILED at lib/test_uuid.c:57
Expected uuid_parse(data->uuid, &be) == 1, but
uuid_parse(data->uuid, &be) == 0

failed to parse 'c33f4995-3701-450e-9fbf-206a2e98e576'
# uuid_correct_be: not ok 1 - c33f4995-3701-450e-9fbf-206a2e98e576
# uuid_correct_be: ASSERTION FAILED at lib/test_uuid.c:57
Expected uuid_parse(data->uuid, &be) == 1, but
uuid_parse(data->uuid, &be) == 0

failed to parse '64b4371c-77c1-48f9-8221-29f054fc023b'
# uuid_correct_be: not ok 2 - 64b4371c-77c1-48f9-8221-29f054fc023b
# uuid_correct_be: ASSERTION FAILED at lib/test_uuid.c:57
Expected uuid_parse(data->uuid, &be) == 1, but
uuid_parse(data->uuid, &be) == 0

failed to parse '0cb4ddff-a545-4401-9d06-688af53e7f84'
# uuid_correct_be: not ok 3 - 0cb4ddff-a545-4401-9d06-688af53e7f84
not ok 1 - uuid_correct_be

[FAILED] uuid_correct_le
# uuid_correct_le: ASSERTION FAILED at lib/test_uuid.c:46
Expected guid_parse(data->uuid, &le) == 1, but
guid_parse(data->uuid, &le) == 0

failed to parse 'c33f4995-3701-450e-9fbf-206a2e98e576'
# uuid_correct_le: not ok 1 - c33f4995-3701-450e-9fbf-206a2e98e576
# uuid_correct_le: ASSERTION FAILED at lib/test_uuid.c:46
Expected guid_parse(data->uuid, &le) == 1, but
guid_parse(data->uuid, &le) == 0

failed to parse '64b4371c-77c1-48f9-8221-29f054fc023b'
# uuid_correct_le: not ok 2 - 64b4371c-77c1-48f9-8221-29f054fc023b
# uuid_correct_le: ASSERTION FAILED at lib/test_uuid.c:46
Expected guid_parse(data->uuid, &le) == 1, but
guid_parse(data->uuid, &le) == 0

failed to parse '0cb4ddff-a545-4401-9d06-688af53e7f84'
# uuid_correct_le: not ok 3 - 0cb4ddff-a545-4401-9d06-688af53e7f84
not ok 2 - uuid_correct_le

[FAILED] uuid_wrong_be
# uuid_wrong_be: ASSERTION FAILED at lib/test_uuid.c:77
Expected uuid_parse(*data, &be) == 0, but
uuid_parse(*data, &be) == -22

parsing of 'c33f4995-3701-450e-9fbf206a2e98e576 ' should've failed
# uuid_wrong_be: not ok 1 - c33f4995-3701-450e-9fbf206a2e98e576
# uuid_wrong_be: ASSERTION FAILED at lib/test_uuid.c:77
Expected uuid_parse(*data, &be) == 0, but
uuid_parse(*data, &be) == -22

parsing of '64b4371c-77c1-48f9-8221-29f054XX023b' should've failed
# uuid_wrong_be: not ok 2 - 64b4371c-77c1-48f9-8221-29f054XX023b
# uuid_wrong_be: ASSERTION FAILED at lib/test_uuid.c:77
Expected uuid_parse(*data, &be) == 0, but
uuid_parse(*data, &be) == -22

parsing of '0cb4ddff-a545-4401-9d06-688af53e' should've failed
# uuid_wrong_be: not ok 3 - 0cb4ddff-a545-4401-9d06-688af53e
not ok 3 - uuid_wrong_be

[FAILED] uuid_wrong_le
# uuid_wrong_le: ASSERTION FAILED at lib/test_uuid.c:68
Expected guid_parse(*data, &le) == 0, but
guid_parse(*data, &le) == -22

parsing of 'c33f4995-3701-450e-9fbf206a2e98e576 ' should've failed
# uuid_wrong_le: not ok 1 - c33f4995-3701-450e-9fbf206a2e98e576
# uuid_wrong_le: ASSERTION FAILED at lib/test_uuid.c:68
Expected guid_parse(*data, &le) == 0, but
guid_parse(*data, &le) == -22

parsing of '64b4371c-77c1-48f9-8221-29f054XX023b' should've failed
# uuid_wrong_le: not ok 2 - 64b4371c-77c1-48f9-8221-29f054XX023b
# uuid_wrong_le: ASSERTION FAILED at lib/test_uuid.c:68
Expected guid_parse(*data, &le) == 0, but
guid_parse(*data, &le) == -22

parsing of '0cb4ddff-a545-4401-9d06-688af53e' should've failed
# uuid_wrong_le: not ok 3 - 0cb4ddff-a545-4401-9d06-688af53e
not ok 4 - uuid_wrong_le

Changes from v1:
- Test suite name: uuid_test -> uuid
- Config name: TEST_UUID -> UUID_KUNIT_TEST
- Config entry in the Kconfig file left where it is
- Converted tests to use _MSG variant

André Almeida (1):
lib: Convert UUID runtime test to KUnit

lib/Kconfig.debug | 11 +++-
lib/Makefile | 2 +-
lib/test_uuid.c | 137 +++++++++++++++++++---------------------------
3 files changed, 67 insertions(+), 83 deletions(-)

--
2.31.1

[André Almeida]

Remove custom functions for testing and use KUnit framework. Test cases
and test data remains the same.

Signed-off-by: André Almeida <andre...@collabora.com>
---

lib/Kconfig.debug | 11 +++-
lib/Makefile | 2 +-
lib/test_uuid.c | 137 +++++++++++++++++++---------------------------
3 files changed, 67 insertions(+), 83 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 678c13967580..1d879197f303 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2188,8 +2188,15 @@ config TEST_BITMAP

If unsure, say N.

-config TEST_UUID
- tristate "Test functions located in the uuid module at runtime"

+config UUID_KUNIT_TEST

+ tristate "Unit test for UUID" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help
+ This builds the UUID unit test.
+ Tests parsing functions for UUID/GUID strings.
+
+ If unsure, say N.

config TEST_XARRAY
tristate "Test the XArray code at runtime"

diff --git a/lib/Makefile b/lib/Makefile
index 2cc359ec1fdd..cc19048961c0 100644

--- a/lib/Makefile
+++ b/lib/Makefile
@@ -85,7 +85,6 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
obj-$(CONFIG_TEST_PRINTF) += test_printf.o
obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
obj-$(CONFIG_TEST_STRSCPY) += test_strscpy.o
-obj-$(CONFIG_TEST_UUID) += test_uuid.o
obj-$(CONFIG_TEST_XARRAY) += test_xarray.o
obj-$(CONFIG_TEST_PARMAN) += test_parman.o
obj-$(CONFIG_TEST_KMOD) += test_kmod.o
@@ -354,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
obj-$(CONFIG_BITS_TEST) += test_bits.o
obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o

+obj-$(CONFIG_UUID_KUNIT_TEST) += test_uuid.o

obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
diff --git a/lib/test_uuid.c b/lib/test_uuid.c

index cd819c397dc7..65394ec5501e 100644

@@ -33,101 +32,79 @@ static const struct test_uuid_data test_uuid_test_data[] = {

+ KUNIT_ASSERT_EQ_MSG(test, guid_parse(data->uuid, &le), 0,
+ "failed to parse '%s'", data->uuid);
+ KUNIT_EXPECT_TRUE_MSG(test, guid_equal(&data->le, &le),
+ "'%s' should be equal to %pUl", data->uuid, &le);

+ KUNIT_ASSERT_EQ_MSG(test, uuid_parse(data->uuid, &be), 0,
+ "failed to parse '%s'", data->uuid);
+ KUNIT_EXPECT_TRUE_MSG(test, uuid_equal(&data->be, &be),
+ "'%s' should be equal to %pUl", data->uuid, &be);

}

-static void __init test_uuid_wrong(const char *data)
+static void uuid_wrong_le(struct kunit *test)
{
guid_t le;
- uuid_t be;
-
- /* LE */
- total_tests++;
- if (!guid_parse(data, &le))
- test_uuid_failed("negative", true, false, data, NULL);

+ const char **data = (const char **)(test->param_value);

- /* BE */
- total_tests++;
- if (!uuid_parse(data, &be))
- test_uuid_failed("negative", true, true, data, NULL);

+ KUNIT_ASSERT_NE_MSG(test, guid_parse(*data, &le), 0,
+ "parsing of '%s' should've failed", *data);

}

-static int __init test_uuid_init(void)
+static void uuid_wrong_be(struct kunit *test)
{
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(test_uuid_test_data); i++)
- test_uuid_test(&test_uuid_test_data[i]);
-
- for (i = 0; i < ARRAY_SIZE(test_uuid_wrong_data); i++)
- test_uuid_wrong(test_uuid_wrong_data[i]);
+ uuid_t be;

+ const char **data = (const char **)(test->param_value);

- if (failed_tests == 0)
- pr_info("all %u tests passed\n", total_tests);
- else
- pr_err("failed %u out of %u tests\n", failed_tests, total_tests);

+ KUNIT_ASSERT_NE_MSG(test, uuid_parse(*data, &be), 0,
+ "parsing of '%s' should've failed", *data);

+ .name = "uuid",

+ .test_cases = uuid_test_cases,
+};
+kunit_test_suite(uuid_test_suite);

MODULE_AUTHOR("Andy Shevchenko <andriy.s...@linux.intel.com>");
MODULE_LICENSE("Dual BSD/GPL");
--

2.32.0

[Andy Shevchenko]

Thanks!

It's not your fault but I think we need to defer this until KUnit gains support
of the run statistics. My guts telling me if we allow more and more conversions
like this the point will vanish and nobody will care.

I like the code, but I can give my tag after KUnit prints some kind of this:

* This is how the current output looks like in success:

test_uuid: all 18 tests passed

* And when it fails:

test_uuid: failed 18 out of 18 tests

> Changes from v1:
> - Test suite name: uuid_test -> uuid
> - Config name: TEST_UUID -> UUID_KUNIT_TEST
> - Config entry in the Kconfig file left where it is
> - Converted tests to use _MSG variant
>
> André Almeida (1):
> lib: Convert UUID runtime test to KUnit
>
> lib/Kconfig.debug | 11 +++-
> lib/Makefile | 2 +-
> lib/test_uuid.c | 137 +++++++++++++++++++---------------------------
> 3 files changed, 67 insertions(+), 83 deletions(-)
>
> --
> 2.31.1
>

[David Gow]

Note that this output is from the kunit_tool script, which parses the
test output.
It does include a summary line:
[04:41:01] Testing complete. 4 tests run. 0 failed. 0 crashed.

Note that this does only count the number of "tests" run --- the
individual UUIDs are parameters to the same test, so aren't counted
independently by the wrapper at the moment.

That being said, the raw output looks like this (all tests passed):
TAP version 14
1..1
# Subtest: uuid
1..4
# uuid_correct_be: ok 1 - c33f4995-3701-450e-9fbf-206a2e98e576
# uuid_correct_be: ok 2 - 64b4371c-77c1-48f9-8221-29f054fc023b
# uuid_correct_be: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e7f84
ok 1 - uuid_correct_be
# uuid_correct_le: ok 1 - c33f4995-3701-450e-9fbf-206a2e98e576
# uuid_correct_le: ok 2 - 64b4371c-77c1-48f9-8221-29f054fc023b
# uuid_correct_le: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e7f84
ok 2 - uuid_correct_le
# uuid_wrong_be: ok 1 - c33f4995-3701-450e-9fbf206a2e98e576
# uuid_wrong_be: ok 2 - 64b4371c-77c1-48f9-8221-29f054XX023b
# uuid_wrong_be: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e
ok 3 - uuid_wrong_be
# uuid_wrong_le: ok 1 - c33f4995-3701-450e-9fbf206a2e98e576
# uuid_wrong_le: ok 2 - 64b4371c-77c1-48f9-8221-29f054XX023b
# uuid_wrong_le: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e
ok 4 - uuid_wrong_le
ok 1 - uuid

A test which failed could look like this:

# uuid_correct_le: ASSERTION FAILED at lib/test_uuid.c:46

Expected guid_parse(data->uuid, &le) == 0, but
guid_parse(data->uuid, &le) == -22

failed to parse 'c33f499x5-3701-450e-9fbf-206a2e98e576'
# uuid_correct_le: not ok 1 - c33f499x5-3701-450e-9fbf-206a2e98e576
# uuid_correct_le: ok 2 - 64b4371c-77c1-48f9-8221-29f054fc023b
# uuid_correct_le: ok 3 - 0cb4ddff-a545-4401-9d06-688af53e7f84

not ok 2 - uuid_correct_le

>

> Thanks!
>
> It's not your fault but I think we need to defer this until KUnit gains support
> of the run statistics. My guts telling me if we allow more and more conversions
> like this the point will vanish and nobody will care.

Did the test statistics patch we sent out before meet your expectations?
https://patchwork.kernel.org/project/linux-kselftest/patch/20201211072319.5...@google.com/

If so, we can tidy it up and try to push it through straight away, we
were just waiting for a review from someone who wanted the feature.

> I like the code, but I can give my tag after KUnit prints some kind of this:
>
> * This is how the current output looks like in success:
>
> test_uuid: all 18 tests passed
>
> * And when it fails:
>
> test_uuid: failed 18 out of 18 tests
>

There are some small restrictions on the exact format KUnit can use
for this if we want to continue to match the (K)TAP specification
which is being adopted by kselftest. The patch linked above should
give something formatted like:

# test_uuid: (0 / 4) tests failed (0 / 12 test parameters)

Would that work for you?

Cheers,
-- David

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 2:54 PM David Gow <davi...@google.com> wrote:
> On Thu, Jun 10, 2021 at 5:14 PM Andy Shevchenko
> <andriy.s...@linux.intel.com> wrote:
> > On Wed, Jun 09, 2021 at 08:37:29PM -0300, André Almeida wrote:

...

Let me look at it at some point.

> If so, we can tidy it up and try to push it through straight away, we
> were just waiting for a review from someone who wanted the feature.
>
>
> > I like the code, but I can give my tag after KUnit prints some kind of this:
> >
> > * This is how the current output looks like in success:
> >
> > test_uuid: all 18 tests passed
> >
> > * And when it fails:
> >
> > test_uuid: failed 18 out of 18 tests
> >
>
> There are some small restrictions on the exact format KUnit can use
> for this if we want to continue to match the (K)TAP specification
> which is being adopted by kselftest. The patch linked above should
> give something formatted like:
>
> # test_uuid: (0 / 4) tests failed (0 / 12 test parameters)
>
> Would that work for you?

Can you decode it for me, please?

(Assuming that the above question arisen, perhaps some rephrasing is
needed. The idea that user should have clear understanding on how many
test cases were run and how many of them successfully finished or
failed. According to this thread I have to see the cumulative number
of 18 (either as one number or sum over test cases or how you call
them, I see 4 here).

[André Almeida]

Às 09:39 de 10/06/21, Andy Shevchenko escreveu:

In the original code, each `if(uuid/guid_parse/equal)` was considered as
a test, so there were 4 tests for the 3 correct inputs and 2 tests for
the 3 wrong inputs: 4 * 3 + 2 * 3 = 18 tests.

In my patch, I've organized in a different way, with 4 test cases:

- A test case for guid_parse and guid_equal for correct inputs
- A test case for uuid_parse and uuid_equal for correct inputs
- A test case for guid_parse for incorrect inputs
- A test case for uuid_parse for incorrect inputs

So now we have 4 test cases, instead of the 6 test cases in the original
code, because I've united _parse and _equal in a single test case. Given
that each test has 3 parameters, this is why we see 12 test parameters
and that's why there's no "18 tests" around anymore.

[André Almeida]

Às 10:52 de 10/06/21, Andy Shevchenko escreveu:

> On Thu, Jun 10, 2021 at 10:08:30AM -0300, André Almeida wrote:
>> Às 09:39 de 10/06/21, Andy Shevchenko escreveu:
>>> On Thu, Jun 10, 2021 at 2:54 PM David Gow <davi...@google.com> wrote:
>>>> On Thu, Jun 10, 2021 at 5:14 PM Andy Shevchenko
>>>> <andriy.s...@linux.intel.com> wrote:
>>>>> On Wed, Jun 09, 2021 at 08:37:29PM -0300, André Almeida wrote:
>
> ...
>

> I see, is it mentioned in the commit message? If no, please add this
> explanation.
>
> Let's assume 12 now is the correct number, then the output can be somehow
> rephrased, but again, it's not in your patch anyway :-)
>

Sure! I'll add this for v3 :)

[André Almeida]

Hi,

This patch converts existing UUID runtime test to use KUnit framework.

Below, there's a comparison between the old output format and the new
one. Keep in mind that even if KUnit seems very verbose, this is the
corner case where _every_ test has failed.

* This is how the current output looks like in success:

test_uuid: all 18 tests passed

* And when it fails:

test_uuid: conversion test #1 failed on LE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #2 failed on LE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #2 actual data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: conversion test #3 failed on BE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #4 failed on BE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #4 actual data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: conversion test #5 failed on LE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #6 failed on LE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #6 actual data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: conversion test #7 failed on BE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #8 failed on BE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #8 actual data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: conversion test #9 failed on LE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #10 failed on LE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #10 actual data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: conversion test #11 failed on BE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #12 failed on BE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #12 actual data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: negative test #13 passed on wrong LE data: 'c33f4995-3701-450e-9fbf206a2e98e576 '
test_uuid: negative test #14 passed on wrong BE data: 'c33f4995-3701-450e-9fbf206a2e98e576 '
test_uuid: negative test #15 passed on wrong LE data: '64b4371c-77c1-48f9-8221-29f054XX023b'
test_uuid: negative test #16 passed on wrong BE data: '64b4371c-77c1-48f9-8221-29f054XX023b'
test_uuid: negative test #17 passed on wrong LE data: '0cb4ddff-a545-4401-9d06-688af53e'
test_uuid: negative test #18 passed on wrong BE data: '0cb4ddff-a545-4401-9d06-688af53e'

test_uuid: failed 18 out of 18 tests

# uuid_correct_le: ASSERTION FAILED at lib/test_uuid.c:46

Expected guid_parse(data->uuid, &le) == 1, but
guid_parse(data->uuid, &le) == 0

failed to parse 'c33f4995-3701-450e-9fbf-206a2e98e576'
# uuid_correct_le: not ok 1 - c33f4995-3701-450e-9fbf-206a2e98e576

# uuid_correct_le: ASSERTION FAILED at lib/test_uuid.c:46

Expected guid_parse(data->uuid, &le) == 1, but
guid_parse(data->uuid, &le) == 0

failed to parse '64b4371c-77c1-48f9-8221-29f054fc023b'

# uuid_correct_le: not ok 2 - 64b4371c-77c1-48f9-8221-29f054fc023b

# uuid_correct_le: ASSERTION FAILED at lib/test_uuid.c:46

Expected guid_parse(data->uuid, &le) == 1, but
guid_parse(data->uuid, &le) == 0

failed to parse '0cb4ddff-a545-4401-9d06-688af53e7f84'

# uuid_correct_le: not ok 3 - 0cb4ddff-a545-4401-9d06-688af53e7f84

not ok 2 - uuid_correct_le

Changes from v2:
- Clarify in commit message the new test cases setup
v2: https://lore.kernel.org/lkml/20210609233730.16...@collabora.com/

Changes from v1:
- Test suite name: uuid_test -> uuid
- Config name: TEST_UUID -> UUID_KUNIT_TEST
- Config entry in the Kconfig file left where it is
- Converted tests to use _MSG variant

v1: https://lore.kernel.org/lkml/20210605215215.17...@collabora.com/

[André Almeida]

Remove custom functions for testing and use KUnit framework. Keep the
tested functions and test data the same.

Current test threat (g/u)uid_parse and (g/u)uid_equal as different test
cases. Make both functions being part of the same test case, given the
dependency regarding their results. This reduces the tests cases from 6
cases to 4, while keeping the test coverage the same. Given that we have
3 strings for each test case, current test output notifies 18 tests
results, and the KUnit output announces 12 results.

Signed-off-by: André Almeida <andre...@collabora.com>
---

lib/Kconfig.debug | 11 +++-
lib/Makefile | 2 +-
lib/test_uuid.c | 137 +++++++++++++++++++---------------------------
3 files changed, 67 insertions(+), 83 deletions(-)

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 01:39:58PM -0300, André Almeida wrote:
> Hi,
>
> This patch converts existing UUID runtime test to use KUnit framework.
>
> Below, there's a comparison between the old output format and the new
> one. Keep in mind that even if KUnit seems very verbose, this is the
> corner case where _every_ test has failed.

Btw, do we have test coverage statistics?

I mean since we reduced 18 test cases to 12, do we still have the same / better
test coverage?

[André Almeida]

Hi Andy,

Às 06:55 de 11/06/21, Andy Shevchenko escreveu:

I don't think we have automated statistics, but I can assure you that
the coverage it's exactly the same. We are testing two correlated
functions with the same input, in a single test case, instead of having
a single case for each one, so that's why the number of cases is reduced.

For example, instead of:

total_tests++;
if (guid_parse(data->uuid, &le))


total_tests++;
if (!guid_equal(&data->le, &le))

We now have:

KUNIT_ASSERT_EQ(guid_parse(data->guid, &le), 0)
KUNIT_EXPECT_TRUE(guid_equal(&data->le, &le))

That will count as a single test.

[Andy Shevchenko]

On Thu, Jun 10, 2021 at 10:08:30AM -0300, André Almeida wrote:

> Às 09:39 de 10/06/21, Andy Shevchenko escreveu:
> > On Thu, Jun 10, 2021 at 2:54 PM David Gow <davi...@google.com> wrote:
> >> On Thu, Jun 10, 2021 at 5:14 PM Andy Shevchenko
> >> <andriy.s...@linux.intel.com> wrote:
> >>> On Wed, Jun 09, 2021 at 08:37:29PM -0300, André Almeida wrote:

...

I see, is it mentioned in the commit message? If no, please add this
explanation.

Let's assume 12 now is the correct number, then the output can be somehow
rephrased, but again, it's not in your patch anyway :-)

[Christoph Hellwig]

> +config UUID_KUNIT_TEST
> + tristate "Unit test for UUID" if !KUNIT_ALL_TESTS
> + depends on KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + This builds the UUID unit test.

Does this first help line really add any value if we have this second
line:

> + Tests parsing functions for UUID/GUID strings.

?

> + If unsure, say N.

Not specific to this case, but IMHO we can drop this line for all kunit
tests as it is completely obvious.

> @@ -354,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> obj-$(CONFIG_BITS_TEST) += test_bits.o
> obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> +obj-$(CONFIG_UUID_KUNIT_TEST) += test_uuid.o

Another meta-comment on the kunit tests: Wouldn't it make more sense
to name them all as CONFIG_KUNIT_TEST_FOO to allow for easier grepping?

> -struct test_uuid_data {
> +struct test_data {
> const char *uuid;
> guid_t le;
> uuid_t be;
> };
>
> -static const struct test_uuid_data test_uuid_test_data[] = {
> +static const struct test_data correct_data[] = {

What is the reason for these renames? Is this a pattern used for
other kunit tests?

> +static void uuid_correct_le(struct kunit *test)
> {

> + guid_t le;
> + const struct test_data *data = (const struct test_data *)(test->param_value);

Overly long line. But as far as I can tell there is no need for the
case that causes this mess anyway given that param_value is a
"const void *".

Same for all the other instances of this.

> +static void uuid_wrong_le(struct kunit *test)
> {
> guid_t le;

> + const char **data = (const char **)(test->param_value);

No need for the second pair of braces. Same for various other instances.

[Daniel Latypov]

On Sun, Jun 13, 2021 at 11:42 PM Christoph Hellwig <h...@lst.de> wrote:
>
> > +config UUID_KUNIT_TEST
> > + tristate "Unit test for UUID" if !KUNIT_ALL_TESTS
> > + depends on KUNIT
> > + default KUNIT_ALL_TESTS
> > + help
> > + This builds the UUID unit test.
>
> Does this first help line really add any value if we have this second
> line:
>
> > + Tests parsing functions for UUID/GUID strings.
>
> ?
>
> > + If unsure, say N.
>
> Not specific to this case, but IMHO we can drop this line for all kunit
> tests as it is completely obvious.
>
> > @@ -354,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> > obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> > obj-$(CONFIG_BITS_TEST) += test_bits.o
> > obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> > +obj-$(CONFIG_UUID_KUNIT_TEST) += test_uuid.o
>
> Another meta-comment on the kunit tests: Wouldn't it make more sense
> to name them all as CONFIG_KUNIT_TEST_FOO to allow for easier grepping?

But putting them in a "kunit namespace" by prefixing them as such
would be misleading, IMO.
The tests live adjacent to the code they test and are owned by the
same maintainers, or at least that's the intent.

And if the goal is just to find configs, then I don't see much
difference between "config.*KUNIT_TEST" and "config KUNIT_TEST.*"

>
> > -struct test_uuid_data {
> > +struct test_data {
> > const char *uuid;
> > guid_t le;
> > uuid_t be;
> > };
> >
> > -static const struct test_uuid_data test_uuid_test_data[] = {
> > +static const struct test_data correct_data[] = {
>
> What is the reason for these renames? Is this a pattern used for
> other kunit tests?

No, this is not a pattern.
The structs can be renamed back.

>
> > +static void uuid_correct_le(struct kunit *test)
> > {
> > + guid_t le;
> > + const struct test_data *data = (const struct test_data *)(test->param_value);
>
> Overly long line. But as far as I can tell there is no need for the
> case that causes this mess anyway given that param_value is a
> "const void *".

There is no need for the cast or the brace, yes.
This is my fault.

The documentation has both since I had thought that would make how it
works more clear:
https://www.kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing
I don't really understand my past thought process...

[André Almeida]

Às 13:55 de 14/06/21, Daniel Latypov escreveu:

The idea behind this renaming is to be more explicit about what this
data is about: correct UUIDs inputs.

>>
>>> +static void uuid_correct_le(struct kunit *test)
>>> {
>>> + guid_t le;
>>> + const struct test_data *data = (const struct test_data *)(test->param_value);
>>
>> Overly long line. But as far as I can tell there is no need for the
>> case that causes this mess anyway given that param_value is a
>> "const void *".
>
> There is no need for the cast or the brace, yes.
> This is my fault.
>
> The documentation has both since I had thought that would make how it
> works more clear:
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing
> I don't really understand my past thought process...
>

Ok, I'll change my code to remove the cast and braces. I can also send a
patch to rework this part of documentation.

[André Almeida]

Hi,

This patch converts existing UUID runtime test to use KUnit framework.

Below, there's a comparison between the old output format and the new
one. Keep in mind that even if KUnit seems very verbose, this is the
corner case where _every_ test has failed.

* This is how the current output looks like in success:

test_uuid: all 18 tests passed

* And when it fails:

test_uuid: conversion test #1 failed on LE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #2 failed on LE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #2 actual data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: conversion test #3 failed on BE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #4 failed on BE data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: cmp test #4 actual data: 'c33f4995-3701-450e-9fbf-206a2e98e576'
test_uuid: conversion test #5 failed on LE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #6 failed on LE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #6 actual data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: conversion test #7 failed on BE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #8 failed on BE data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: cmp test #8 actual data: '64b4371c-77c1-48f9-8221-29f054fc023b'
test_uuid: conversion test #9 failed on LE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #10 failed on LE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #10 actual data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: conversion test #11 failed on BE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #12 failed on BE data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: cmp test #12 actual data: '0cb4ddff-a545-4401-9d06-688af53e7f84'
test_uuid: negative test #13 passed on wrong LE data: 'c33f4995-3701-450e-9fbf206a2e98e576 '
test_uuid: negative test #14 passed on wrong BE data: 'c33f4995-3701-450e-9fbf206a2e98e576 '
test_uuid: negative test #15 passed on wrong LE data: '64b4371c-77c1-48f9-8221-29f054XX023b'
test_uuid: negative test #16 passed on wrong BE data: '64b4371c-77c1-48f9-8221-29f054XX023b'
test_uuid: negative test #17 passed on wrong LE data: '0cb4ddff-a545-4401-9d06-688af53e'
test_uuid: negative test #18 passed on wrong BE data: '0cb4ddff-a545-4401-9d06-688af53e'

test_uuid: failed 18 out of 18 tests

Changes from v3:
- Drop unnecessary casts and braces.
- Simplify Kconfig entry
v3: https://lore.kernel.org/lkml/20210610163959.71...@collabora.com/

Changes from v2:
- Clarify in commit message the new test cases setup
v2: https://lore.kernel.org/lkml/20210609233730.16...@collabora.com/

Changes from v1:
- Test suite name: uuid_test -> uuid
- Config name: TEST_UUID -> UUID_KUNIT_TEST
- Config entry in the Kconfig file left where it is
- Converted tests to use _MSG variant
v1: https://lore.kernel.org/lkml/20210605215215.17...@collabora.com/

André Almeida (1):
lib: Convert UUID runtime test to KUnit

lib/Kconfig.debug | 8 ++-

lib/Makefile | 2 +-
lib/test_uuid.c | 137 +++++++++++++++++++---------------------------

3 files changed, 64 insertions(+), 83 deletions(-)

--
2.31.1

[André Almeida]

Remove custom functions for testing and use KUnit framework. Keep the
tested functions and test data the same.

Current test threat (g/u)uid_parse and (g/u)uid_equal as different test
cases. Make both functions being part of the same test case, given the
dependency regarding their results. This reduces the tests cases from 6
cases to 4, while keeping the test coverage the same. Given that we have
3 strings for each test case, current test output notifies 18 tests
results, and the KUnit output announces 12 results.

Signed-off-by: André Almeida <andre...@collabora.com>
---

lib/Kconfig.debug | 8 ++-
lib/Makefile | 2 +-
lib/test_uuid.c | 137 +++++++++++++++++++---------------------------
3 files changed, 64 insertions(+), 83 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 678c13967580..606ec5e2586d 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2188,8 +2188,12 @@ config TEST_BITMAP

If unsure, say N.

-config TEST_UUID
- tristate "Test functions located in the uuid module at runtime"

+config UUID_KUNIT_TEST
+ tristate "Unit test for UUID" if !KUNIT_ALL_TESTS
+ depends on KUNIT
+ default KUNIT_ALL_TESTS
+ help

+ Tests parsing functions for UUID/GUID strings.

config TEST_XARRAY
tristate "Test the XArray code at runtime"
diff --git a/lib/Makefile b/lib/Makefile
index 2cc359ec1fdd..cc19048961c0 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -85,7 +85,6 @@ obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_key_base.o
obj-$(CONFIG_TEST_PRINTF) += test_printf.o
obj-$(CONFIG_TEST_BITMAP) += test_bitmap.o
obj-$(CONFIG_TEST_STRSCPY) += test_strscpy.o
-obj-$(CONFIG_TEST_UUID) += test_uuid.o
obj-$(CONFIG_TEST_XARRAY) += test_xarray.o
obj-$(CONFIG_TEST_PARMAN) += test_parman.o
obj-$(CONFIG_TEST_KMOD) += test_kmod.o

@@ -354,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
obj-$(CONFIG_BITS_TEST) += test_bits.o
obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
+obj-$(CONFIG_UUID_KUNIT_TEST) += test_uuid.o

obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o
diff --git a/lib/test_uuid.c b/lib/test_uuid.c

index cd819c397dc7..30f350301e6d 100644

--- a/lib/test_uuid.c
+++ b/lib/test_uuid.c
@@ -1,21 +1,20 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
/*
- * Test cases for lib/uuid.c module.
+ * Unit tests for lib/uuid.c module.
+ *
+ * Copyright 2016 Andy Shevchenko <andriy.s...@linux.intel.com>
+ * Copyright 2021 André Almeida <andre...@riseup.net>
*/
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/string.h>
+#include <kunit/test.h>
#include <linux/uuid.h>

-struct test_uuid_data {
+struct test_data {
const char *uuid;
guid_t le;
uuid_t be;
};

-static const struct test_uuid_data test_uuid_test_data[] = {
+static const struct test_data correct_data[] = {

{
.uuid = "c33f4995-3701-450e-9fbf-206a2e98e576",
.le = GUID_INIT(0xc33f4995, 0x3701, 0x450e, 0x9f, 0xbf, 0x20, 0x6a, 0x2e, 0x98, 0xe5, 0x76),
@@ -33,101 +32,79 @@ static const struct test_uuid_data test_uuid_test_data[] = {
},
};

-static const char * const test_uuid_wrong_data[] = {
+static const char * const wrong_data[] = {
"c33f4995-3701-450e-9fbf206a2e98e576 ", /* no hyphen(s) */
"64b4371c-77c1-48f9-8221-29f054XX023b", /* invalid character(s) */
"0cb4ddff-a545-4401-9d06-688af53e", /* not enough data */
};

-static unsigned total_tests __initdata;
-static unsigned failed_tests __initdata;
-
-static void __init test_uuid_failed(const char *prefix, bool wrong, bool be,
- const char *data, const char *actual)

+static void uuid_correct_le(struct kunit *test)
{

- pr_err("%s test #%u %s %s data: '%s'\n",
- prefix,
- total_tests,
- wrong ? "passed on wrong" : "failed on",
- be ? "BE" : "LE",
- data);
- if (actual && *actual)
- pr_err("%s test #%u actual data: '%s'\n",
- prefix,
- total_tests,
- actual);
- failed_tests++;

+ guid_t le;
+ const struct test_data *data = test->param_value;

+ const struct test_data *data = test->param_value;

+
+ KUNIT_ASSERT_EQ_MSG(test, uuid_parse(data->uuid, &be), 0,
+ "failed to parse '%s'", data->uuid);
+ KUNIT_EXPECT_TRUE_MSG(test, uuid_equal(&data->be, &be),
+ "'%s' should be equal to %pUl", data->uuid, &be);
}

-static void __init test_uuid_wrong(const char *data)

+static void uuid_wrong_le(struct kunit *test)
{
guid_t le;

- uuid_t be;
-
- /* LE */
- total_tests++;
- if (!guid_parse(data, &le))
- test_uuid_failed("negative", true, false, data, NULL);

+ const char * const *data = test->param_value;

- /* BE */
- total_tests++;
- if (!uuid_parse(data, &be))
- test_uuid_failed("negative", true, true, data, NULL);
+ KUNIT_ASSERT_NE_MSG(test, guid_parse(*data, &le), 0,
+ "parsing of '%s' should've failed", *data);
}

-static int __init test_uuid_init(void)
+static void uuid_wrong_be(struct kunit *test)
{
- unsigned int i;
-
- for (i = 0; i < ARRAY_SIZE(test_uuid_test_data); i++)
- test_uuid_test(&test_uuid_test_data[i]);
-
- for (i = 0; i < ARRAY_SIZE(test_uuid_wrong_data); i++)
- test_uuid_wrong(test_uuid_wrong_data[i]);
+ uuid_t be;

+ const char * const *data = test->param_value;

[Daniel Latypov]

On Mon, Jun 21, 2021 at 6:32 AM André Almeida <andre...@collabora.com> wrote:
>
> Remove custom functions for testing and use KUnit framework. Keep the
> tested functions and test data the same.
>
> Current test threat (g/u)uid_parse and (g/u)uid_equal as different test
> cases. Make both functions being part of the same test case, given the
> dependency regarding their results. This reduces the tests cases from 6
> cases to 4, while keeping the test coverage the same. Given that we have
> 3 strings for each test case, current test output notifies 18 tests
> results, and the KUnit output announces 12 results.
>
> Signed-off-by: André Almeida <andre...@collabora.com>

Reviewed-by: Daniel Latypov <dlat...@google.com>

Looks good to me.

I haven't been keeping up, but this won't get picked up until KUnit
prints test statistics?
But even if that's the case or not, this patch would be fine as-is.
It'd be strictly an KUnit-internal thing to print test stats.

[André Almeida]

Às 18:29 de 21/06/21, Daniel Latypov escreveu:

> On Mon, Jun 21, 2021 at 6:32 AM André Almeida <andre...@collabora.com> wrote:
>>
>> Remove custom functions for testing and use KUnit framework. Keep the
>> tested functions and test data the same.
>>
>> Current test threat (g/u)uid_parse and (g/u)uid_equal as different test
>> cases. Make both functions being part of the same test case, given the
>> dependency regarding their results. This reduces the tests cases from 6
>> cases to 4, while keeping the test coverage the same. Given that we have
>> 3 strings for each test case, current test output notifies 18 tests
>> results, and the KUnit output announces 12 results.
>>
>> Signed-off-by: André Almeida <andre...@collabora.com>
>
> Reviewed-by: Daniel Latypov <dlat...@google.com>
>
> Looks good to me.
>
> I haven't been keeping up, but this won't get picked up until KUnit
> prints test statistics?

Good question, I don't think this was decided.

Andy, what do you think?