Re: [PATCH v2] lib: Convert test_printf.c to KUnit
From: Andy Shevchenko
Date: Thu Oct 22 2020 - 15:15:15 EST
On Thu, Oct 22, 2020 at 08:43:49PM +0530, Arpitha Raghunandan wrote:
> Convert test lib/test_printf.c to KUnit. More information about
> KUnit can be found at:
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/index.html.
> KUnit provides a common framework for unit tests in the kernel.
> KUnit and kselftest are standardizing around KTAP, converting this
> test to KUnit makes this test output in KTAP which we are trying to
> make the standard test result format for the kernel. More about
> the KTAP format can be found at:
> https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD830@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/.
> I ran both the original and converted tests as is to produce the
> output for success of the test in the two cases. I also ran these
> tests with a small modification to show the difference in the output
> for failure of the test in both cases. The modification I made is:
> - test("127.000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
> + test("127-000.000.001|127.0.0.1", "%pi4|%pI4", &sa.sin_addr, &sa.sin_addr);
>
> Original test success:
> [ 0.591262] test_printf: loaded.
> [ 0.591409] test_printf: all 388 tests passed
>
> Original test failure:
> [ 0.619345] test_printf: loaded.
> [ 0.619394] test_printf: vsnprintf(buf, 256, "%piS|%pIS", ...)
> wrote '127.000.000.001|127.0.0.1', expected
> '127-000.000.001|127.0.0.1'
> [ 0.619395] test_printf: vsnprintf(buf, 25, "%piS|%pIS", ...) wrote
> '127.000.000.001|127.0.0.', expected '127-000.000.001|127.0.0.'
> [ 0.619396] test_printf: kvasprintf(..., "%piS|%pIS", ...) returned
> '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
> [ 0.619495] test_printf: failed 3 out of 388 tests
>
> Converted test success:
> # Subtest: printf-kunit-test
> 1..1
> ok 1 - selftest
> ok 1 - printf-kunit-test
>
> Converted test failure:
> # Subtest: printf-kunit-test
> 1..1
> # selftest: EXPECTATION FAILED at lib/printf_kunit.c:82
> vsnprintf(buf, 256, "%pi4|%pI4", ...) wrote
> '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
> # selftest: EXPECTATION FAILED at lib/printf_kunit.c:82
> vsnprintf(buf, 5, "%pi4|%pI4", ...) wrote '127.', expected '127-'
> # selftest: EXPECTATION FAILED at lib/printf_kunit.c:118
> kvasprintf(..., "%pi4|%pI4", ...) returned
> '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
> not ok 1 - selftest
> not ok 1 - printf-kunit-test
Not bad. Rasmus, what do you think?
> Signed-off-by: Arpitha Raghunandan <98.arpi@xxxxxxxxx>
> ---
> Changes v1->v2:
> - Retain the control flow (early returns) in do_test()
> - Display less irrelevant information on test failure
> - A more detailed commit message
>
> lib/Kconfig.debug | 7 +--
> lib/Makefile | 2 +-
> lib/{test_printf.c => printf_kunit.c} | 76 +++++++++++++--------------
> 3 files changed, 43 insertions(+), 42 deletions(-)
> rename lib/{test_printf.c => printf_kunit.c} (91%)
I would based this on top of my series that renames existing KUnit based tests
in lib/.
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 66d44d35cc97..e82ca893ed5b 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2053,9 +2053,6 @@ config TEST_STRSCPY
> config TEST_KSTRTOX
> tristate "Test kstrto*() family of functions at runtime"
>
> -config TEST_PRINTF
> - tristate "Test printf() family of functions at runtime"
> -
> config TEST_BITMAP
> tristate "Test bitmap_*() family of functions at runtime"
> help
> @@ -2282,6 +2279,10 @@ config BITS_TEST
>
> If unsure, say N.
>
> +config PRINTF_KUNIT_TEST
> + tristate "KUnit test for printf() family of functions at runtime"
> + depends on KUNIT
> +
> config TEST_UDELAY
> tristate "udelay test driver"
> help
> diff --git a/lib/Makefile b/lib/Makefile
> index ce45af50983a..c323447022b7 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -84,7 +84,6 @@ obj-$(CONFIG_TEST_SORT) += test_sort.o
> obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
> obj-$(CONFIG_TEST_STATIC_KEYS) += test_static_keys.o
> 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
> @@ -352,3 +351,4 @@ obj-$(CONFIG_BITFIELD_KUNIT) += bitfield_kunit.o
> 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_PRINTF_KUNIT_TEST) += printf_kunit.o
> diff --git a/lib/test_printf.c b/lib/printf_kunit.c
> similarity index 91%
> rename from lib/test_printf.c
> rename to lib/printf_kunit.c
> index 7ac87f18a10f..e4dc1b1c8972 100644
> --- a/lib/test_printf.c
> +++ b/lib/printf_kunit.c
> @@ -5,6 +5,7 @@
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> +#include <kunit/test.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -30,64 +31,57 @@
> #define PAD_SIZE 16
> #define FILL_CHAR '$'
>
> -static unsigned total_tests __initdata;
> -static unsigned failed_tests __initdata;
> static char *test_buffer __initdata;
> static char *alloced_buffer __initdata;
> +struct kunit *kunittest;
>
> -static int __printf(4, 0) __init
> +static void __printf(4, 0) __init
> do_test(int bufsize, const char *expect, int elen,
> const char *fmt, va_list ap)
> {
> va_list aq;
> int ret, written;
>
> - total_tests++;
> -
> memset(alloced_buffer, FILL_CHAR, BUF_SIZE + 2*PAD_SIZE);
> va_copy(aq, ap);
> ret = vsnprintf(test_buffer, bufsize, fmt, aq);
> va_end(aq);
>
> if (ret != elen) {
> - pr_warn("vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d\n",
> + KUNIT_FAIL(kunittest, "vsnprintf(buf, %d, \"%s\", ...) returned %d, expected %d\n",
> bufsize, fmt, ret, elen);
> - return 1;
> + return;
> }
>
> if (memchr_inv(alloced_buffer, FILL_CHAR, PAD_SIZE)) {
> - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote before buffer\n", bufsize, fmt);
> - return 1;
> + KUNIT_FAIL(kunittest, "vsnprintf(buf, %d, \"%s\", ...) wrote before buffer\n", bufsize, fmt);
> + return;
> }
>
> if (!bufsize) {
> - if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE)) {
> - pr_warn("vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n",
> - fmt);
> - return 1;
> - }
> - return 0;
> + if (memchr_inv(test_buffer, FILL_CHAR, BUF_SIZE + PAD_SIZE))
> + KUNIT_FAIL(kunittest, "vsnprintf(buf, 0, \"%s\", ...) wrote to buffer\n", fmt);
> + return;
> }
>
> written = min(bufsize-1, elen);
> if (test_buffer[written]) {
> - pr_warn("vsnprintf(buf, %d, \"%s\", ...) did not nul-terminate buffer\n",
> + KUNIT_FAIL(kunittest, "vsnprintf(buf, %d, \"%s\", ...) did not nul-terminate buffer\n",
> bufsize, fmt);
> - return 1;
> + return;
> }
>
> if (memchr_inv(test_buffer + written + 1, FILL_CHAR, BUF_SIZE + PAD_SIZE - (written + 1))) {
> - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
> + KUNIT_FAIL(kunittest, "vsnprintf(buf, %d, \"%s\", ...) wrote beyond the nul-terminator\n",
> bufsize, fmt);
> - return 1;
> + return;
> }
>
> if (memcmp(test_buffer, expect, written)) {
> - pr_warn("vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
> + KUNIT_FAIL(kunittest, "vsnprintf(buf, %d, \"%s\", ...) wrote '%s', expected '%.*s'\n",
> bufsize, fmt, test_buffer, written, expect);
> - return 1;
> + return;
> }
> - return 0;
> }
>
> static void __printf(3, 4) __init
> @@ -98,9 +92,8 @@ __test(const char *expect, int elen, const char *fmt, ...)
> char *p;
>
> if (elen >= BUF_SIZE) {
> - pr_err("error in test suite: expected output length %d too long. Format was '%s'.\n",
> + KUNIT_FAIL(kunittest, "error in test suite: expected output length %d too long. Format was '%s'.\n",
> elen, fmt);
> - failed_tests++;
> return;
> }
>
> @@ -112,19 +105,17 @@ __test(const char *expect, int elen, const char *fmt, ...)
> * enough and 0), and then we also test that kvasprintf would
> * be able to print it as expected.
> */
> - failed_tests += do_test(BUF_SIZE, expect, elen, fmt, ap);
> + do_test(BUF_SIZE, expect, elen, fmt, ap);
> rand = 1 + prandom_u32_max(elen+1);
> /* Since elen < BUF_SIZE, we have 1 <= rand <= BUF_SIZE. */
> - failed_tests += do_test(rand, expect, elen, fmt, ap);
> - failed_tests += do_test(0, expect, elen, fmt, ap);
> + do_test(rand, expect, elen, fmt, ap);
> + do_test(0, expect, elen, fmt, ap);
>
> p = kvasprintf(GFP_KERNEL, fmt, ap);
> if (p) {
> - total_tests++;
> if (memcmp(p, expect, elen+1)) {
> - pr_warn("kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
> + KUNIT_FAIL(kunittest, "kvasprintf(..., \"%s\", ...) returned '%s', expected '%s'\n",
> fmt, p, expect);
> - failed_tests++;
> }
> kfree(p);
> }
> @@ -303,16 +294,13 @@ plain(void)
>
> err = plain_hash();
> if (err) {
> - pr_warn("plain 'p' does not appear to be hashed\n");
> - failed_tests++;
> + KUNIT_FAIL(kunittest, "plain 'p' does not appear to be hashed\n");
> return;
> }
>
> err = plain_format();
> - if (err) {
> - pr_warn("hashing plain 'p' has unexpected format\n");
> - failed_tests++;
> - }
> + if (err)
> + KUNIT_FAIL(kunittest, "hashing plain 'p' has unexpected format\n");
> }
>
> static void __init
> @@ -696,8 +684,9 @@ test_pointer(void)
> fwnode_pointer();
> }
>
> -static void __init selftest(void)
> +static void __init selftest(struct kunit *ktest)
> {
> + kunittest = ktest;
> alloced_buffer = kmalloc(BUF_SIZE + 2*PAD_SIZE, GFP_KERNEL);
> if (!alloced_buffer)
> return;
> @@ -711,6 +700,17 @@ static void __init selftest(void)
> kfree(alloced_buffer);
> }
>
> -KSTM_MODULE_LOADERS(test_printf);
> +static struct kunit_case printf_test_cases[] = {
> + KUNIT_CASE(selftest),
> + {}
> +};
> +
> +static struct kunit_suite printf_test_suite = {
> + .name = "printf-kunit-test",
> + .test_cases = printf_test_cases,
> +};
> +
> +kunit_test_suite(printf_test_suite);
> +
> MODULE_AUTHOR("Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>");
> MODULE_LICENSE("GPL");
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko