* Re: [PATCH] lib/math: Add int_pow test suite
2024-08-15 2:07 [PATCH] lib/math: Add int_pow test suite Luis Felipe Hernandez
@ 2024-08-16 6:07 ` David Gow
0 siblings, 0 replies; 2+ messages in thread
From: David Gow @ 2024-08-16 6:07 UTC (permalink / raw)
To: Luis Felipe Hernandez
Cc: brendan.higgins, rmoar, linux-kselftest, kunit-dev, skhan,
linux-kernel-mentees
[-- Attachment #1: Type: text/plain, Size: 5223 bytes --]
On Thu, 15 Aug 2024 at 10:07, Luis Felipe Hernandez
<luis.hernandez093@gmail.com> wrote:
>
> Adds test suite for integer based power function.
>
> Signed-off-by: Luis Felipe Hernandez <luis.hernandez093@gmail.com>
> ---
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/20240720181025.work.002-kees@kernel.org/
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
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread