lib/math: Add int_pow test suite

Commit Message

Luis Felipe Hernandez Aug. 15, 2024, 2:07 a.m. UTC

Adds test suite for integer based power function.

Signed-off-by: Luis Felipe Hernandez <[e-mail]>
---
 lib/math/Makefile       |  1 +
 lib/math/test_int_pow.c | 70 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)
 create mode 100644 lib/math/test_int_pow.c

Comments

David Gow Aug. 16, 2024, 6:07 a.m. UTC | #1

On Thu, 15 Aug 2024 at 10:07, Luis Felipe Hernandez
<[e-mail]> wrote:
>
> Adds test suite for integer based power function.
>
> Signed-off-by: Luis Felipe Hernandez <[e-mail]>
> ---

Hi Luis,

Thanks for your patch. Personally, I'm all in favour of adding it, but
there are a few issues which would need to be resolved first:

1. You'll need a Kconfig entry to enable the test -- as is, it's not
being built at all.
Somewhat confusingly, the Kconfig entries for lib/math tests are in
lib/Kconfig.debug
The following worked for me:
---
config TEST_INT_POW
       tristate "Integer exponentiation (int_pow) test" if !KUNIT_ALL_TESTS
       depends on KUNIT
       default KUNIT_ALL_TESTS
       help
         Enable this to test the int_pow maths function KUnit test.

         If unsure, say N
---

2. Once enabled, the test doesn't build. This is due to a typo:
'UNIT_EXPECT_EQ'.

3. Stylistically, there are a few things which are worth looking
closer at. I've left more detailed notes inline.

In general, you should make sure your tests run before sending them in, using:
./tools/testing/kunit/kunit.py run

>  lib/math/Makefile       |  1 +
>  lib/math/test_int_pow.c | 70 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
>  create mode 100644 lib/math/test_int_pow.c
>
> diff --git a/lib/math/Makefile b/lib/math/Makefile
> index 91fcdb0c9efe..ba564bf4fb00 100644
> --- a/lib/math/Makefile
> +++ b/lib/math/Makefile
> @@ -6,4 +6,5 @@ obj-$(CONFIG_PRIME_NUMBERS)     += prime_numbers.o
>  obj-$(CONFIG_RATIONAL)         += rational.o
>
>  obj-$(CONFIG_TEST_DIV64)       += test_div64.o
> +obj-$(CONFIG_TEST_INT_POW)     += test_int_pow.o

The KUnit style guide now recommends using <test>_KUNIT_TEST as a
config option, and putting the file in a tests subdirectory, and
naming it <int_pow>_kunit.c.

This is something we're obviously still cleaning up on existing tests,
but for new tests, I'd recommend CONFIG_INT_POW_TEST in Kconfig, and
moving the file to lib/tests or lib/math/tests, and name it
int_pow_kunit.c.

See https://lore.kernel.org/all/[e-mail]/
for some examples.

>  obj-$(CONFIG_RATIONAL_KUNIT_TEST) += rational-test.o
> diff --git a/lib/math/test_int_pow.c b/lib/math/test_int_pow.c
> new file mode 100644
> index 000000000000..ecabe71d01cc
> --- /dev/null
> +++ b/lib/math/test_int_pow.c
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <kunit/test.h>
> +#include <kunit/static_stub.h>

This test doesn't use static stubs, so you shouldn't need to include
kunit/static_stub.h
> +
> +#include <linux/math.h>
> +
> +
> +static void test_pow_0(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 1, int_pow(64, 0));
> +}
> +
> +static void test_pow_1(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 64, int_pow(64, 1));
> +}
> +
> +static void test_base_0(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 0, int_pow(0, 5));
> +}
> +
> +static void test_base_1(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 1, int_pow(1, 100));
> +       KUNIT_EXPECT_EQ(test, 1, int_pow(1, 0));
> +}
> +
> +static void test_base_0_pow_0(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 1, int_pow(0, 0));
> +}
> +
> +static void test_base_2_pow_2(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, 4, int_pow(2, 2));
> +}
> +
> +static void test_max_base(struct kunit *test)
> +{
> +       KUNIT_EXPECT_EQ(test, U64_MAX, int_pow(U64_MAX, 1));
> +}
> +
> +static void test_large_result(struct kunit *test)
> +{
> +       UNIT_EXPECT_EQ(test, 1152921504606846976, int_pow(2, 60));

This should be KUNIT_EXPECT_EQ, otherwise the test doesn't compile.

> +}



> +
> +static struct kunit_case math_int_pow_test_cases[] = {
> +       KUNIT_CASE(test_pow_0),
> +       KUNIT_CASE(test_pow_1),
> +       KUNIT_CASE(test_base_0),
> +       KUNIT_CASE(test_base_1),
> +       KUNIT_CASE(test_base_0_pow_0),
> +       KUNIT_CASE(test_base_2_pow_2),
> +       KUNIT_CASE(test_max_base),
> +       KUNIT_CASE(test_large_result),

Two notes:
- All of these tests are testing the same code, just with different
inputs. It may be easier / cleaner to use a parameterised test, as
lib/math/rational-test.c does.
- Is test_large_result the largest possible result int_pow can give?
If not, is it worth having a test case which is. Equally, I'd like to
see a nice, small real-world example with a base other than 1, and
non-power-of-two exponent. (e.g., 5^5 == 3125), as that'd be a useful
case to be able to debug.

> +       {}
> +};
> +
> +static struct kunit_suite int_pow_test_suite = {
> +       .name = "lib-math-int_pow",
> +       .test_cases = math_int_pow_test_cases,
> +};
> +
> +kunit_test_suites(&int_pow_test_suite);
> +
> +MODULE_DESCRIPTION("math.int_pow KUnit test suite");
> +MODULE_LICENSE("GPL v2");

This should just be "GPL", not "GPL v2". Running
./scripts/checkpatch.pl on the patch gives the warning:
WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db
("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
#100: FILE: lib/math/test_int_pow.c:70:

+MODULE_LICENSE("GPL v2");



> --
> 2.46.0
>

Thanks very much,
-- David

Patch

diff --git a/lib/math/Makefile b/lib/math/Makefile
index 91fcdb0c9efe..ba564bf4fb00 100644
--- a/lib/math/Makefile
+++ b/lib/math/Makefile
@@ -6,4 +6,5 @@  obj-$(CONFIG_PRIME_NUMBERS)	+= prime_numbers.o
 obj-$(CONFIG_RATIONAL)		+= rational.o
 
 obj-$(CONFIG_TEST_DIV64)	+= test_div64.o
+obj-$(CONFIG_TEST_INT_POW)	+= test_int_pow.o
 obj-$(CONFIG_RATIONAL_KUNIT_TEST) += rational-test.o
diff --git a/lib/math/test_int_pow.c b/lib/math/test_int_pow.c
new file mode 100644
index 000000000000..ecabe71d01cc
--- /dev/null
+++ b/lib/math/test_int_pow.c
@@ -0,0 +1,70 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/test.h>
+#include <kunit/static_stub.h>
+
+#include <linux/math.h>
+
+
+static void test_pow_0(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1, int_pow(64, 0));
+}
+
+static void test_pow_1(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 64, int_pow(64, 1));
+}
+
+static void test_base_0(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 0, int_pow(0, 5));
+}
+
+static void test_base_1(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1, int_pow(1, 100));
+	KUNIT_EXPECT_EQ(test, 1, int_pow(1, 0));
+}
+
+static void test_base_0_pow_0(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 1, int_pow(0, 0));
+}
+
+static void test_base_2_pow_2(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, 4, int_pow(2, 2));
+}
+
+static void test_max_base(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, U64_MAX, int_pow(U64_MAX, 1));
+}
+
+static void test_large_result(struct kunit *test)
+{
+	UNIT_EXPECT_EQ(test, 1152921504606846976, int_pow(2, 60));
+}
+
+static struct kunit_case math_int_pow_test_cases[] = {
+	KUNIT_CASE(test_pow_0),
+	KUNIT_CASE(test_pow_1),
+	KUNIT_CASE(test_base_0),
+	KUNIT_CASE(test_base_1),
+	KUNIT_CASE(test_base_0_pow_0),
+	KUNIT_CASE(test_base_2_pow_2),
+	KUNIT_CASE(test_max_base),
+	KUNIT_CASE(test_large_result),
+	{}
+};
+
+static struct kunit_suite int_pow_test_suite = {
+	.name = "lib-math-int_pow",
+	.test_cases = math_int_pow_test_cases,
+};
+
+kunit_test_suites(&int_pow_test_suite);
+
+MODULE_DESCRIPTION("math.int_pow KUnit test suite");
+MODULE_LICENSE("GPL v2");