Re: [PATCH v5] lib: add basic KUnit test for lib/math
On Tue, Apr 13, 2021 at 5:33 PM Daniel Latypov wrote:
>
> On Mon, Apr 12, 2021 at 11:41 PM David Gow wrote:
> >
> > On Tue, Apr 13, 2021 at 3:07 AM Daniel Latypov wrote:
> > >
> > > Add basic test coverage for files that don't require any config options:
> > > * part of math.h (what seem to be the most commonly used macros)
> > > * 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
> > > * provide short and simple examples of parameterized tests
> > > * provide a place to add tests for any new files in this dir
> > > * are written so adding new test cases to cover edge cases should be easy
> > > * looking at code coverage, we hit all the branches in the .c files
> > >
> > > Signed-off-by: Daniel Latypov
> >
> > This looks good to me. A few comments/observations below, but nothing
> > that I think should actually block this.
> >
> > Reviewed-by: David Gow
> >
> > -- David
> >
> > > ---
> > > Changes since v4:
> > > * add in test cases for some math.h macros (abs, round_up/round_down,
> > > div_round_down/closest)
> > > * use parameterized testing less to keep things terser
> > >
> > > Changes since v3:
> > > * fix `checkpatch.pl --strict` warnings
> > > * add test cases for gcd(0,0) and lcm(0,0)
> > > * minor: don't test both gcd(a,b) and gcd(b,a) when a == b
> > >
> > > Changes since v2: mv math_test.c => math_kunit.c
> > >
> > > Changes since v1:
> > > * Rebase and rewrite to use the new parameterized testing support.
> > > * misc: fix overflow in literal and inline int_sqrt format string.
> > > * related: commit 1f0e943df68a ("Documentation: kunit: provide guidance
> > > for testing many inputs") was merged explaining the patterns shown here.
> > > * there's an in-flight patch to update it for parameterized testing.
> > >
> > > v1:
> > > https://lore.kernel.org/lkml/20201019224556.3536790-1-dlaty...@google.com/
> > > ---
> > > lib/math/Kconfig | 5 +
> > > lib/math/Makefile | 2 +
> > > lib/math/math_kunit.c | 264 ++
> > > 3 files changed, 271 insertions(+)
> > > create mode 100644 lib/math/math_kunit.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
> >
> > This could have a description of the test and KUnit here, as mentioned
> > in the style guide doc:
> > https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
> >
> > (I think it's sufficiently self explanatory that it's not essential,
> > but it could be nice to have a more detailed description of the things
> > being tested than just "lib/math".)
> >
>
> Done. I've left off the details about what the test tests so we have
> less places to go and update if/when new tests are added.
>
> > > diff --git a/lib/math/Makefile b/lib/math/Makefile
> > > index be6909e943bd..30abb7a8d564 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_kunit.o
> > > diff --git a/lib/math/math_kunit.c b/lib/math/math_kunit.c
> > > new file mode 100644
> > > index ..80a087a32884
> > > --- /dev/null
> > > +++ b/lib/math/math_kunit.c
> > > @@ -0,0 +1,264 @@
> > > +// 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
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +static void abs_test(struct kunit *test)
> > > +{
> >
> > There's something weird about taking the absolute values of char
> > literals. I'm not sure if it's better to case integer literals (like
> > with 'short' below), or keep it as-is.
>
> I just thought it was amusing :)
> Converting it to be like the short test below.
>
> > > + KUNIT_EXPECT_EQ(test, abs('\0'), '\0');
> > > + KUNIT_EXPECT_EQ(test, abs('a'), 'a');
> > > + KUNIT_EXPECT_EQ(test, abs(-'a'), 'a');
> > > +
> > > + /* The expression in the macro is actually promoted to an int. */
> > > + KUNIT_EXPECT_EQ(test, abs((short)0), 0);
> > > + KUNIT_EXPECT_EQ(test, abs((short)42), 42);
> > > + KUNIT_EXPECT_EQ(test, abs((short)-42), 42);
> > > +
> >
Re: [PATCH v5] lib: add basic KUnit test for lib/math
On Mon, Apr 12, 2021 at 11:41 PM David Gow wrote:
>
> On Tue, Apr 13, 2021 at 3:07 AM Daniel Latypov wrote:
> >
> > Add basic test coverage for files that don't require any config options:
> > * part of math.h (what seem to be the most commonly used macros)
> > * 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
> > * provide short and simple examples of parameterized tests
> > * provide a place to add tests for any new files in this dir
> > * are written so adding new test cases to cover edge cases should be easy
> > * looking at code coverage, we hit all the branches in the .c files
> >
> > Signed-off-by: Daniel Latypov
>
> This looks good to me. A few comments/observations below, but nothing
> that I think should actually block this.
>
> Reviewed-by: David Gow
>
> -- David
>
> > ---
> > Changes since v4:
> > * add in test cases for some math.h macros (abs, round_up/round_down,
> > div_round_down/closest)
> > * use parameterized testing less to keep things terser
> >
> > Changes since v3:
> > * fix `checkpatch.pl --strict` warnings
> > * add test cases for gcd(0,0) and lcm(0,0)
> > * minor: don't test both gcd(a,b) and gcd(b,a) when a == b
> >
> > Changes since v2: mv math_test.c => math_kunit.c
> >
> > Changes since v1:
> > * Rebase and rewrite to use the new parameterized testing support.
> > * misc: fix overflow in literal and inline int_sqrt format string.
> > * related: commit 1f0e943df68a ("Documentation: kunit: provide guidance
> > for testing many inputs") was merged explaining the patterns shown here.
> > * there's an in-flight patch to update it for parameterized testing.
> >
> > v1:
> > https://lore.kernel.org/lkml/20201019224556.3536790-1-dlaty...@google.com/
> > ---
> > lib/math/Kconfig | 5 +
> > lib/math/Makefile | 2 +
> > lib/math/math_kunit.c | 264 ++
> > 3 files changed, 271 insertions(+)
> > create mode 100644 lib/math/math_kunit.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
>
> This could have a description of the test and KUnit here, as mentioned
> in the style guide doc:
> https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
>
> (I think it's sufficiently self explanatory that it's not essential,
> but it could be nice to have a more detailed description of the things
> being tested than just "lib/math".)
>
Done. I've left off the details about what the test tests so we have
less places to go and update if/when new tests are added.
> > diff --git a/lib/math/Makefile b/lib/math/Makefile
> > index be6909e943bd..30abb7a8d564 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_kunit.o
> > diff --git a/lib/math/math_kunit.c b/lib/math/math_kunit.c
> > new file mode 100644
> > index ..80a087a32884
> > --- /dev/null
> > +++ b/lib/math/math_kunit.c
> > @@ -0,0 +1,264 @@
> > +// 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
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +static void abs_test(struct kunit *test)
> > +{
>
> There's something weird about taking the absolute values of char
> literals. I'm not sure if it's better to case integer literals (like
> with 'short' below), or keep it as-is.
I just thought it was amusing :)
Converting it to be like the short test below.
> > + KUNIT_EXPECT_EQ(test, abs('\0'), '\0');
> > + KUNIT_EXPECT_EQ(test, abs('a'), 'a');
> > + KUNIT_EXPECT_EQ(test, abs(-'a'), 'a');
> > +
> > + /* The expression in the macro is actually promoted to an int. */
> > + KUNIT_EXPECT_EQ(test, abs((short)0), 0);
> > + KUNIT_EXPECT_EQ(test, abs((short)42), 42);
> > + KUNIT_EXPECT_EQ(test, abs((short)-42), 42);
> > +
> > + KUNIT_EXPECT_EQ(test, abs(0), 0);
> > + KUNIT_EXPECT_EQ(test, abs(42), 42);
> > + KUNIT_EXPECT_EQ(test, abs(-42), 42);
> > +
> > + KUNIT_EXPECT_EQ(test, abs(0L), 0L);
> > + KUNIT_EXPECT_EQ(test, abs(42L), 42L);
> > + KUNIT_EXPECT_EQ(test, abs(-42L), 42L);
> > +
> > +
Re: [PATCH v5] lib: add basic KUnit test for lib/math
On Tue, Apr 13, 2021 at 3:07 AM Daniel Latypov wrote:
>
> Add basic test coverage for files that don't require any config options:
> * part of math.h (what seem to be the most commonly used macros)
> * 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
> * provide short and simple examples of parameterized tests
> * provide a place to add tests for any new files in this dir
> * are written so adding new test cases to cover edge cases should be easy
> * looking at code coverage, we hit all the branches in the .c files
>
> Signed-off-by: Daniel Latypov
This looks good to me. A few comments/observations below, but nothing
that I think should actually block this.
Reviewed-by: David Gow
-- David
> ---
> Changes since v4:
> * add in test cases for some math.h macros (abs, round_up/round_down,
> div_round_down/closest)
> * use parameterized testing less to keep things terser
>
> Changes since v3:
> * fix `checkpatch.pl --strict` warnings
> * add test cases for gcd(0,0) and lcm(0,0)
> * minor: don't test both gcd(a,b) and gcd(b,a) when a == b
>
> Changes since v2: mv math_test.c => math_kunit.c
>
> Changes since v1:
> * Rebase and rewrite to use the new parameterized testing support.
> * misc: fix overflow in literal and inline int_sqrt format string.
> * related: commit 1f0e943df68a ("Documentation: kunit: provide guidance
> for testing many inputs") was merged explaining the patterns shown here.
> * there's an in-flight patch to update it for parameterized testing.
>
> v1: https://lore.kernel.org/lkml/20201019224556.3536790-1-dlaty...@google.com/
> ---
> lib/math/Kconfig | 5 +
> lib/math/Makefile | 2 +
> lib/math/math_kunit.c | 264 ++
> 3 files changed, 271 insertions(+)
> create mode 100644 lib/math/math_kunit.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
This could have a description of the test and KUnit here, as mentioned
in the style guide doc:
https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries
(I think it's sufficiently self explanatory that it's not essential,
but it could be nice to have a more detailed description of the things
being tested than just "lib/math".)
> diff --git a/lib/math/Makefile b/lib/math/Makefile
> index be6909e943bd..30abb7a8d564 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_kunit.o
> diff --git a/lib/math/math_kunit.c b/lib/math/math_kunit.c
> new file mode 100644
> index ..80a087a32884
> --- /dev/null
> +++ b/lib/math/math_kunit.c
> @@ -0,0 +1,264 @@
> +// 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
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static void abs_test(struct kunit *test)
> +{
There's something weird about taking the absolute values of char
literals. I'm not sure if it's better to case integer literals (like
with 'short' below), or keep it as-is.
> + KUNIT_EXPECT_EQ(test, abs('\0'), '\0');
> + KUNIT_EXPECT_EQ(test, abs('a'), 'a');
> + KUNIT_EXPECT_EQ(test, abs(-'a'), 'a');
> +
> + /* The expression in the macro is actually promoted to an int. */
> + KUNIT_EXPECT_EQ(test, abs((short)0), 0);
> + KUNIT_EXPECT_EQ(test, abs((short)42), 42);
> + KUNIT_EXPECT_EQ(test, abs((short)-42), 42);
> +
> + KUNIT_EXPECT_EQ(test, abs(0), 0);
> + KUNIT_EXPECT_EQ(test, abs(42), 42);
> + KUNIT_EXPECT_EQ(test, abs(-42), 42);
> +
> + KUNIT_EXPECT_EQ(test, abs(0L), 0L);
> + KUNIT_EXPECT_EQ(test, abs(42L), 42L);
> + KUNIT_EXPECT_EQ(test, abs(-42L), 42L);
> +
> + KUNIT_EXPECT_EQ(test, abs(0LL), 0LL);
> + KUNIT_EXPECT_EQ(test, abs(42LL), 42LL);
> + KUNIT_EXPECT_EQ(test, abs(-42LL), 42LL);
> +
> + /* Unsigned types get casted to signed. */
> + KUNIT_EXPECT_EQ(test, abs(0ULL), 0LL);
> + KUNIT_EXPECT_EQ(test, abs(42ULL), 42LL);
A part of me is curious what the result is for -0x8000, but I
guess that's not defined, so shouldn't be tested. :-)
> +}
> +
> +static void int_sqrt_test(struct kunit *test)
> +{
> + KUNIT_EXPECT_EQ(test,
