| Message ID | 20241210172822.97683-1-luis.hernandez093@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] lib/math: Add int_sqrt test suite | expand |
On Tue, Dec 10, 2024 at 02:59:12PM -0500, Nicolas Pitre wrote: > This will fail on a 32-bit system where sizeof(long) == 32 and > ULONG_MAX == 4294967295. (meaning the result would be 65535). Thank you for taking the time to review my patch and for pointing out the issue with the ULONG_MAX test case. I failed to think about how a 32-bit system would treat ULONG_MAX == 4294967295. To address this, I was thinking of updating the test case to use a large enough value that remains within the bounds of unsigned long for both 32-bit and 64-bit architectures instead. Specifically, 2147483648 (2^31), which has an integer square root of 46340. I believe this would ensure the test remains valid and portable across all supported architectures. The updated test case would be as follows: { 2147483648, 46340, "large input"} I'd appreciate any feedback on this proposed change and thanks again for your time Nicolas. - Felipe
On Tue, 10 Dec 2024, Luis Felipe Hernandez wrote: > On Tue, Dec 10, 2024 at 02:59:12PM -0500, Nicolas Pitre wrote: > > This will fail on a 32-bit system where sizeof(long) == 32 and > > ULONG_MAX == 4294967295. (meaning the result would be 65535). > > Thank you for taking the time to review my patch and for pointing out the issue with the ULONG_MAX test case. I failed to think about how a 32-bit system would treat ULONG_MAX == 4294967295. > > To address this, I was thinking of updating the test case to use a large enough value that remains within the bounds of unsigned long for both 32-bit and 64-bit architectures instead. Specifically, 2147483648 (2^31), which has an integer square root of 46340. I believe this would ensure the test remains valid and portable across all supported architectures. > > The updated test case would be as follows: > { 2147483648, 46340, "large input"} Sure. And for such kind of test, more values to test is not a bad thing. So I'd suggest keeping { 4294967295, 65535 ] as well in the set as this represents a nice edge case. It wouldn't hurt adding the entire set from 0 to 9 as well. Many different edge cases in that range. Nicolas
On Tue, Dec 10, 2024 at 10:58:02PM -0500, Nicolas Pitre wrote: > And for such kind of test, more values to test is not a bad thing. So > I'd suggest keeping { 4294967295, 65535 ] as well in the set as this > represents a nice > edge case. > > It wouldn't hurt adding the entire set from 0 to 9 as well. Many > different edge cases in that range. I see, agreed, thank you for your suggestions. I'll go ahead and carry out the following changes: 1. Replace the ULONG_MAX test case with the aforementioned { 2147483648, 46340, "large input"} case 2. Add the { 4294967295, 65535 } edge case 3. Add missing cases between 0 - 9 4. Add a couple more cases The updated test cases would be as follows: { 0, 0, "edge case: square root of 0" }, { 1, 1, "perfect square: square root of 1" }, { 2, 1, "non-perfect square: square root of 2" }, { 3, 1, "non-perfect square: sqaure root of 3" }, { 4, 2, "perfect square: square root of 4" }, { 5, 2, "non-perfect square: square root of 5" }, { 6, 2, "non-perfect square: square root of 6" }, { 7, 2, "non-perfect square: square root of 7" }, { 8, 2, "non-perfect square: square root of 8" }, { 9, 3, "perfect square: square root of 9" }, { 16, 4, "perfect square: square root of 16" }, { 81, 9, "perfect square: square root of 81" }, { 256, 16, "perfect square: square root of 256" }, { 2147483648, 46340, "large input: square root of 2147483648" }, { 4294967295, 65535, "edge case: ULONG_MAX for 32-bit" }, I'll incorporate these changes and submit a new revision of this patch. Please let me know if there's anything else you'd like me to address. Thanks again for your guidance, Felipe
On Tue, 10 Dec 2024, Luis Felipe Hernandez wrote: > On Tue, Dec 10, 2024 at 10:58:02PM -0500, Nicolas Pitre wrote: > > > And for such kind of test, more values to test is not a bad thing. So > > I'd suggest keeping { 4294967295, 65535 ] as well in the set as this > > represents a nice > > edge case. > > > > It wouldn't hurt adding the entire set from 0 to 9 as well. Many > > different edge cases in that range. > > I see, agreed, thank you for your suggestions. I'll go ahead and carry > out the following changes: > 1. Replace the ULONG_MAX test case with the aforementioned { 2147483648, > 46340, "large input"} case > 2. Add the { 4294967295, 65535 } edge case > 3. Add missing cases between 0 - 9 > 4. Add a couple more cases > > The updated test cases would be as follows: > { 0, 0, "edge case: square root of 0" }, > { 1, 1, "perfect square: square root of 1" }, > { 2, 1, "non-perfect square: square root of 2" }, > { 3, 1, "non-perfect square: sqaure root of 3" }, > { 4, 2, "perfect square: square root of 4" }, > { 5, 2, "non-perfect square: square root of 5" }, > { 6, 2, "non-perfect square: square root of 6" }, > { 7, 2, "non-perfect square: square root of 7" }, > { 8, 2, "non-perfect square: square root of 8" }, > { 9, 3, "perfect square: square root of 9" }, > { 16, 4, "perfect square: square root of 16" }, > { 81, 9, "perfect square: square root of 81" }, > { 256, 16, "perfect square: square root of 256" }, > { 2147483648, 46340, "large input: square root of 2147483648" }, > { 4294967295, 65535, "edge case: ULONG_MAX for 32-bit" }, > > I'll incorporate these changes and submit a new revision of this patch. > Please let me know if there's anything else you'd like me to address. Looks fine to me. Nicolas
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index f3d723705879..cc00c4ace855 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -3161,6 +3161,21 @@ config INT_POW_TEST If unsure, say N +config INT_SQRT_KUNIT_TEST + tristate "Integer square root test test" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + help + This option enables the KUnit test suite for the int_sqrt() function, + which performs square root calculation. The test suite checks + various scenarios, including edge cases, to ensure correctness. + + Enabling this option will include tests that check various scenarios + and edge cases to ensure the accuracy and reliability of the square root + function. + + If unsure, say N + endif # RUNTIME_TESTING_MENU config ARCH_USE_MEMTEST diff --git a/lib/math/Makefile b/lib/math/Makefile index 3ef11305f8d2..25bcb968b369 100644 --- a/lib/math/Makefile +++ b/lib/math/Makefile @@ -9,3 +9,4 @@ obj-$(CONFIG_INT_POW_TEST) += tests/int_pow_kunit.o obj-$(CONFIG_TEST_DIV64) += test_div64.o obj-$(CONFIG_TEST_MULDIV64) += test_mul_u64_u64_div_u64.o obj-$(CONFIG_RATIONAL_KUNIT_TEST) += rational-test.o +obj-y += tests/ diff --git a/lib/math/tests/Makefile b/lib/math/tests/Makefile index 6a169123320a..e1a79f093b2d 100644 --- a/lib/math/tests/Makefile +++ b/lib/math/tests/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only obj-$(CONFIG_INT_POW_TEST) += int_pow_kunit.o +obj-$(CONFIG_INT_SQRT_KUNIT_TEST) += int_sqrt_kunit.o diff --git a/lib/math/tests/int_sqrt_kunit.c b/lib/math/tests/int_sqrt_kunit.c new file mode 100644 index 000000000000..3590142d2012 --- /dev/null +++ b/lib/math/tests/int_sqrt_kunit.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#include <kunit/test.h> +#include <linux/limits.h> +#include <linux/math.h> +#include <linux/module.h> +#include <linux/string.h> + +struct test_case_params { + unsigned long x; + unsigned long expected_result; + const char *name; +}; + +static const struct test_case_params params[] = { + { 0, 0, "edge-case: square root of 0" }, + { 4, 2, "perfect square: square root of 4" }, + { 81, 9, "perfect square: square root of 9" }, + { 2, 1, "non-perfect square: square root of 2" }, + { 5, 2, "non-perfect square: square root of 5"}, + { ULONG_MAX, 4294967295, "large input"}, +}; + +static void get_desc(const struct test_case_params *tc, char *desc) +{ + strscpy(desc, tc->name, KUNIT_PARAM_DESC_SIZE); +} + +KUNIT_ARRAY_PARAM(int_sqrt, params, get_desc); + +static void int_sqrt_test(struct kunit *test) +{ + const struct test_case_params *tc = (const struct test_case_params *)test->param_value; + + KUNIT_EXPECT_EQ(test, tc->expected_result, int_sqrt(tc->x)); +} + +static struct kunit_case math_int_sqrt_test_cases[] = { + KUNIT_CASE_PARAM(int_sqrt_test, int_sqrt_gen_params), + {} +}; + +static struct kunit_suite int_sqrt_test_suite = { + .name = "math-int_sqrt", + .test_cases = math_int_sqrt_test_cases, +}; + +kunit_test_suites(&int_sqrt_test_suite); + +MODULE_DESCRIPTION("math.int_sqrt KUnit test suite"); +MODULE_LICENSE("GPL");
Adds test suite for integer based square root function. The test suite is designed to verify the correctness of the int_sqrt() math library function. Signed-off-by: Luis Felipe Hernandez <luis.hernandez093@gmail.com> --- Changes in v2 - Add new line at the end of int_sqrt_kunit.c - Add explicit header includes for MODULE_* macros, strscpy, and ULONG_MAX Changes in v3 - Remove unnecesary new line after Kconfig entry for INT_SQRT_KUNIT_TEST - Correct int_sqrt instances with int_sqrt() in commit message and kconfig entry desc - Fix limits.h header include path Changes in v4 - Fix Kconfig entry: remove redundant word test --- lib/Kconfig.debug | 15 ++++++++++ lib/math/Makefile | 1 + lib/math/tests/Makefile | 1 + lib/math/tests/int_sqrt_kunit.c | 51 +++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+) create mode 100644 lib/math/tests/int_sqrt_kunit.c