lib: add basic KUnit test for lib/math

Commit Message

Daniel Latypov Oct. 19, 2020, 10:45 p.m. UTC

Add basic test coverage for files that don't require any config options:
* gcd.c
* lcm.c
* int_sqrt.c
* reciprocal_div.c
(Ignored int_pow.c since it's a simple textbook algorithm.)

These tests aren't particularly interesting, but
* they're chosen as easy to understand examples of how to write tests
* provides a place to add tests for any new files in this dir
* written so adding new test cases to cover edge cases should be easy

Signed-off-by: Daniel Latypov <[e-mail]>
---
 lib/math/Kconfig     |   5 ++
 lib/math/Makefile    |   2 +
 lib/math/math_test.c | 197 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+)
 create mode 100644 lib/math/math_test.c


base-commit: 7cf726a59435301046250c42131554d9ccc566b8

Comments

kernel test robot Oct. 20, 2020, 12:45 a.m. UTC | #1

Hi Daniel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 7cf726a59435301046250c42131554d9ccc566b8]

url:    https://github.com/0day-ci/linux/commits/Daniel-Latypov/lib-add-basic-KUnit-test-for-lib-math/20201020-064737
base:    7cf726a59435301046250c42131554d9ccc566b8
config: nds32-allyesconfig (attached as .config)
compiler: nds32le-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/e268c8ae297dc311e5deb6b25daad9a2f88309ba
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Latypov/lib-add-basic-KUnit-test-for-lib-math/20201020-064737
        git checkout e268c8ae297dc311e5deb6b25daad9a2f88309ba
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

   lib/math/math_test.c: In function 'int_sqrt_test':
>> lib/math/math_test.c:137:13: warning: right shift count >= width of type [-Wshift-count-overflow]
     137 |    .a = 1UL >> 32,
         |             ^~

vim +137 lib/math/math_test.c

   109	
   110	static void int_sqrt_test(struct kunit *test)
   111	{
   112		const char *message_fmt = "sqrt(%lu)";
   113		int i;
   114	
   115		struct test_case test_cases[] = {
   116			{
   117				.a = 0,
   118				.result = 0,
   119			},
   120			{
   121				.a = 1,
   122				.result = 1,
   123			},
   124			{
   125				.a = 4,
   126				.result = 2,
   127			},
   128			{
   129				.a = 5,
   130				.result = 2,
   131			},
   132			{
   133				.a = 8,
   134				.result = 2,
   135			},
   136			{
 > 137				.a = 1UL >> 32,
   138				.result = 1UL >> 16,
   139			},
   140		};
   141	
   142		for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
   143			KUNIT_EXPECT_EQ_MSG(test, int_sqrt(test_cases[i].a),
   144					    test_cases[i].result, message_fmt,
   145					    test_cases[i].a);
   146		}
   147	}
   148	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Andy Shevchenko Oct. 20, 2020, 8:09 a.m. UTC | #2

On Mon, Oct 19, 2020 at 03:45:56PM -0700, Daniel Latypov wrote:
> Add basic test coverage for files that don't require any config options:
> * gcd.c
> * lcm.c
> * int_sqrt.c
> * reciprocal_div.c
> (Ignored int_pow.c since it's a simple textbook algorithm.)
> 
> These tests aren't particularly interesting, but
> * they're chosen as easy to understand examples of how to write tests
> * provides a place to add tests for any new files in this dir
> * written so adding new test cases to cover edge cases should be easy

Is documentation not enough?

I have recently wrote my first KUnit test and I found documentation pretty good
for the start. I think we actually need more complex examples in the code (and
useful).

So, I'm in doubt these test are a good point to start with.

Daniel Latypov Oct. 20, 2020, 4:13 p.m. UTC | #3

On Tue, Oct 20, 2020 at 1:08 AM Andy Shevchenko
<[e-mail]> wrote:
>
> On Mon, Oct 19, 2020 at 03:45:56PM -0700, Daniel Latypov wrote:
> > Add basic test coverage for files that don't require any config options:
> > * gcd.c
> > * lcm.c
> > * int_sqrt.c
> > * reciprocal_div.c
> > (Ignored int_pow.c since it's a simple textbook algorithm.)
> >
> > These tests aren't particularly interesting, but
> > * they're chosen as easy to understand examples of how to write tests
> > * provides a place to add tests for any new files in this dir
> > * written so adding new test cases to cover edge cases should be easy
>
> Is documentation not enough?
>
> I have recently wrote my first KUnit test and I found documentation pretty good
> for the start. I think we actually need more complex examples in the code (and
> useful).

I should have been more clear.
The documentation clearly covers the mechanics of how to set up a test
suite with some test cases and KUNIT_EXPECT_* calls.

But it doesn't provide much guidance in the way of _how_ to structure tests.
Or how to use more advanced features, e.g. there are only two files
tree-wide using the _MSG variants of macros:
$ ag KUNIT_.*MSG -l --ignore include/kunit/test.h
fs/ext4/inode-test.c
lib/bitfield_kunit.c
(And both happen to be written by people working on/with KUnit).

While the _MSG macros are not perfect, they add some context when
calling KUNIT_* in a loop.
I already see some tests merged that probably would benefit from using it.
Considering the perspective of an outsider whose change broke one of
those tests, they'd need to first edit the test to add more debug info
to even understand what failed.

But I can see a case for mentioning the _MSG variants in the example
test or Documentation/kunit instead of this patch.
There doesn't seem to be any mention of them at present in the docs.

To put it in kunit_example_test.c (and have the value be clear), we'd
need something like
@@ -27,6 +28,9 @@ static void example_simple_test(struct kunit *test)
         * behavior matched what was expected.
         */
        KUNIT_EXPECT_EQ(test, 1 + 1, 2);
+
+       for (x = 0; x < 2; ++x)
+               KUNIT_EXPECT_EQ_MSG(test, 42, myfunc(x), "myfunc(%d)", x);
 }

Using and testing an actual function like gcd() et al. felt a bit
better than adding a trivial function there.

>
> So, I'm in doubt these test are a good point to start with.

If the above still seems too contrived, I can take a look at where to
update kunit/usage.rst instead.




>
> --
> With Best Regards,
> Andy Shevchenko
>
>

David Gow Oct. 21, 2020, 3:40 a.m. UTC | #4

On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <[e-mail]> wrote:
>
> Add basic test coverage for files that don't require any config options:
> * gcd.c
> * lcm.c
> * int_sqrt.c
> * reciprocal_div.c
> (Ignored int_pow.c since it's a simple textbook algorithm.)
>
I don't see a particular reason why int_pow.c being a simple algorithm
means it shouldn't be tested. I'm not saying it has to be tested by
this particular change -- and I doubt the test would be
earth-shatteringly interesting -- but there's no real reason against
testing it.

> These tests aren't particularly interesting, but
> * they're chosen as easy to understand examples of how to write tests
> * provides a place to add tests for any new files in this dir
> * written so adding new test cases to cover edge cases should be easy

I think these tests can stand on their own merits, rather than just as
examples (though I do think they do make good additional examples for
how to test these sorts of functions).
So, I'd treat this as an actual test of the maths functions (and
you've got what seems to me a decent set of test cases for that,
though there are a couple of comments below) first, and any use it
gains as an example is sort-of secondary to that (anything that makes
it a better example is likely to make it a better test anyway).

In any case, modulo the comments below, this seems good to me.

-- David

> Signed-off-by: Daniel Latypov <[e-mail]>
> ---
>  lib/math/Kconfig     |   5 ++
>  lib/math/Makefile    |   2 +
>  lib/math/math_test.c | 197 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 204 insertions(+)
>  create mode 100644 lib/math/math_test.c
>
> diff --git a/lib/math/Kconfig b/lib/math/Kconfig
> index f19bc9734fa7..6ba8680439c1 100644
> --- a/lib/math/Kconfig
> +++ b/lib/math/Kconfig
> @@ -15,3 +15,8 @@ config PRIME_NUMBERS
>
>  config RATIONAL
>         bool
> +
> +config MATH_KUNIT_TEST
> +       tristate "KUnit test for lib/math" if !KUNIT_ALL_TESTS
> +       default KUNIT_ALL_TESTS
> +       depends on KUNIT
> diff --git a/lib/math/Makefile b/lib/math/Makefile
> index be6909e943bd..fba6fe90f50b 100644
> --- a/lib/math/Makefile
> +++ b/lib/math/Makefile
> @@ -4,3 +4,5 @@ obj-y += div64.o gcd.o lcm.o int_pow.o int_sqrt.o reciprocal_div.o
>  obj-$(CONFIG_CORDIC)           += cordic.o
>  obj-$(CONFIG_PRIME_NUMBERS)    += prime_numbers.o
>  obj-$(CONFIG_RATIONAL)         += rational.o
> +
> +obj-$(CONFIG_MATH_KUNIT_TEST)  += math_test.o
> diff --git a/lib/math/math_test.c b/lib/math/math_test.c
> new file mode 100644
> index 000000000000..6f4681ea7c72
> --- /dev/null
> +++ b/lib/math/math_test.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Simple KUnit suite for math helper funcs that are always enabled.
> + *
> + * Copyright (C) 2020, Google LLC.
> + * Author: Daniel Latypov <[e-mail]>
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/gcd.h>
> +#include <linux/kernel.h>
> +#include <linux/lcm.h>
> +#include <linux/reciprocal_div.h>
> +
> +/* Generic test case for unsigned long inputs. */
> +struct test_case {
> +       unsigned long a, b;
> +       unsigned long result;
> +};
> +
> +static void gcd_test(struct kunit *test)
> +{
> +       const char *message_fmt = "gcd(%lu, %lu)";
> +       int i;
> +
> +       struct test_case test_cases[] = {
> +               {
> +                       .a = 0, .b = 1,
> +                       .result = 1,
> +               },
> +               {
> +                       .a = 2, .b = 2,
> +                       .result = 2,
> +               },
> +               {
> +                       .a = 2, .b = 4,
> +                       .result = 2,
> +               },
> +               {
> +                       .a = 3, .b = 5,
> +                       .result = 1,
> +               },
> +               {
> +                       .a = 3*9, .b = 3*5,
> +                       .result = 3,
> +               },
> +               {
> +                       .a = 3*5*7, .b = 3*5*11,
> +                       .result = 15,
> +               },
> +               {
> +                       .a = (1 << 21) - 1,
> +                       .b = (1 << 22) - 1,

It might be worth noting the factors of these (7^2*127*337 and
3*23*89*683) in a comment.
They aren't mersenne primes, if that's what you were going for, though
they are coprime.

> +                       .result = 1,
> +               },
> +       };
> +
> +       for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
> +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> +                                   gcd(test_cases[i].a, test_cases[i].b),
> +                                   message_fmt, test_cases[i].a,
> +                                   test_cases[i].b);
> +
> +               /* gcd(a,b) == gcd(b,a) */
> +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> +                                   gcd(test_cases[i].b, test_cases[i].a),
> +                                   message_fmt, test_cases[i].b,
> +                                   test_cases[i].a);
> +       }
> +}
> +
> +static void lcm_test(struct kunit *test)
> +{
> +       const char *message_fmt = "lcm(%lu, %lu)";
> +       int i;
> +
> +       struct test_case test_cases[] = {
> +               {
> +                       .a = 0, .b = 1,
> +                       .result = 0,
> +               },
> +               {
> +                       .a = 1, .b = 2,
> +                       .result = 2,
> +               },
> +               {
> +                       .a = 2, .b = 2,
> +                       .result = 2,
> +               },
> +               {
> +                       .a = 3*5, .b = 3*7,
> +                       .result = 3*5*7,
> +               },

If you were looking for extra testcases here, one where b < a would be nice.

> +       };
> +
> +       for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
> +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> +                                   lcm(test_cases[i].a, test_cases[i].b),
> +                                   message_fmt, test_cases[i].a,
> +                                   test_cases[i].b);
> +
> +               /* lcm(a,b) == lcm(b,a) */
> +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> +                                   lcm(test_cases[i].b, test_cases[i].a),
> +                                   message_fmt, test_cases[i].b,
> +                                   test_cases[i].a);
> +       }
> +}
> +

Again, not pushing for it in this test, but lcm_not_zero() could be
worth testing at some point, too...

> +static void int_sqrt_test(struct kunit *test)
> +{
> +       const char *message_fmt = "sqrt(%lu)";
> +       int i;
> +
> +       struct test_case test_cases[] = {
> +               {
> +                       .a = 0,
> +                       .result = 0,
> +               },
> +               {
> +                       .a = 1,
> +                       .result = 1,
> +               },
> +               {
> +                       .a = 4,
> +                       .result = 2,
> +               },
> +               {
> +                       .a = 5,
> +                       .result = 2,
> +               },
> +               {
> +                       .a = 8,
> +                       .result = 2,
> +               },
> +               {
> +                       .a = 1UL >> 32,
> +                       .result = 1UL >> 16,

As the kernel test robot noted, these are wrong (the shifts go the
wrong way, 2^32 might not fit in an unsigned long).

> +               },
> +       };
> +
> +       for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
> +               KUNIT_EXPECT_EQ_MSG(test, int_sqrt(test_cases[i].a),
> +                                   test_cases[i].result, message_fmt,
> +                                   test_cases[i].a);
> +       }
> +}
> +
> +struct reciprocal_test_case {
> +       u32 a, b;
> +       u32 result;
> +};
> +
> +static void reciprocal_div_test(struct kunit *test)
> +{
> +       int i;
> +       struct reciprocal_value rv;
> +       struct reciprocal_test_case test_cases[] = {
> +               {
> +                       .a = 0, .b = 1,
> +                       .result = 0,
> +               },
> +               {
> +                       .a = 42, .b = 20,
> +                       .result = 2,
> +               },
> +               {
> +                       .a = (1<<16), .b = (1<<14),
> +                       .result = 1<<2,
> +               },
> +       };
> +
> +       for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
> +               rv = reciprocal_value(test_cases[i].b);
> +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> +                                   reciprocal_divide(test_cases[i].a, rv),
> +                                   "reciprocal_divide(%u, %u)",
> +                                   test_cases[i].a, test_cases[i].b);
> +       }
> +}
> +
> +static struct kunit_case math_test_cases[] = {
> +       KUNIT_CASE(gcd_test),
> +       KUNIT_CASE(lcm_test),
> +       KUNIT_CASE(int_sqrt_test),
> +       KUNIT_CASE(reciprocal_div_test),
> +       {}
> +};
> +
> +static struct kunit_suite math_test_suite = {
> +       .name = "lib-math",
> +       .test_cases = math_test_cases,
> +};
> +
> +kunit_test_suites(&math_test_suite);
> +
> +MODULE_LICENSE("GPL v2");
>
> base-commit: 7cf726a59435301046250c42131554d9ccc566b8
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>

Daniel Latypov Oct. 21, 2020, 5:47 p.m. UTC | #5

On Tue, Oct 20, 2020 at 8:40 PM David Gow <[e-mail]> wrote:
>
> On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <[e-mail]> wrote:
> >
> > Add basic test coverage for files that don't require any config options:
> > * gcd.c
> > * lcm.c
> > * int_sqrt.c
> > * reciprocal_div.c
> > (Ignored int_pow.c since it's a simple textbook algorithm.)
> >
> I don't see a particular reason why int_pow.c being a simple algorithm
> means it shouldn't be tested. I'm not saying it has to be tested by
> this particular change -- and I doubt the test would be
> earth-shatteringly interesting -- but there's no real reason against
> testing it.

Agreed on principle, but int_pow() feels like a special case.
I've written it the exact same way (modulo variable names+types)
several times in personal projects.
Even the spacing matched exactly in a few of those...

>
> > These tests aren't particularly interesting, but
> > * they're chosen as easy to understand examples of how to write tests
> > * provides a place to add tests for any new files in this dir
> > * written so adding new test cases to cover edge cases should be easy
>
> I think these tests can stand on their own merits, rather than just as
> examples (though I do think they do make good additional examples for
> how to test these sorts of functions).
> So, I'd treat this as an actual test of the maths functions (and
> you've got what seems to me a decent set of test cases for that,
> though there are a couple of comments below) first, and any use it
> gains as an example is sort-of secondary to that (anything that makes
> it a better example is likely to make it a better test anyway).
>
> In any case, modulo the comments below, this seems good to me.

Ack.
I'll wait on Andy's input before deciding whether or not to push out a
v2 with the changes.

>
> -- David
>
> > Signed-off-by: Daniel Latypov <[e-mail]>
> > ---
> >  lib/math/Kconfig     |   5 ++
> >  lib/math/Makefile    |   2 +
> >  lib/math/math_test.c | 197 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 204 insertions(+)
> >  create mode 100644 lib/math/math_test.c
> >
> > diff --git a/lib/math/Kconfig b/lib/math/Kconfig
> > index f19bc9734fa7..6ba8680439c1 100644
> > --- a/lib/math/Kconfig
> > +++ b/lib/math/Kconfig
> > @@ -15,3 +15,8 @@ config PRIME_NUMBERS
> >
> >  config RATIONAL
> >         bool
> > +
> > +config MATH_KUNIT_TEST
> > +       tristate "KUnit test for lib/math" if !KUNIT_ALL_TESTS
> > +       default KUNIT_ALL_TESTS
> > +       depends on KUNIT
> > diff --git a/lib/math/Makefile b/lib/math/Makefile
> > index be6909e943bd..fba6fe90f50b 100644
> > --- a/lib/math/Makefile
> > +++ b/lib/math/Makefile
> > @@ -4,3 +4,5 @@ obj-y += div64.o gcd.o lcm.o int_pow.o int_sqrt.o reciprocal_div.o
> >  obj-$(CONFIG_CORDIC)           += cordic.o
> >  obj-$(CONFIG_PRIME_NUMBERS)    += prime_numbers.o
> >  obj-$(CONFIG_RATIONAL)         += rational.o
> > +
> > +obj-$(CONFIG_MATH_KUNIT_TEST)  += math_test.o
> > diff --git a/lib/math/math_test.c b/lib/math/math_test.c
> > new file mode 100644
> > index 000000000000..6f4681ea7c72
> > --- /dev/null
> > +++ b/lib/math/math_test.c
> > @@ -0,0 +1,197 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Simple KUnit suite for math helper funcs that are always enabled.
> > + *
> > + * Copyright (C) 2020, Google LLC.
> > + * Author: Daniel Latypov <[e-mail]>
> > + */
> > +
> > +#include <kunit/test.h>
> > +#include <linux/gcd.h>
> > +#include <linux/kernel.h>
> > +#include <linux/lcm.h>
> > +#include <linux/reciprocal_div.h>
> > +
> > +/* Generic test case for unsigned long inputs. */
> > +struct test_case {
> > +       unsigned long a, b;
> > +       unsigned long result;
> > +};
> > +
> > +static void gcd_test(struct kunit *test)
> > +{
> > +       const char *message_fmt = "gcd(%lu, %lu)";
> > +       int i;
> > +
> > +       struct test_case test_cases[] = {
> > +               {
> > +                       .a = 0, .b = 1,
> > +                       .result = 1,
> > +               },
> > +               {
> > +                       .a = 2, .b = 2,
> > +                       .result = 2,
> > +               },
> > +               {
> > +                       .a = 2, .b = 4,
> > +                       .result = 2,
> > +               },
> > +               {
> > +                       .a = 3, .b = 5,
> > +                       .result = 1,
> > +               },
> > +               {
> > +                       .a = 3*9, .b = 3*5,
> > +                       .result = 3,
> > +               },
> > +               {
> > +                       .a = 3*5*7, .b = 3*5*11,
> > +                       .result = 15,
> > +               },
> > +               {
> > +                       .a = (1 << 21) - 1,
> > +                       .b = (1 << 22) - 1,
>
> It might be worth noting the factors of these (7^2*127*337 and
> 3*23*89*683) in a comment.
> They aren't mersenne primes, if that's what you were going for, though
> they are coprime.

Yes, they aren't primes.
But 2^x-1 2^(x+1)-1 should always be coprime.
So I figured it was an easy way to get "large" coprimes.

I can pick a pair of Mersenne primes (and comment to that effect) if
you think that'd be clearer.

>
> > +                       .result = 1,
> > +               },
> > +       };
> > +
> > +       for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
> > +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> > +                                   gcd(test_cases[i].a, test_cases[i].b),
> > +                                   message_fmt, test_cases[i].a,
> > +                                   test_cases[i].b);
> > +
> > +               /* gcd(a,b) == gcd(b,a) */
> > +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> > +                                   gcd(test_cases[i].b, test_cases[i].a),
> > +                                   message_fmt, test_cases[i].b,
> > +                                   test_cases[i].a);
> > +       }
> > +}
> > +
> > +static void lcm_test(struct kunit *test)
> > +{
> > +       const char *message_fmt = "lcm(%lu, %lu)";
> > +       int i;
> > +
> > +       struct test_case test_cases[] = {
> > +               {
> > +                       .a = 0, .b = 1,
> > +                       .result = 0,
> > +               },
> > +               {
> > +                       .a = 1, .b = 2,
> > +                       .result = 2,
> > +               },
> > +               {
> > +                       .a = 2, .b = 2,
> > +                       .result = 2,
> > +               },
> > +               {
> > +                       .a = 3*5, .b = 3*7,
> > +                       .result = 3*5*7,
> > +               },
>
> If you were looking for extra testcases here, one where b < a would be nice.

Good point, added
+               {
+                       .a = 42, .b = 9999,
+                       .result = 0,
+               },

>
> > +       };
> > +
> > +       for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
> > +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> > +                                   lcm(test_cases[i].a, test_cases[i].b),
> > +                                   message_fmt, test_cases[i].a,
> > +                                   test_cases[i].b);
> > +
> > +               /* lcm(a,b) == lcm(b,a) */
> > +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> > +                                   lcm(test_cases[i].b, test_cases[i].a),
> > +                                   message_fmt, test_cases[i].b,
> > +                                   test_cases[i].a);
> > +       }
> > +}
> > +
>
> Again, not pushing for it in this test, but lcm_not_zero() could be
> worth testing at some point, too...

Ack.
I'll hold off on adding that for now, since reusing the lcm table
would require basically a copy paste of the internal logic to figure
out what we'd expect in the zero case.
And atm, it doesn't seem interesting enough to add another separate
test case, esp. if there's already concern the lcm test isn't really
worth having.

>
> > +static void int_sqrt_test(struct kunit *test)
> > +{
> > +       const char *message_fmt = "sqrt(%lu)";
> > +       int i;
> > +
> > +       struct test_case test_cases[] = {
> > +               {
> > +                       .a = 0,
> > +                       .result = 0,
> > +               },
> > +               {
> > +                       .a = 1,
> > +                       .result = 1,
> > +               },
> > +               {
> > +                       .a = 4,
> > +                       .result = 2,
> > +               },
> > +               {
> > +                       .a = 5,
> > +                       .result = 2,
> > +               },
> > +               {
> > +                       .a = 8,
> > +                       .result = 2,
> > +               },
> > +               {
> > +                       .a = 1UL >> 32,
> > +                       .result = 1UL >> 16,
>
> As the kernel test robot noted, these are wrong (the shifts go the
> wrong way, 2^32 might not fit in an unsigned long).

Thanks, fixed locally.


>
> > +               },
> > +       };
> > +
> > +       for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
> > +               KUNIT_EXPECT_EQ_MSG(test, int_sqrt(test_cases[i].a),
> > +                                   test_cases[i].result, message_fmt,
> > +                                   test_cases[i].a);
> > +       }
> > +}
> > +
> > +struct reciprocal_test_case {
> > +       u32 a, b;
> > +       u32 result;
> > +};
> > +
> > +static void reciprocal_div_test(struct kunit *test)
> > +{
> > +       int i;
> > +       struct reciprocal_value rv;
> > +       struct reciprocal_test_case test_cases[] = {
> > +               {
> > +                       .a = 0, .b = 1,
> > +                       .result = 0,
> > +               },
> > +               {
> > +                       .a = 42, .b = 20,
> > +                       .result = 2,
> > +               },
> > +               {
> > +                       .a = (1<<16), .b = (1<<14),
> > +                       .result = 1<<2,
> > +               },
> > +       };
> > +
> > +       for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
> > +               rv = reciprocal_value(test_cases[i].b);
> > +               KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
> > +                                   reciprocal_divide(test_cases[i].a, rv),
> > +                                   "reciprocal_divide(%u, %u)",
> > +                                   test_cases[i].a, test_cases[i].b);
> > +       }
> > +}
> > +
> > +static struct kunit_case math_test_cases[] = {
> > +       KUNIT_CASE(gcd_test),
> > +       KUNIT_CASE(lcm_test),
> > +       KUNIT_CASE(int_sqrt_test),
> > +       KUNIT_CASE(reciprocal_div_test),
> > +       {}
> > +};
> > +
> > +static struct kunit_suite math_test_suite = {
> > +       .name = "lib-math",
> > +       .test_cases = math_test_cases,
> > +};
> > +
> > +kunit_test_suites(&math_test_suite);
> > +
> > +MODULE_LICENSE("GPL v2");
> >
> > base-commit: 7cf726a59435301046250c42131554d9ccc566b8
> > --
> > 2.29.0.rc1.297.gfa9743e501-goog
> >

Andy Shevchenko Oct. 22, 2020, 3:06 p.m. UTC | #6

On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote:
> On Tue, Oct 20, 2020 at 8:40 PM David Gow <[e-mail]> wrote:
> > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <[e-mail]> wrote:
> > >
> > > Add basic test coverage for files that don't require any config options:
> > > * gcd.c
> > > * lcm.c
> > > * int_sqrt.c
> > > * reciprocal_div.c
> > > (Ignored int_pow.c since it's a simple textbook algorithm.)
> > >
> > I don't see a particular reason why int_pow.c being a simple algorithm
> > means it shouldn't be tested. I'm not saying it has to be tested by
> > this particular change -- and I doubt the test would be
> > earth-shatteringly interesting -- but there's no real reason against
> > testing it.
> 
> Agreed on principle, but int_pow() feels like a special case.
> I've written it the exact same way (modulo variable names+types)
> several times in personal projects.
> Even the spacing matched exactly in a few of those...

But if you would like to *teach* somebody by this exemplary piece of code, you
better do it close to ideal.

> > > These tests aren't particularly interesting, but
> > > * they're chosen as easy to understand examples of how to write tests
> > > * provides a place to add tests for any new files in this dir
> > > * written so adding new test cases to cover edge cases should be easy
> >
> > I think these tests can stand on their own merits, rather than just as
> > examples (though I do think they do make good additional examples for
> > how to test these sorts of functions).
> > So, I'd treat this as an actual test of the maths functions (and
> > you've got what seems to me a decent set of test cases for that,
> > though there are a couple of comments below) first, and any use it
> > gains as an example is sort-of secondary to that (anything that makes
> > it a better example is likely to make it a better test anyway).
> >
> > In any case, modulo the comments below, this seems good to me.
> 
> Ack.
> I'll wait on Andy's input before deciding whether or not to push out a
> v2 with the changes.

You need to put detailed comments in the code to have it as real example how to
create the KUnit test. But hey, it will mean that documentation sucks. So,
please update documentation to cover issues that you found and which motivated
you to create these test cases.

Summarize this, please create usable documentation first.

Daniel Latypov Oct. 22, 2020, 4:26 p.m. UTC | #7

On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko
<[e-mail]> wrote:
>
> On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote:
> > On Tue, Oct 20, 2020 at 8:40 PM David Gow <[e-mail]> wrote:
> > > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <[e-mail]> wrote:
> > > >
> > > > Add basic test coverage for files that don't require any config options:
> > > > * gcd.c
> > > > * lcm.c
> > > > * int_sqrt.c
> > > > * reciprocal_div.c
> > > > (Ignored int_pow.c since it's a simple textbook algorithm.)
> > > >
> > > I don't see a particular reason why int_pow.c being a simple algorithm
> > > means it shouldn't be tested. I'm not saying it has to be tested by
> > > this particular change -- and I doubt the test would be
> > > earth-shatteringly interesting -- but there's no real reason against
> > > testing it.
> >
> > Agreed on principle, but int_pow() feels like a special case.
> > I've written it the exact same way (modulo variable names+types)
> > several times in personal projects.
> > Even the spacing matched exactly in a few of those...
>
> But if you would like to *teach* somebody by this exemplary piece of code, you
> better do it close to ideal.
>
> > > > These tests aren't particularly interesting, but
> > > > * they're chosen as easy to understand examples of how to write tests
> > > > * provides a place to add tests for any new files in this dir
> > > > * written so adding new test cases to cover edge cases should be easy
> > >
> > > I think these tests can stand on their own merits, rather than just as
> > > examples (though I do think they do make good additional examples for
> > > how to test these sorts of functions).
> > > So, I'd treat this as an actual test of the maths functions (and
> > > you've got what seems to me a decent set of test cases for that,
> > > though there are a couple of comments below) first, and any use it
> > > gains as an example is sort-of secondary to that (anything that makes
> > > it a better example is likely to make it a better test anyway).
> > >
> > > In any case, modulo the comments below, this seems good to me.
> >
> > Ack.
> > I'll wait on Andy's input before deciding whether or not to push out a
> > v2 with the changes.
>
> You need to put detailed comments in the code to have it as real example how to
> create the KUnit test. But hey, it will mean that documentation sucks. So,

Out of curiosity
* By "it will mean that documentation sucks," do you mean that the
documentation will rot faster if people are using the existing in-tree
tests as their entrypoint?
* What level of detailed comments? On the level of kunit-example-test.c?
  * More concretely, then we'd have a comment block linking to the
example and then describing table driven unit testing?
  * And then maybe another block about invariants instead of the
perhaps too-terse /* gcd(a,b) == gcd(b,a) */ ?

> please update documentation to cover issues that you found and which motivated
> you to create these test cases.
>
> Summarize this, please create usable documentation first.

Sounds good.
I'm generally wary people not reading the docs, and of documentation
examples becoming bitrotted faster than actual code.
But so far KUnit seems to be doing relatively well on both fronts.

usage.rst is currently the best place for this, but it felt like it
would quickly become a dumping ground for miscellaneous tips and
tricks.
I'll spend some time thinking if we want a new file or not, based on
how much I want to write about the things this test demonstrated
(EXPECT_*MSG, table driven tests, testing invariants, etc).

In offline discussions with David, the idea had come up with having a
set of (relatively) simple tests in tree that the documentation could
point to as examples of these things. That would keep the line count
in usage.rst down a bit.
(But then it would necessitate more tests like this math_test.c)


>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Brendan Higgins Oct. 22, 2020, 6:51 p.m. UTC | #8

On Thu, Oct 22, 2020 at 9:26 AM Daniel Latypov <[e-mail]> wrote:
>
> On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko
> <[e-mail]> wrote:
> >
> > On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote:
> > > On Tue, Oct 20, 2020 at 8:40 PM David Gow <[e-mail]> wrote:
> > > > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <[e-mail]> wrote:
> > > > >
> > > > > Add basic test coverage for files that don't require any config options:
> > > > > * gcd.c
> > > > > * lcm.c
> > > > > * int_sqrt.c
> > > > > * reciprocal_div.c
> > > > > (Ignored int_pow.c since it's a simple textbook algorithm.)
> > > > >
> > > > I don't see a particular reason why int_pow.c being a simple algorithm
> > > > means it shouldn't be tested. I'm not saying it has to be tested by
> > > > this particular change -- and I doubt the test would be
> > > > earth-shatteringly interesting -- but there's no real reason against
> > > > testing it.
> > >
> > > Agreed on principle, but int_pow() feels like a special case.
> > > I've written it the exact same way (modulo variable names+types)
> > > several times in personal projects.
> > > Even the spacing matched exactly in a few of those...
> >
> > But if you would like to *teach* somebody by this exemplary piece of code, you
> > better do it close to ideal.
> >
> > > > > These tests aren't particularly interesting, but
> > > > > * they're chosen as easy to understand examples of how to write tests
> > > > > * provides a place to add tests for any new files in this dir
> > > > > * written so adding new test cases to cover edge cases should be easy
> > > >
> > > > I think these tests can stand on their own merits, rather than just as
> > > > examples (though I do think they do make good additional examples for
> > > > how to test these sorts of functions).
> > > > So, I'd treat this as an actual test of the maths functions (and
> > > > you've got what seems to me a decent set of test cases for that,
> > > > though there are a couple of comments below) first, and any use it
> > > > gains as an example is sort-of secondary to that (anything that makes
> > > > it a better example is likely to make it a better test anyway).
> > > >
> > > > In any case, modulo the comments below, this seems good to me.
> > >
> > > Ack.
> > > I'll wait on Andy's input before deciding whether or not to push out a
> > > v2 with the changes.
> >
> > You need to put detailed comments in the code to have it as real example how to
> > create the KUnit test. But hey, it will mean that documentation sucks. So,
>
> Out of curiosity
> * By "it will mean that documentation sucks," do you mean that the
> documentation will rot faster if people are using the existing in-tree
> tests as their entrypoint?
> * What level of detailed comments? On the level of kunit-example-test.c?
>   * More concretely, then we'd have a comment block linking to the
> example and then describing table driven unit testing?
>   * And then maybe another block about invariants instead of the
> perhaps too-terse /* gcd(a,b) == gcd(b,a) */ ?
>
> > please update documentation to cover issues that you found and which motivated
> > you to create these test cases.
> >
> > Summarize this, please create usable documentation first.
>
> Sounds good.
> I'm generally wary people not reading the docs, and of documentation
> examples becoming bitrotted faster than actual code.
> But so far KUnit seems to be doing relatively well on both fronts.
>
> usage.rst is currently the best place for this, but it felt like it
> would quickly become a dumping ground for miscellaneous tips and
> tricks.

Yeah, I think it has already started to become a dumping ground for
everything; it already has everything except: getting started, FAQ,
API reference, and some minor details about the command line tool.

> I'll spend some time thinking if we want a new file or not, based on
> how much I want to write about the things this test demonstrated
> (EXPECT_*MSG, table driven tests, testing invariants, etc).
>
> In offline discussions with David, the idea had come up with having a
> set of (relatively) simple tests in tree that the documentation could
> point to as examples of these things. That would keep the line count
> in usage.rst down a bit.
> (But then it would necessitate more tests like this math_test.c)

I do like the idea of having as much example code as possible in a
place where it actually gets compiled occasionally. One of the biggest
bitrot issues we have had in KUnit documentation so far is example
code falling out of date; that's less likely to happen if the examples
get compiled.

It would be super cool if there was some way to have example code in
the documentation that also gets compiled and run, but I don't believe
that that would be a feasible thing to do right now.

In any case, not to sound hypocritical or lazy, but when I use other
people's code I tend to trust the code a lot more than I trust the
docs. Even if we do an absolutely stellar job with our docs, I suspect
other devs will feel the same.

Brendan Higgins Oct. 22, 2020, 6:53 p.m. UTC | #9

On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko
<[e-mail]> wrote:
>
> On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote:
> > On Tue, Oct 20, 2020 at 8:40 PM David Gow <[e-mail]> wrote:
> > > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <[e-mail]> wrote:
> > > >
> > > > Add basic test coverage for files that don't require any config options:
> > > > * gcd.c
> > > > * lcm.c
> > > > * int_sqrt.c
> > > > * reciprocal_div.c
> > > > (Ignored int_pow.c since it's a simple textbook algorithm.)
> > > >
> > > I don't see a particular reason why int_pow.c being a simple algorithm
> > > means it shouldn't be tested. I'm not saying it has to be tested by
> > > this particular change -- and I doubt the test would be
> > > earth-shatteringly interesting -- but there's no real reason against
> > > testing it.
> >
> > Agreed on principle, but int_pow() feels like a special case.
> > I've written it the exact same way (modulo variable names+types)
> > several times in personal projects.
> > Even the spacing matched exactly in a few of those...
>
> But if you would like to *teach* somebody by this exemplary piece of code, you
> better do it close to ideal.
>
> > > > These tests aren't particularly interesting, but
> > > > * they're chosen as easy to understand examples of how to write tests
> > > > * provides a place to add tests for any new files in this dir
> > > > * written so adding new test cases to cover edge cases should be easy
> > >
> > > I think these tests can stand on their own merits, rather than just as
> > > examples (though I do think they do make good additional examples for
> > > how to test these sorts of functions).
> > > So, I'd treat this as an actual test of the maths functions (and
> > > you've got what seems to me a decent set of test cases for that,
> > > though there are a couple of comments below) first, and any use it
> > > gains as an example is sort-of secondary to that (anything that makes
> > > it a better example is likely to make it a better test anyway).
> > >
> > > In any case, modulo the comments below, this seems good to me.
> >
> > Ack.
> > I'll wait on Andy's input before deciding whether or not to push out a
> > v2 with the changes.
>
> You need to put detailed comments in the code to have it as real example how to
> create the KUnit test. But hey, it will mean that documentation sucks. So,
> please update documentation to cover issues that you found and which motivated
> you to create these test cases.

I don't entirely disagree; leaning too heavily on code examples can be
detrimental to docs. That being said, when I use other people's code,
I often don't even look at the docs. So, I think the ideal is to have
both.

> Summarize this, please create usable documentation first.

Andy Shevchenko Oct. 22, 2020, 7:05 p.m. UTC | #10

On Thu, Oct 22, 2020 at 11:53:50AM -0700, Brendan Higgins wrote:
> On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko
> <[e-mail]> wrote:
> > On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote:

...

> > You need to put detailed comments in the code to have it as real example how to
> > create the KUnit test. But hey, it will mean that documentation sucks. So,
> > please update documentation to cover issues that you found and which motivated
> > you to create these test cases.
> 
> I don't entirely disagree; leaning too heavily on code examples can be
> detrimental to docs. That being said, when I use other people's code,
> I often don't even look at the docs. So, I think the ideal is to have
> both.

Why do we have docs in the first place?
For test cases I think it's a crucial part, because tests many time are written
by newbies, who would like to understand all under-the-hood stuff. You imply
that they need to drop themselves into the code directly. It's very harsh to
begin with Linux kernel development, really.

> > Summarize this, please create usable documentation first.

So, no go for this w/o documentation being up-to-date. Or be honest to
everybody, it's sucks it needs to be removed. Then I will get your point.

Andy Shevchenko Oct. 22, 2020, 7:10 p.m. UTC | #11

On Thu, Oct 22, 2020 at 09:26:45AM -0700, Daniel Latypov wrote:
> On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko
> <[e-mail]> wrote:
> > On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote:

...

> > You need to put detailed comments in the code to have it as real example how to
> > create the KUnit test. But hey, it will mean that documentation sucks. So,
> 
> Out of curiosity
> * By "it will mean that documentation sucks," do you mean that the
> documentation will rot faster if people are using the existing in-tree
> tests as their entrypoint?

Yes. And it will discourage to write documentation as well and read.
Good documentation is like a good book. I like how doc.python.org works for me
when I need something new to get about it, for example.

> * What level of detailed comments? On the level of kunit-example-test.c?
>   * More concretely, then we'd have a comment block linking to the

Something like explaining each line with KUNIT / kunit in it.
What it does and why it's written in the given form. Something like that.

> example and then describing table driven unit testing?
>   * And then maybe another block about invariants instead of the
> perhaps too-terse /* gcd(a,b) == gcd(b,a) */ ?

Right.

> > please update documentation to cover issues that you found and which motivated
> > you to create these test cases.
> >
> > Summarize this, please create usable documentation first.
> 
> Sounds good.
> I'm generally wary people not reading the docs, and of documentation
> examples becoming bitrotted faster than actual code.
> But so far KUnit seems to be doing relatively well on both fronts.

Dunno. As I told, I have created first unit test based on documentation (okay,
I looked at the code, but you may read this as ratio was 90% doc / 10% existing
code).

> usage.rst is currently the best place for this, but it felt like it
> would quickly become a dumping ground for miscellaneous tips and
> tricks.
> I'll spend some time thinking if we want a new file or not, based on
> how much I want to write about the things this test demonstrated
> (EXPECT_*MSG, table driven tests, testing invariants, etc).

Thanks!

> In offline discussions with David, the idea had come up with having a
> set of (relatively) simple tests in tree that the documentation could
> point to as examples of these things. That would keep the line count
> in usage.rst down a bit.
> (But then it would necessitate more tests like this math_test.c)

Andy Shevchenko Oct. 22, 2020, 7:12 p.m. UTC | #12

On Thu, Oct 22, 2020 at 10:10:38PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 22, 2020 at 09:26:45AM -0700, Daniel Latypov wrote:
> > On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko
> > <[e-mail]> wrote:
> > > On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote:

...

> > > Summarize this, please create usable documentation first.
> > 
> > Sounds good.
> > I'm generally wary people not reading the docs, and of documentation
> > examples becoming bitrotted faster than actual code.
> > But so far KUnit seems to be doing relatively well on both fronts.
> 
> Dunno. As I told, I have created first unit test based on documentation (okay,
> I looked at the code, but you may read this as ratio was 90% doc / 10% existing
> code).

Side note: some cases are not described in doc and produced "interesting"
results that I have to look a lot into KUnit Python wrapper to fix bugs in it.

You may find my patches in the KUnit mailing list.

Brendan Higgins Oct. 22, 2020, 9:21 p.m. UTC | #13

On Thu, Oct 22, 2020 at 12:04 PM Andy Shevchenko
<[e-mail]> wrote:
>
> On Thu, Oct 22, 2020 at 11:53:50AM -0700, Brendan Higgins wrote:
> > On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko
> > <[e-mail]> wrote:
> > > On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote:
>
> ...
>
> > > You need to put detailed comments in the code to have it as real example how to
> > > create the KUnit test. But hey, it will mean that documentation sucks. So,
> > > please update documentation to cover issues that you found and which motivated
> > > you to create these test cases.
> >
> > I don't entirely disagree; leaning too heavily on code examples can be
> > detrimental to docs. That being said, when I use other people's code,
> > I often don't even look at the docs. So, I think the ideal is to have
> > both.
>
> Why do we have docs in the first place?
> For test cases I think it's a crucial part, because tests many time are written
> by newbies, who would like to understand all under-the-hood stuff. You imply

Good point. Yeah, we don't really have any documentation that explains
the internals at all. I agree: we need to fix that.

> that they need to drop themselves into the code directly. It's very harsh to
> begin with Linux kernel development, really.

No, I was not trying to imply that everyone should just jump in the
code and ignore the docs. I was just trying to point out that some
people - myself included - rather see code than docs. Clearly some
people prefer docs over code as well. Thus, I was trying to argue that
both are appropriate.

Nevertheless, based on the other comments you sent, I don't think
that's what we are talking about: sounds like you just want us to
improve the docs first to make sure we do it. That's fine with me.

> > > Summarize this, please create usable documentation first.
>
> So, no go for this w/o documentation being up-to-date. Or be honest to
> everybody, it's sucks it needs to be removed. Then I will get your point.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Andy Shevchenko Oct. 23, 2020, 9:02 a.m. UTC | #14

On Thu, Oct 22, 2020 at 02:21:40PM -0700, Brendan Higgins wrote:
> On Thu, Oct 22, 2020 at 12:04 PM Andy Shevchenko
> <[e-mail]> wrote:
> > On Thu, Oct 22, 2020 at 11:53:50AM -0700, Brendan Higgins wrote:
> > > On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko
> > > <[e-mail]> wrote:

...

> > Why do we have docs in the first place?
> > For test cases I think it's a crucial part, because tests many time are written
> > by newbies, who would like to understand all under-the-hood stuff. You imply
> 
> Good point. Yeah, we don't really have any documentation that explains
> the internals at all. I agree: we need to fix that.
> 
> > that they need to drop themselves into the code directly. It's very harsh to
> > begin with Linux kernel development, really.
> 
> No, I was not trying to imply that everyone should just jump in the
> code and ignore the docs. I was just trying to point out that some
> people - myself included - rather see code than docs. Clearly some
> people prefer docs over code as well. Thus, I was trying to argue that
> both are appropriate.
> 
> Nevertheless, based on the other comments you sent, I don't think
> that's what we are talking about: sounds like you just want us to
> improve the docs first to make sure we do it. That's fine with me.

Right. What confused me is that test cases for math were pushed as a good
example how to create a test case, but at the same time docs left untouched.

kernel test robot Nov. 2, 2020, 2:51 p.m. UTC | #15

Hi Daniel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 7cf726a59435301046250c42131554d9ccc566b8]

url:    https://github.com/0day-ci/linux/commits/Daniel-Latypov/lib-add-basic-KUnit-test-for-lib-math/20201020-064737
base:    7cf726a59435301046250c42131554d9ccc566b8
:::::: branch date: 13 days ago
:::::: commit date: 13 days ago
config: i386-randconfig-s002-20201101 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-68-g49c98aa3-dirty
        # https://github.com/0day-ci/linux/commit/e268c8ae297dc311e5deb6b25daad9a2f88309ba
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Latypov/lib-add-basic-KUnit-test-for-lib-math/20201020-064737
        git checkout e268c8ae297dc311e5deb6b25daad9a2f88309ba
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>


"sparse warnings: (new ones prefixed by >>)"
>> lib/math/math_test.c:137:37: sparse: sparse: shift too big (32) for type unsigned long

vim +137 lib/math/math_test.c

e268c8ae297dc31 Daniel Latypov 2020-10-19  109  
e268c8ae297dc31 Daniel Latypov 2020-10-19  110  static void int_sqrt_test(struct kunit *test)
e268c8ae297dc31 Daniel Latypov 2020-10-19  111  {
e268c8ae297dc31 Daniel Latypov 2020-10-19  112  	const char *message_fmt = "sqrt(%lu)";
e268c8ae297dc31 Daniel Latypov 2020-10-19  113  	int i;
e268c8ae297dc31 Daniel Latypov 2020-10-19  114  
e268c8ae297dc31 Daniel Latypov 2020-10-19  115  	struct test_case test_cases[] = {
e268c8ae297dc31 Daniel Latypov 2020-10-19  116  		{
e268c8ae297dc31 Daniel Latypov 2020-10-19  117  			.a = 0,
e268c8ae297dc31 Daniel Latypov 2020-10-19  118  			.result = 0,
e268c8ae297dc31 Daniel Latypov 2020-10-19  119  		},
e268c8ae297dc31 Daniel Latypov 2020-10-19  120  		{
e268c8ae297dc31 Daniel Latypov 2020-10-19  121  			.a = 1,
e268c8ae297dc31 Daniel Latypov 2020-10-19  122  			.result = 1,
e268c8ae297dc31 Daniel Latypov 2020-10-19  123  		},
e268c8ae297dc31 Daniel Latypov 2020-10-19  124  		{
e268c8ae297dc31 Daniel Latypov 2020-10-19  125  			.a = 4,
e268c8ae297dc31 Daniel Latypov 2020-10-19  126  			.result = 2,
e268c8ae297dc31 Daniel Latypov 2020-10-19  127  		},
e268c8ae297dc31 Daniel Latypov 2020-10-19  128  		{
e268c8ae297dc31 Daniel Latypov 2020-10-19  129  			.a = 5,
e268c8ae297dc31 Daniel Latypov 2020-10-19  130  			.result = 2,
e268c8ae297dc31 Daniel Latypov 2020-10-19  131  		},
e268c8ae297dc31 Daniel Latypov 2020-10-19  132  		{
e268c8ae297dc31 Daniel Latypov 2020-10-19  133  			.a = 8,
e268c8ae297dc31 Daniel Latypov 2020-10-19  134  			.result = 2,
e268c8ae297dc31 Daniel Latypov 2020-10-19  135  		},
e268c8ae297dc31 Daniel Latypov 2020-10-19  136  		{
e268c8ae297dc31 Daniel Latypov 2020-10-19 @137  			.a = 1UL >> 32,
e268c8ae297dc31 Daniel Latypov 2020-10-19  138  			.result = 1UL >> 16,
e268c8ae297dc31 Daniel Latypov 2020-10-19  139  		},
e268c8ae297dc31 Daniel Latypov 2020-10-19  140  	};
e268c8ae297dc31 Daniel Latypov 2020-10-19  141  
e268c8ae297dc31 Daniel Latypov 2020-10-19  142  	for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
e268c8ae297dc31 Daniel Latypov 2020-10-19  143  		KUNIT_EXPECT_EQ_MSG(test, int_sqrt(test_cases[i].a),
e268c8ae297dc31 Daniel Latypov 2020-10-19  144  				    test_cases[i].result, message_fmt,
e268c8ae297dc31 Daniel Latypov 2020-10-19  145  				    test_cases[i].a);
e268c8ae297dc31 Daniel Latypov 2020-10-19  146  	}
e268c8ae297dc31 Daniel Latypov 2020-10-19  147  }
e268c8ae297dc31 Daniel Latypov 2020-10-19  148  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

kernel test robot Nov. 3, 2020, 1:32 a.m. UTC | #16

Hi Daniel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 7cf726a59435301046250c42131554d9ccc566b8]

url:    https://github.com/0day-ci/linux/commits/Daniel-Latypov/lib-add-basic-KUnit-test-for-lib-math/20201020-064737
base:    7cf726a59435301046250c42131554d9ccc566b8
config: mips-randconfig-r032-20201030 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project fa5a13276764a2657b3571fa3c57b07ee5d2d661)
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
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/e268c8ae297dc311e5deb6b25daad9a2f88309ba
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Latypov/lib-add-basic-KUnit-test-for-lib-math/20201020-064737
        git checkout e268c8ae297dc311e5deb6b25daad9a2f88309ba
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> lib/math/math_test.c:137:13: warning: shift count >= width of type [-Wshift-count-overflow]
                           .a = 1UL >> 32,
                                    ^  ~~
   1 warning generated.

vim +137 lib/math/math_test.c

   109	
   110	static void int_sqrt_test(struct kunit *test)
   111	{
   112		const char *message_fmt = "sqrt(%lu)";
   113		int i;
   114	
   115		struct test_case test_cases[] = {
   116			{
   117				.a = 0,
   118				.result = 0,
   119			},
   120			{
   121				.a = 1,
   122				.result = 1,
   123			},
   124			{
   125				.a = 4,
   126				.result = 2,
   127			},
   128			{
   129				.a = 5,
   130				.result = 2,
   131			},
   132			{
   133				.a = 8,
   134				.result = 2,
   135			},
   136			{
 > 137				.a = 1UL >> 32,
   138				.result = 1UL >> 16,
   139			},
   140		};
   141	
   142		for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
   143			KUNIT_EXPECT_EQ_MSG(test, int_sqrt(test_cases[i].a),
   144					    test_cases[i].result, message_fmt,
   145					    test_cases[i].a);
   146		}
   147	}
   148	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Patch

diff --git a/lib/math/Kconfig b/lib/math/Kconfig
index f19bc9734fa7..6ba8680439c1 100644
--- a/lib/math/Kconfig
+++ b/lib/math/Kconfig
@@ -15,3 +15,8 @@  config PRIME_NUMBERS
 
 config RATIONAL
 	bool
+
+config MATH_KUNIT_TEST
+	tristate "KUnit test for lib/math" if !KUNIT_ALL_TESTS
+	default KUNIT_ALL_TESTS
+	depends on KUNIT
diff --git a/lib/math/Makefile b/lib/math/Makefile
index be6909e943bd..fba6fe90f50b 100644
--- a/lib/math/Makefile
+++ b/lib/math/Makefile
@@ -4,3 +4,5 @@  obj-y += div64.o gcd.o lcm.o int_pow.o int_sqrt.o reciprocal_div.o
 obj-$(CONFIG_CORDIC)		+= cordic.o
 obj-$(CONFIG_PRIME_NUMBERS)	+= prime_numbers.o
 obj-$(CONFIG_RATIONAL)		+= rational.o
+
+obj-$(CONFIG_MATH_KUNIT_TEST)	+= math_test.o
diff --git a/lib/math/math_test.c b/lib/math/math_test.c
new file mode 100644
index 000000000000..6f4681ea7c72
--- /dev/null
+++ b/lib/math/math_test.c
@@ -0,0 +1,197 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Simple KUnit suite for math helper funcs that are always enabled.
+ *
+ * Copyright (C) 2020, Google LLC.
+ * Author: Daniel Latypov <[e-mail]>
+ */
+
+#include <kunit/test.h>
+#include <linux/gcd.h>
+#include <linux/kernel.h>
+#include <linux/lcm.h>
+#include <linux/reciprocal_div.h>
+
+/* Generic test case for unsigned long inputs. */
+struct test_case {
+	unsigned long a, b;
+	unsigned long result;
+};
+
+static void gcd_test(struct kunit *test)
+{
+	const char *message_fmt = "gcd(%lu, %lu)";
+	int i;
+
+	struct test_case test_cases[] = {
+		{
+			.a = 0, .b = 1,
+			.result = 1,
+		},
+		{
+			.a = 2, .b = 2,
+			.result = 2,
+		},
+		{
+			.a = 2, .b = 4,
+			.result = 2,
+		},
+		{
+			.a = 3, .b = 5,
+			.result = 1,
+		},
+		{
+			.a = 3*9, .b = 3*5,
+			.result = 3,
+		},
+		{
+			.a = 3*5*7, .b = 3*5*11,
+			.result = 15,
+		},
+		{
+			.a = (1 << 21) - 1,
+			.b = (1 << 22) - 1,
+			.result = 1,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
+		KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
+				    gcd(test_cases[i].a, test_cases[i].b),
+				    message_fmt, test_cases[i].a,
+				    test_cases[i].b);
+
+		/* gcd(a,b) == gcd(b,a) */
+		KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
+				    gcd(test_cases[i].b, test_cases[i].a),
+				    message_fmt, test_cases[i].b,
+				    test_cases[i].a);
+	}
+}
+
+static void lcm_test(struct kunit *test)
+{
+	const char *message_fmt = "lcm(%lu, %lu)";
+	int i;
+
+	struct test_case test_cases[] = {
+		{
+			.a = 0, .b = 1,
+			.result = 0,
+		},
+		{
+			.a = 1, .b = 2,
+			.result = 2,
+		},
+		{
+			.a = 2, .b = 2,
+			.result = 2,
+		},
+		{
+			.a = 3*5, .b = 3*7,
+			.result = 3*5*7,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
+		KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
+				    lcm(test_cases[i].a, test_cases[i].b),
+				    message_fmt, test_cases[i].a,
+				    test_cases[i].b);
+
+		/* lcm(a,b) == lcm(b,a) */
+		KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
+				    lcm(test_cases[i].b, test_cases[i].a),
+				    message_fmt, test_cases[i].b,
+				    test_cases[i].a);
+	}
+}
+
+static void int_sqrt_test(struct kunit *test)
+{
+	const char *message_fmt = "sqrt(%lu)";
+	int i;
+
+	struct test_case test_cases[] = {
+		{
+			.a = 0,
+			.result = 0,
+		},
+		{
+			.a = 1,
+			.result = 1,
+		},
+		{
+			.a = 4,
+			.result = 2,
+		},
+		{
+			.a = 5,
+			.result = 2,
+		},
+		{
+			.a = 8,
+			.result = 2,
+		},
+		{
+			.a = 1UL >> 32,
+			.result = 1UL >> 16,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
+		KUNIT_EXPECT_EQ_MSG(test, int_sqrt(test_cases[i].a),
+				    test_cases[i].result, message_fmt,
+				    test_cases[i].a);
+	}
+}
+
+struct reciprocal_test_case {
+	u32 a, b;
+	u32 result;
+};
+
+static void reciprocal_div_test(struct kunit *test)
+{
+	int i;
+	struct reciprocal_value rv;
+	struct reciprocal_test_case test_cases[] = {
+		{
+			.a = 0, .b = 1,
+			.result = 0,
+		},
+		{
+			.a = 42, .b = 20,
+			.result = 2,
+		},
+		{
+			.a = (1<<16), .b = (1<<14),
+			.result = 1<<2,
+		},
+	};
+
+	for (i = 0; i < ARRAY_SIZE(test_cases); ++i) {
+		rv = reciprocal_value(test_cases[i].b);
+		KUNIT_EXPECT_EQ_MSG(test, test_cases[i].result,
+				    reciprocal_divide(test_cases[i].a, rv),
+				    "reciprocal_divide(%u, %u)",
+				    test_cases[i].a, test_cases[i].b);
+	}
+}
+
+static struct kunit_case math_test_cases[] = {
+	KUNIT_CASE(gcd_test),
+	KUNIT_CASE(lcm_test),
+	KUNIT_CASE(int_sqrt_test),
+	KUNIT_CASE(reciprocal_div_test),
+	{}
+};
+
+static struct kunit_suite math_test_suite = {
+	.name = "lib-math",
+	.test_cases = math_test_cases,
+};
+
+kunit_test_suites(&math_test_suite);
+
+MODULE_LICENSE("GPL v2");