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