[v2] lib: kunit: Convert test_sort to KUnit test
diff mbox series

Message ID 20200729191151.476368-1-vitor@massaru.org
State New
Headers show
Series
  • [v2] lib: kunit: Convert test_sort to KUnit test
Related show

Commit Message

Vitor Massaru Iha July 29, 2020, 7:11 p.m. UTC
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

Comments

andriy.shevchenko@linux.intel.com July 29, 2020, 7:19 p.m. UTC | #1
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?
Brendan Higgins July 29, 2020, 7:59 p.m. UTC | #2
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
Vitor Massaru Iha July 29, 2020, 8:16 p.m. UTC | #3
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
>
>
kernel test robot Aug. 3, 2020, 6:16 a.m. UTC | #4
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
kernel test robot Aug. 3, 2020, 7:28 a.m. UTC | #5
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
andriy.shevchenko@linux.intel.com Aug. 3, 2020, 8:57 a.m. UTC | #6
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

Patch
diff mbox series

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");