| Message ID | 20201019224556.3536790-1-dlatypov@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | lib: add basic KUnit test for lib/math | expand |
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 <lkp@intel.com>
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/kbuild-all@lists.01.org
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. -- With Best Regards, Andy Shevchenko
On Tue, Oct 20, 2020 at 1:08 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 > >
On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <dlatypov@google.com> 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 <dlatypov@google.com> > --- > 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 <dlatypov@google.com> > + */ > + > +#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 >
On Tue, Oct 20, 2020 at 8:40 PM David Gow <davidgow@google.com> wrote: > > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <dlatypov@google.com> 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 <dlatypov@google.com> > > --- > > 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 <dlatypov@google.com> > > + */ > > + > > +#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 > >
On Wed, Oct 21, 2020 at 10:47:50AM -0700, Daniel Latypov wrote: > On Tue, Oct 20, 2020 at 8:40 PM David Gow <davidgow@google.com> wrote: > > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <dlatypov@google.com> 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. -- With Best Regards, Andy Shevchenko
On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 <davidgow@google.com> wrote: > > > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <dlatypov@google.com> 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 > >
On Thu, Oct 22, 2020 at 9:26 AM Daniel Latypov <dlatypov@google.com> wrote: > > On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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 <davidgow@google.com> wrote: > > > > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <dlatypov@google.com> 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.
On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 <davidgow@google.com> wrote: > > > On Tue, Oct 20, 2020 at 6:46 AM Daniel Latypov <dlatypov@google.com> 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.
On Thu, Oct 22, 2020 at 11:53:50AM -0700, Brendan Higgins wrote: > On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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.
On Thu, Oct 22, 2020 at 09:26:45AM -0700, Daniel Latypov wrote: > On Thu, Oct 22, 2020 at 8:06 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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) -- With Best Regards, Andy Shevchenko
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 > > <andriy.shevchenko@linux.intel.com> 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.
On Thu, Oct 22, 2020 at 12:04 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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 > > <andriy.shevchenko@linux.intel.com> 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 > >
On Thu, Oct 22, 2020 at 02:21:40PM -0700, Brendan Higgins wrote: > On Thu, Oct 22, 2020 at 12:04 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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 > > > <andriy.shevchenko@linux.intel.com> 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.
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 <lkp@intel.com>
"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/kbuild-all@lists.01.org
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 <lkp@intel.com>
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/kbuild-all@lists.01.org
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 <dlatypov@google.com> + */ + +#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");
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 <dlatypov@google.com> --- 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