lib: Convert test_printf.c to KUnit
Subsystems (2)
PATCH v2 1/1 lib: Convert test_printf.c to KUnit
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@CY4PR13MB1175.namprd13.prod.outlook.com/.
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:82vsnprintf(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:82vsnprintf(buf, 5, "%pi4|%pI4", ...) wrote '127.', expected '127-'
# selftest: EXPECTATION FAILED at lib/printf_kunit.c:118kvasprintf(..., "%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
---
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%)lib/Kconfig.debug +4 -3
@@ -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
lib/Makefile +1 -1
@@ -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
lib/printf_kunit.c renamed from lib/test_printf.c +38 -38
166 lines not on this page · raw message
Signature · 1 line
2.25.1
Quoted 37 lines · Arpitha Raghunandan
Convert test
lib/test_printf.cto 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@CY4PR13MB1175.namprd13.prod.outlook.com/.
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 passedOriginal 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 testsConverted test success:
# Subtest: printf-kunit-test
1..1
ok 1 - selftest
ok 1 - printf-kunit-test
Quoted 13 lines
Converted test failure:
# Subtest: printf-kunit-test
1..1
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:82vsnprintf(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 atlib/printf_kunit.c:82vsnprintf(buf, 5, "%pi4|%pI4", ...)wrote '127.', expected '127-'
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:118kvasprintf(..., "%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?
Quoted 7 lines
Signed-off-by Arpitha Raghunandan <[email protected]>---
Changesv1->v2:
- Retain the control flow (early returns) indo_test()
- Display less irrelevant information on test failure
- A more detailed commit message
Quoted 5 lines
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/.
Quoted 225 lines
lib/Kconfig.debug +4
-3@@ -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" helplib/Makefile +1
-1@@ -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.olib/printf_kunit.c renamed from lib/test_printf.c +38
-38@@ -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 <[email protected]>" );MODULE_LICENSE ("GPL" );Signature · 1 line
2.25.1
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 52 lines · Andy Shevchenko
Quoted 37 lines · Arpitha Raghunandan
Convert test
lib/test_printf.cto 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@CY4PR13MB1175.namprd13.prod.outlook.com/.
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 passedOriginal 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 testsConverted test success:
# Subtest: printf-kunit-test
1..1
ok 1 - selftest
ok 1 - printf-kunit-testQuoted 13 lines
Converted test failure:
# Subtest: printf-kunit-test
1..1
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:82vsnprintf(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 atlib/printf_kunit.c:82vsnprintf(buf, 5, "%pi4|%pI4", ...)wrote '127.', expected '127-'
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:118kvasprintf(..., "%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
Quoted 1 line
Not bad. Rasmus, what do you think?
Much better, but that '1..1' and reporting the entire test suite as 1 single (failing or passing) test is (also) a regression. Look at the original
Quoted 1 line
Quoted 1 line
[ 0.591409] test_printf: all 388 tests passed
or
Quoted 1 line
Quoted 1 line
[ 0.619495] test_printf: failed 3 out of 388 tests
That's far more informative, and I'd prefer if the summary information (whether in the all-good case or some-failing) included something like this. In particular, I have at some point spotted that I failed to properly hook up a new test case (or perhaps failed to re-compile, or somehow still ran the old kernel binary, don't remember which it was) by noticing that the total number of tests hadn't increased. The new output would not help catch such PEBKACs.
Rasmus
Quoted 73 lines · Rasmus Villemoes
Quoted 52 lines · Andy Shevchenko
Quoted 37 lines · Arpitha Raghunandan
Convert test
lib/test_printf.cto 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@CY4PR13MB1175.namprd13.prod.outlook.com/.
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 passedOriginal 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 testsConverted test success:
# Subtest: printf-kunit-test
1..1
ok 1 - selftest
ok 1 - printf-kunit-testQuoted 13 lines
Converted test failure:
# Subtest: printf-kunit-test
1..1
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:82vsnprintf(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 atlib/printf_kunit.c:82vsnprintf(buf, 5, "%pi4|%pI4", ...)wrote '127.', expected '127-'
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:118kvasprintf(..., "%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-testQuoted 1 line
Not bad. Rasmus, what do you think?
Much better, but that '1..1' and reporting the entire test suite as 1 single (failing or passing) test is (also) a regression. Look at the original
Quoted 1 line
Quoted 1 line
[ 0.591409] test_printf: all 388 tests passedor
Quoted 1 line
Quoted 1 line
[ 0.619495] test_printf: failed 3 out of 388 testsThat's far more informative, and I'd prefer if the summary information (whether in the all-good case or some-failing) included something like this. In particular, I have at some point spotted that I failed to properly hook up a new test case (or perhaps failed to re-compile, or somehow still ran the old kernel binary, don't remember which it was) by noticing that the total number of tests hadn't increased. The new output would not help catch such PEBKACs.
Quoted 2 lines
Rasmus
Splitting the test into multiple test cases in KUnit will display the number and name of tests that pass or fail. This will be similar to the lib/list-test.c test as can be seen here: https://elixir.bootlin.com/linux/latest/source/lib/list-test.c. I will work on this for the next version of this patch.
Quoted 37 lines · Arpitha Raghunandan
Quoted 36 lines · Rasmus Villemoes
Quoted 15 lines · Andy Shevchenko
Quoted 5 lines · Arpitha Raghunandan
Converted test failure:
# Subtest: printf-kunit-test
1..1
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:82vsnprintf(buf, 256, "%pi4|%pI4", ...)wroteQuoted 8 lines
'127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:82vsnprintf(buf, 5, "%pi4|%pI4", ...)wrote '127.', expected '127-'
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:118kvasprintf(..., "%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-testQuoted 1 line
Not bad. Rasmus, what do you think?
Much better, but that '1..1' and reporting the entire test suite as 1 single (failing or passing) test is (also) a regression. Look at the original
Quoted 1 line
Quoted 1 line
[ 0.591409] test_printf: all 388 tests passedor
Quoted 1 line
Quoted 1 line
[ 0.619495] test_printf: failed 3 out of 388 testsThat's far more informative, and I'd prefer if the summary information (whether in the all-good case or some-failing) included something like this. In particular, I have at some point spotted that I failed to properly hook up a new test case (or perhaps failed to re-compile, or somehow still ran the old kernel binary, don't remember which it was) by noticing that the total number of tests hadn't increased. The new output would not help catch such PEBKACs.
Quoted 8 lines
Quoted 2 lines
Rasmus
Splitting the test into multiple test cases in KUnit will display the number and name of tests that pass or fail. This will be similar to the
lib/list-test.ctest as can be seen here:
https://elixir.bootlin.com/linux/latest/source/lib/list-test.c.
I will work on this for the next version of this patch.
We should probably agree on the granularity first.
It looks like an overkill to split the tests into 388 functions and define KUNIT_CASE() lines. It might be possible to hide this into macros but macros are hell for debugging.
I suggest to split it by the current functions that do more test() call inside. I mean to define something like:
static struct kunit_case printf_test_cases[] = {
KUNIT_CASE(basic),
KUNIT_CASE(number),
KUNIT_CASE(string),
KUNIT_CASE(plain_pointer),
KUNIT_CASE(null_poiter),
KUNIT_CASE(error_pointer),
KUNIT_CASE(addr),
KUNIT_CASE(struct_resource),
KUNIT_CASE(dentry),
KUNIT_CASE(pointer_addr),
...,
{}
};Best Regards,
Petr
Quoted 71 lines · Petr Mladek
Quoted 37 lines · Arpitha Raghunandan
Quoted 36 lines · Rasmus Villemoes
Quoted 15 lines · Andy Shevchenko
Quoted 5 lines · Arpitha Raghunandan
Converted test failure:
# Subtest: printf-kunit-test
1..1
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:82vsnprintf(buf, 256, "%pi4|%pI4", ...)wroteQuoted 8 lines
'127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:82vsnprintf(buf, 5, "%pi4|%pI4", ...)wrote '127.', expected '127-'
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:118kvasprintf(..., "%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-testQuoted 1 line
Not bad. Rasmus, what do you think?
Much better, but that '1..1' and reporting the entire test suite as 1 single (failing or passing) test is (also) a regression. Look at the original
Quoted 1 line
Quoted 1 line
[ 0.591409] test_printf: all 388 tests passedor
Quoted 1 line
Quoted 1 line
[ 0.619495] test_printf: failed 3 out of 388 testsThat's far more informative, and I'd prefer if the summary information (whether in the all-good case or some-failing) included something like this. In particular, I have at some point spotted that I failed to properly hook up a new test case (or perhaps failed to re-compile, or somehow still ran the old kernel binary, don't remember which it was) by noticing that the total number of tests hadn't increased. The new output would not help catch such PEBKACs.
Quoted 8 lines
Quoted 2 lines
Rasmus
Splitting the test into multiple test cases in KUnit will display the number and name of tests that pass or fail. This will be similar to the
lib/list-test.ctest as can be seen here:
https://elixir.bootlin.com/linux/latest/source/lib/list-test.c.
I will work on this for the next version of this patch.We should probably agree on the granularity first.
It looks like an overkill to split the tests into 388 functions and define
KUNIT_CASE()lines. It might be possible to hide this into macros but macros are hell for debugging.I suggest to split it by the current functions that do more
test()call inside. I mean to define something like:static struct kunit_case printf_test_cases[] = { KUNIT_CASE(basic), KUNIT_CASE(number), KUNIT_CASE(string), KUNIT_CASE(plain_pointer), KUNIT_CASE(null_poiter), KUNIT_CASE(error_pointer), KUNIT_CASE(addr), KUNIT_CASE(struct_resource), KUNIT_CASE(dentry), KUNIT_CASE(pointer_addr), ..., {} };
Quoted 3 lines
Best Regards,
Petr
Okay, I will split it by the current functions that have more test() calls inside as suggested. I will also make changes as per your other suggestions for the next version. Thanks!
kernel test robot
build botThank you for the patch! Perhaps something to improve:
Hi Arpitha,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.9 next-20201022]
[cannot apply to pmladek/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Arpitha-Raghunandan/lib-Convert-test_printf-c-to-KUnit/20201022-231505 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f804b3159482eedbb4250b1e9248c308fb63b805 config: mips-allmodconfig (attached as .config) compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/7140dcf9596ec677b7d2a6a38e7f625f86b9e5d4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Arpitha-Raghunandan/lib-Convert-test_printf-c-to-KUnit/20201022-231505 git checkout 7140dcf9596ec677b7d2a6a38e7f625f86b9e5d4
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag as appropriate
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> WARNING: modpost: lib/printf_kunit.o(.data+0x11c): Section mismatch in reference from the variable printf_test_cases to the function .init.text:selftest()
The variable printf_test_cases references
the function __init selftest()
If the reference is valid then annotate the
variable with or __refdata (see linux/init.h) or name the variable:
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
kernel test robot
build botThank you for the patch! Yet something to improve:
Hi Arpitha,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.9 next-20201022]
[cannot apply to pmladek/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Arpitha-Raghunandan/lib-Convert-test_printf-c-to-KUnit/20201022-231505 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f804b3159482eedbb4250b1e9248c308fb63b805 config: sh-allmodconfig (attached as .config) compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/7140dcf9596ec677b7d2a6a38e7f625f86b9e5d4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Arpitha-Raghunandan/lib-Convert-test_printf-c-to-KUnit/20201022-231505 git checkout 7140dcf9596ec677b7d2a6a38e7f625f86b9e5d4
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
All error/warnings (new ones prefixed by >>, old ones prefixed by <<):
>> WARNING: modpost: lib/printf_kunit.o(.data+0x11c): Section mismatch in reference from the variable printf_test_cases to the function .init.text:do_test()
The variable printf_test_cases references
the function __init do_test()
If the reference is valid then annotate the
variable with or __refdata (see linux/init.h) or name the variable:
--
>> ERROR: modpost: "clk_set_min_rate" undefined! >> ERROR: modpost: "scp_get_venc_hw_capa" undefined! >> ERROR: modpost: "scp_ipi_send" undefined! >> ERROR: modpost: "scp_put" undefined! >> ERROR: modpost: "scp_get" undefined! >> ERROR: modpost: "scp_get_vdec_hw_capa" undefined! >> ERROR: modpost: "scp_ipi_register" undefined! >> ERROR: modpost: "scp_mapping_dm_addr" undefined! >> ERROR: modpost: "scp_get_rproc" undefined! >> ERROR: modpost: "rproc_boot" undefined! >> ERROR: modpost: "__delay" undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Quoted 8 lines · Arpitha Raghunandan
Convert test
lib/test_printf.cto KUnit. More information aboutConverted test success:
# Subtest: printf-kunit-test
1..1
ok 1 - selftest
ok 1 - printf-kunit-test
Quoted 12 lines
Converted test failure:
# Subtest: printf-kunit-test
1..1
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:82vsnprintf(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 atlib/printf_kunit.c:82vsnprintf(buf, 5, "%pi4|%pI4", ...)wrote '127.', expected '127-'
# selftest: EXPECTATION FAILED atlib/printf_kunit.c:118kvasprintf(..., "%pi4|%pI4", ...)returned '127.000.000.001|127.0.0.1', expected '127-000.000.001|127.0.0.1'
not ok 1 - selftest
I agree with others that there should be more KUNIT_CASEs.
lib/printf_kunit.c
Quoted 4 lines
not ok 1 - printf-kunit-test
lib/printf_kunit.c renamed from lib/test_printf.c +0
-0
There is no standard at the moment. I see struct kunit_source defined, for example, in the following files:
*test*.c:
drivers/base/power/qos-test.c:
drivers/base/test/property-entry-test.c:
drivers/thunderbolt/test.c:
fs/ext4/inode-test.c:
kernel/kcsan/kcsan-test.c:
kernel/sysctl-test.c:
lib/kunit/string-stream-test.c:
lib/list-test.c:
lib/test_bits.c:
lib/test_kasan.c:
lib/test_linear_ranges.c:
net/mptcp/crypto_test.c:
net/mptcp/token_test.c:
security/apparmor/policy_unpack_test.c:kunit-*-test.c:
lib/kunit/kunit-example-test.c:
lib/kunit/kunit-test.c:
*_kunit.c
lib/bitfield_kunit.c:
Please, either unify names of all the above modules or keep test_printf.c
Quoted 9 lines
lib/printf_kunit.c renamed from lib/test_printf.c +1
-0@@ -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>
lib/printf_kunit.c line 36
Quoted 8 lines
lib/printf_kunit.c renamed from lib/test_printf.c +1
-2@@ -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;
This should be static variable.
Quoted 11 lines
lib/printf_kunit.c renamed from lib/test_printf.c +1
-1-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;
lib/printf_kunit.c test_pointer(void) · line 691
Quoted 9 lines
lib/printf_kunit.c renamed from lib/test_printf.c +2
-1@@ -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 );
The allocation and freeing should be done by the init,exit callbacs in struct kunit_testsuite. For example, see lib/kunit/kunit-test.c
This function can then be removed. The particular tests will be called via more KUNIT_CASE() entries.
Quoted 4 lines
lib/printf_kunit.c renamed from lib/test_printf.c +0
-0if (!alloced_buffer)lib/printf_kunit.c renamed from lib/test_printf.c +0
-0return ;@@ -711,6 +700,17 @@ static void __init selftest(void)kfree (alloced_buffer);
lib/printf_kunit.c static void __init selftest(void) · line 709
Quoted 10 lines
lib/printf_kunit.c renamed from lib/test_printf.c +7
-1} -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" ,
Please, use:
.name = "printf"
The fact that it is kunit-test should be clear from the context.
lib/printf_kunit.c static void __init selftest(void) · line 714
Quoted 5 lines
lib/printf_kunit.c renamed from lib/test_printf.c +5
-0+ .test_cases = printf_test_cases, +}; + +kunit_test_suite (printf_test_suite); +
Best Regards,
Petr
Quoted 2 lines · Petr Mladek
Quoted 1 line · Arpitha Raghunandan
Convert test
lib/test_printf.cto KUnit. More information about
...
Quoted 6 lines
lib/printf_kunit.c
Quoted 4 lines
not ok 1 - printf-kunit-test
lib/printf_kunit.c renamed from lib/test_printf.c +0
-0There is no standard at the moment.
JFYI: from v5.10-rc1 it is expected to have documentation clarifying the naming scheme. Also there is a pending series [1] to move KUnit based test cases to the defined schema.
Quoted 1 line
Please, either unify names of all the above modules or keep
test_printf.c
[1]: https://lore.kernel.org/linux-kselftest/[email protected]/
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 13 lines · Andy Shevchenko
Quoted 2 lines · Petr Mladek
Quoted 1 line · Arpitha Raghunandan
Convert test
lib/test_printf.cto KUnit. More information about...
Quoted 6 lines
lib/printf_kunit.c
Quoted 4 lines
not ok 1 - printf-kunit-test
lib/printf_kunit.c renamed from lib/test_printf.c +0
-0There is no standard at the moment.
Quoted 7 lines
JFYI: from v5.10-rc1 it is expected to have documentation clarifying the naming scheme. Also there is a pending series [1] to move KUnit based test cases to the defined schema.
Quoted 1 line
Please, either unify names of all the above modules or keep
test_printf.c[1]: https://lore.kernel.org/linux-kselftest/[email protected]/
Great, thanks for the pointer. I seems that this patch actually follows the proposed naming schema. I am fine with it then.
Best Regards,
Petr