| Message ID | 20200729191151.476368-1-vitor@massaru.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] lib: kunit: Convert test_sort to KUnit test | expand |
On Wed, Jul 29, 2020 at 04:11:51PM -0300, Vitor Massaru Iha wrote: > This adds the conversion of the test_sort.c to KUnit test. > > Please apply this commit first (linux-kselftest/kunit-fixes): > 3f37d14b8a3152441f36b6bc74000996679f0998 kunit: kunit_config: Fix parsing of CONFIG options with space Looks like you mixed up commit message and changelog / comments. > Code Style Documentation: [0] > > Fix these warnings Reported-by lkp@intel.com > > WARNING: modpost: vmlinux.o(.data+0x4fc70): Section mismatch in reference from the variable sort_test_cases to the variable .init.text:sort_test > The variable sort_test_cases references > the variable __init sort_test > If the reference is valid then annotate the > variable with or __refdata (see linux/init.h) or name the variable > > WARNING: modpost: lib/sort_kunit.o(.data+0x11c): Section mismatch in reference from the variable sort_test_cases to the function .init.text:sort_test() > The variable sort_test_cases references > the function __init sort_test() > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > Reported-by: kernel test robot <lkp@intel.com> > Link: [0] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-davidgow@google.com/T/#u This should be in different order: Link, Reported-by, SoB. Also [0] is unnecessary > lib/{test_sort.c => sort_kunit.c} | 31 +++++++++++++++---------------- Still opened question why kunit is a suffix? Can't we leave same name? Can't we do it rather prefix?
On Wed, Jul 29, 2020 at 12:19 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Jul 29, 2020 at 04:11:51PM -0300, Vitor Massaru Iha wrote: > > This adds the conversion of the test_sort.c to KUnit test. > > > > Please apply this commit first (linux-kselftest/kunit-fixes): > > 3f37d14b8a3152441f36b6bc74000996679f0998 kunit: kunit_config: Fix parsing of CONFIG options with space > > Looks like you mixed up commit message and changelog / comments. > > > Code Style Documentation: [0] > > > > Fix these warnings Reported-by lkp@intel.com > > > > WARNING: modpost: vmlinux.o(.data+0x4fc70): Section mismatch in reference from the variable sort_test_cases to the variable .init.text:sort_test > > The variable sort_test_cases references > > the variable __init sort_test > > If the reference is valid then annotate the > > variable with or __refdata (see linux/init.h) or name the variable > > > > WARNING: modpost: lib/sort_kunit.o(.data+0x11c): Section mismatch in reference from the variable sort_test_cases to the function .init.text:sort_test() > > The variable sort_test_cases references > > the function __init sort_test() > > > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > > Reported-by: kernel test robot <lkp@intel.com> > > Link: [0] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-davidgow@google.com/T/#u > > This should be in different order: Link, Reported-by, SoB. > Also [0] is unnecessary > > > lib/{test_sort.c => sort_kunit.c} | 31 +++++++++++++++---------------- > > Still opened question why kunit is a suffix? Can't we leave same name? Can't we > do it rather prefix? Sorry to jump in now; I thought Vitor's reply with a link to the new proposed documentation[1] addressed this. The naming convention issue came up about a month ago[2]. The people in the discussion (including myself) came to an agreement on _kunit.c. That being said, the documentation hasn't been accepted yet, so if you really feel strongly about it, now is the time to change it. All that being said, I would rather not discuss that issue here for the benefit of the participants in the preceding discussions. I posted lore links for the relevant threads, which should be easy enough to In-Reply-To your way into. Nevertheless, if it makes it easier, let me know and I can CC you into the discussions. [1] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-davidgow@google.com/T/#u [2] https://lore.kernel.org/linux-kselftest/202006141005.BA19A9D3@keescook/t/#u Cheers
On Wed, Jul 29, 2020 at 4:19 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Jul 29, 2020 at 04:11:51PM -0300, Vitor Massaru Iha wrote: > > This adds the conversion of the test_sort.c to KUnit test. > > > > Please apply this commit first (linux-kselftest/kunit-fixes): > > 3f37d14b8a3152441f36b6bc74000996679f0998 kunit: kunit_config: Fix parsing of CONFIG options with space > > Looks like you mixed up commit message and changelog / comments. > > > Code Style Documentation: [0] > > > > Fix these warnings Reported-by lkp@intel.com > > > > WARNING: modpost: vmlinux.o(.data+0x4fc70): Section mismatch in reference from the variable sort_test_cases to the variable .init.text:sort_test > > The variable sort_test_cases references > > the variable __init sort_test > > If the reference is valid then annotate the > > variable with or __refdata (see linux/init.h) or name the variable > > > > WARNING: modpost: lib/sort_kunit.o(.data+0x11c): Section mismatch in reference from the variable sort_test_cases to the function .init.text:sort_test() > > The variable sort_test_cases references > > the function __init sort_test() > > > Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> > > Reported-by: kernel test robot <lkp@intel.com> > > Link: [0] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-davidgow@google.com/T/#u > > This should be in different order: Link, Reported-by, SoB. > Also [0] is unnecessary Sure, thanks! > > > lib/{test_sort.c => sort_kunit.c} | 31 +++++++++++++++---------------- > > Still opened question why kunit is a suffix? Can't we leave same name? Can't we > do it rather prefix? > > -- > With Best Regards, > Andy Shevchenko > >
Hi Vitor,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on d43c7fb05765152d4d4a39a8ef957c4ea14d8847]
url: https://github.com/0day-ci/linux/commits/Vitor-Massaru-Iha/lib-kunit-Convert-test_sort-to-KUnit-test/20200730-031404
base: d43c7fb05765152d4d4a39a8ef957c4ea14d8847
config: x86_64-randconfig-s022-20200803 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-117-g8c7aee71-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: lib/sort_kunit.o: in function `kunit_test_suites_init':
>> lib/sort_kunit.c:47: undefined reference to `__kunit_test_suites_init'
ld: lib/sort_kunit.o: in function `sort_test':
>> lib/sort_kunit.c:19: undefined reference to `kunit_unary_assert_format'
>> ld: lib/sort_kunit.c:19: undefined reference to `kunit_do_assertion'
>> ld: lib/sort_kunit.c:30: undefined reference to `kunit_fail_assert_format'
ld: lib/sort_kunit.c:30: undefined reference to `kunit_do_assertion'
ld: lib/sort_kunit.o: in function `kunit_test_suites_exit':
>> lib/sort_kunit.c:47: undefined reference to `__kunit_test_suites_exit'
vim +47 lib/sort_kunit.c
13
14 static void __init sort_test(struct kunit *test)
15 {
16 int *a, i, r = 1;
17
18 a = kmalloc_array(TEST_LEN, sizeof(*a), GFP_KERNEL);
> 19 KUNIT_ASSERT_FALSE_MSG(test, a == NULL, "kmalloc_array failed");
20
21 for (i = 0; i < TEST_LEN; i++) {
22 r = (r * 725861) % 6599;
23 a[i] = r;
24 }
25
26 sort(a, TEST_LEN, sizeof(*a), cmpint, NULL);
27
28 for (i = 0; i < TEST_LEN-1; i++)
29 if (a[i] > a[i+1]) {
> 30 KUNIT_FAIL(test, "test has failed");
31 goto exit;
32 }
33 exit:
34 kfree(a);
35 }
36
37 static struct kunit_case __refdata sort_test_cases[] = {
38 KUNIT_CASE(sort_test),
39 {}
40 };
41
42 static struct kunit_suite sort_test_suite = {
43 .name = "sort",
44 .test_cases = sort_test_cases,
45 };
46
> 47 kunit_test_suites(&sort_test_suite);
48
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Vitor,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on d43c7fb05765152d4d4a39a8ef957c4ea14d8847]
url: https://github.com/0day-ci/linux/commits/Vitor-Massaru-Iha/lib-kunit-Convert-test_sort-to-KUnit-test/20200730-031404
base: d43c7fb05765152d4d4a39a8ef957c4ea14d8847
config: microblaze-randconfig-s031-20200803 (attached as .config)
compiler: microblaze-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.2-117-g8c7aee71-dirty
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=microblaze
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
microblaze-linux-ld: lib/sort_kunit.o: in function `kunit_test_suites_init':
lib/sort_kunit.c:47: undefined reference to `__kunit_test_suites_init'
microblaze-linux-ld: lib/sort_kunit.o: in function `sort_test':
lib/sort_kunit.c:19: undefined reference to `kunit_unary_assert_format'
>> microblaze-linux-ld: lib/sort_kunit.c:19: undefined reference to `kunit_do_assertion'
>> microblaze-linux-ld: lib/sort_kunit.c:30: undefined reference to `kunit_fail_assert_format'
microblaze-linux-ld: lib/sort_kunit.c:30: undefined reference to `kunit_do_assertion'
microblaze-linux-ld: lib/sort_kunit.o: in function `kunit_test_suites_exit':
lib/sort_kunit.c:47: undefined reference to `__kunit_test_suites_exit'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Jul 29, 2020 at 12:59:28PM -0700, Brendan Higgins wrote: > On Wed, Jul 29, 2020 at 12:19 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Jul 29, 2020 at 04:11:51PM -0300, Vitor Massaru Iha wrote: ... > > > lib/{test_sort.c => sort_kunit.c} | 31 +++++++++++++++---------------- > > > > Still opened question why kunit is a suffix? Can't we leave same name? Can't we > > do it rather prefix? > > Sorry to jump in now; I thought Vitor's reply with a link to the new > proposed documentation[1] addressed this. The naming convention issue > came up about a month ago[2]. The people in the discussion (including > myself) came to an agreement on _kunit.c. That being said, the > documentation hasn't been accepted yet, so if you really feel strongly > about it, now is the time to change it. My argument is to do something like - ls .../test* vs. ls .../*_kunit.c - use shell completion vs. no completion when looking if there is a test module for something But I agree that this is matter of style. > All that being said, I would rather not discuss that issue here for > the benefit of the participants in the preceding discussions. > > I posted lore links for the relevant threads, which should be easy > enough to In-Reply-To your way into. Nevertheless, if it makes it > easier, let me know and I can CC you into the discussions. No need. I think you have enough clever folks and good ideas behind this. Just put a reference to all these conversion patches to the summary of pros and cons of renaming. > [1] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-davidgow@google.com/T/#u > [2] https://lore.kernel.org/linux-kselftest/202006141005.BA19A9D3@keescook/t/#u
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 9ad9210d70a1..1fe19e78d7ca 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -1874,15 +1874,6 @@ config TEST_MIN_HEAP If unsure, say N. -config TEST_SORT - tristate "Array-based sort test" - depends on DEBUG_KERNEL || m - help - This option enables the self-test function of 'sort()' at boot, - or at module load time. - - If unsure, say N. - config KPROBES_SANITY_TEST bool "Kprobes sanity tests" depends on DEBUG_KERNEL @@ -2185,6 +2176,23 @@ config LINEAR_RANGES_TEST If unsure, say N. +config SORT_KUNIT + tristate "KUnit test for Array-based sort" + depends on DEBUG_KERNEL || m + help + This option enables the KUnit function of 'sort()' at boot, + or at module load time. + + KUnit tests run during boot and output the results to the debug log + in TAP format (http://testanything.org/). Only useful for kernel devs + running the KUnit test harness, and not intended for inclusion into a + production build. + + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index b1c42c10073b..c22bb13b0a08 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -77,7 +77,6 @@ obj-$(CONFIG_TEST_LKM) += test_module.o obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o obj-$(CONFIG_TEST_RHASHTABLE) += test_rhashtable.o -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 @@ -318,3 +317,4 @@ obj-$(CONFIG_OBJAGG) += objagg.o # KUnit tests obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o +obj-$(CONFIG_SORT_KUNIT) += sort_kunit.o diff --git a/lib/test_sort.c b/lib/sort_kunit.c similarity index 55% rename from lib/test_sort.c rename to lib/sort_kunit.c index 52edbe10f2e5..602a234f1e7d 100644 --- a/lib/test_sort.c +++ b/lib/sort_kunit.c @@ -1,7 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only #include <linux/sort.h> -#include <linux/slab.h> -#include <linux/module.h> +#include <kunit/test.h> /* a simple boot-time regression test */ @@ -12,13 +11,12 @@ static int __init cmpint(const void *a, const void *b) return *(int *)a - *(int *)b; } -static int __init test_sort_init(void) +static void __init sort_test(struct kunit *test) { - int *a, i, r = 1, err = -ENOMEM; + int *a, i, r = 1; a = kmalloc_array(TEST_LEN, sizeof(*a), GFP_KERNEL); - if (!a) - return err; + KUNIT_ASSERT_FALSE_MSG(test, a == NULL, "kmalloc_array failed"); for (i = 0; i < TEST_LEN; i++) { r = (r * 725861) % 6599; @@ -27,24 +25,25 @@ static int __init test_sort_init(void) sort(a, TEST_LEN, sizeof(*a), cmpint, NULL); - err = -EINVAL; for (i = 0; i < TEST_LEN-1; i++) if (a[i] > a[i+1]) { - pr_err("test has failed\n"); + KUNIT_FAIL(test, "test has failed"); goto exit; } - err = 0; - pr_info("test passed\n"); exit: kfree(a); - return err; } -static void __exit test_sort_exit(void) -{ -} +static struct kunit_case __refdata sort_test_cases[] = { + KUNIT_CASE(sort_test), + {} +}; + +static struct kunit_suite sort_test_suite = { + .name = "sort", + .test_cases = sort_test_cases, +}; -module_init(test_sort_init); -module_exit(test_sort_exit); +kunit_test_suites(&sort_test_suite); MODULE_LICENSE("GPL");
This adds the conversion of the test_sort.c to KUnit test. Please apply this commit first (linux-kselftest/kunit-fixes): 3f37d14b8a3152441f36b6bc74000996679f0998 kunit: kunit_config: Fix parsing of CONFIG options with space Code Style Documentation: [0] Fix these warnings Reported-by lkp@intel.com WARNING: modpost: vmlinux.o(.data+0x4fc70): Section mismatch in reference from the variable sort_test_cases to the variable .init.text:sort_test The variable sort_test_cases references the variable __init sort_test If the reference is valid then annotate the variable with or __refdata (see linux/init.h) or name the variable WARNING: modpost: lib/sort_kunit.o(.data+0x11c): Section mismatch in reference from the variable sort_test_cases to the function .init.text:sort_test() The variable sort_test_cases references the function __init sort_test() Signed-off-by: Vitor Massaru Iha <vitor@massaru.org> Reported-by: kernel test robot <lkp@intel.com> Link: [0] https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-davidgow@google.com/T/#u --- v2: * Add Kunit Code Style reference in commit message; * Fix lkp@intel.com warning report; --- lib/Kconfig.debug | 26 +++++++++++++++++--------- lib/Makefile | 2 +- lib/{test_sort.c => sort_kunit.c} | 31 +++++++++++++++---------------- 3 files changed, 33 insertions(+), 26 deletions(-) rename lib/{test_sort.c => sort_kunit.c} (55%) base-commit: d43c7fb05765152d4d4a39a8ef957c4ea14d8847 -- 2.26.2