* [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing @ 2020-10-23 15:05 Arpitha Raghunandan 2020-10-23 15:06 ` [Linux-kernel-mentees] [PATCH v2 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature Arpitha Raghunandan 2020-10-23 18:48 ` [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing Marco Elver via Linux-kernel-mentees 0 siblings, 2 replies; 7+ messages in thread From: Arpitha Raghunandan @ 2020-10-23 15:05 UTC (permalink / raw) To: brendanhiggins, skhan, elver, yzaikin, tytso, adilger.kernel Cc: Arpitha Raghunandan, linux-kernel, linux-kselftest, linux-ext4, linux-kernel-mentees, kunit-dev Implementation of support for parameterized testing in KUnit. Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com> --- Changes v1->v2: - Use of a generator method to access test case parameters include/kunit/test.h | 45 ++++++++++++++++++++++++++++++++++++++++++++ lib/kunit/test.c | 20 +++++++++++++++++++- 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/include/kunit/test.h b/include/kunit/test.h index a423fffefea0..c417ac140326 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -141,6 +141,7 @@ struct kunit; struct kunit_case { void (*run_case)(struct kunit *test); const char *name; + void* (*generate_params)(struct kunit *test, void *prev); /* private: internal use only. */ bool success; @@ -162,6 +163,9 @@ static inline char *kunit_status_to_string(bool status) * &struct kunit_case for an example on how to use it. */ #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } +#define KUNIT_CASE_PARAM(test_name, gen_params) \ + { .run_case = test_name, .name = #test_name, \ + .generate_params = gen_params } /** * struct kunit_suite - describes a related collection of &struct kunit_case @@ -208,6 +212,15 @@ struct kunit { const char *name; /* Read only after initialization! */ char *log; /* Points at case log after initialization */ struct kunit_try_catch try_catch; + /* param_values points to test case parameters in parameterized tests */ + void *param_values; + /* + * current_param stores the index of the parameter in + * the array of parameters in parameterized tests. + * current_param + 1 is printed to indicate the parameter + * that causes the test to fail in case of test failure. + */ + int current_param; /* * success starts as true, and may only be set to false during a * test case; thus, it is safe to update this across multiple @@ -1742,4 +1755,36 @@ do { \ fmt, \ ##__VA_ARGS__) +/** + * kunit_param_generator_helper() - Helper method for test parameter generators + * required in parameterized tests. + * @test: The test context object. + * @prev_param: a pointer to the previous test parameter, NULL for first parameter. + * @param_array: a user-supplied pointer to an array of test parameters. + * @array_size: number of test parameters in the array. + * @type_size: size of one test parameter. + */ +static inline void *kunit_param_generator_helper(struct kunit *test, + void *prev_param, + void *param_array, + size_t array_size, + size_t type_size) +{ + KUNIT_ASSERT_EQ(test, (prev_param - param_array) % type_size, 0); + + if (!prev_param) + return param_array; + + KUNIT_ASSERT_GE(test, prev_param, param_array); + + if (prev_param + type_size < param_array + (array_size * type_size)) + return prev_param + type_size; + else + return NULL; +} + +#define KUNIT_PARAM_GENERATOR_HELPER(test, prev_param, param_array, param_type) \ + kunit_param_generator_helper(test, prev_param, param_array, \ + ARRAY_SIZE(param_array), sizeof(param_type)) + #endif /* _KUNIT_TEST_H */ diff --git a/lib/kunit/test.c b/lib/kunit/test.c index 750704abe89a..0e6ffe6384a7 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -127,6 +127,11 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite, } EXPORT_SYMBOL_GPL(kunit_test_case_num); +static void kunit_print_failed_param(struct kunit *test) +{ + kunit_err(test, "\n\tTest failed at parameter: %d\n", test->current_param + 1); +} + static void kunit_print_string_stream(struct kunit *test, struct string_stream *stream) { @@ -168,6 +173,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) assert->format(assert, stream); kunit_print_string_stream(test, stream); + if (test->param_values) + kunit_print_failed_param(test); WARN_ON(string_stream_destroy(stream)); } @@ -239,7 +246,18 @@ static void kunit_run_case_internal(struct kunit *test, } } - test_case->run_case(test); + if (!test_case->generate_params) { + test_case->run_case(test); + } else { + test->param_values = test_case->generate_params(test, NULL); + test->current_param = 0; + + while (test->param_values) { + test_case->run_case(test); + test->param_values = test_case->generate_params(test, test->param_values); + test->current_param++; + } + } } static void kunit_case_internal_cleanup(struct kunit *test) -- 2.25.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Linux-kernel-mentees] [PATCH v2 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature 2020-10-23 15:05 [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing Arpitha Raghunandan @ 2020-10-23 15:06 ` Arpitha Raghunandan 2020-10-26 18:11 ` Iurii Zaikin via Linux-kernel-mentees 2020-10-23 18:48 ` [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing Marco Elver via Linux-kernel-mentees 1 sibling, 1 reply; 7+ messages in thread From: Arpitha Raghunandan @ 2020-10-23 15:06 UTC (permalink / raw) To: brendanhiggins, skhan, elver, yzaikin, tytso, adilger.kernel Cc: Arpitha Raghunandan, linux-kernel, linux-kselftest, linux-ext4, linux-kernel-mentees, kunit-dev Modify fs/ext4/inode-test.c to use the parameterized testing feature of KUnit. Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com> --- Changes v1->v2: - Modification based on latest implementation of KUnit parameterized testing fs/ext4/inode-test.c | 318 ++++++++++++++++++++++--------------------- 1 file changed, 162 insertions(+), 156 deletions(-) diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c index d62d802c9c12..611a1cf2581d 100644 --- a/fs/ext4/inode-test.c +++ b/fs/ext4/inode-test.c @@ -80,6 +80,137 @@ struct timestamp_expectation { bool lower_bound; }; +static struct timestamp_expectation test_data[] = { + { + .test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE, + .msb_set = true, + .lower_bound = true, + .extra_bits = 0, + .expected = {.tv_sec = -0x80000000LL, .tv_nsec = 0L}, + }, + + { + .test_case_name = UPPER_BOUND_NEG_NO_EXTRA_BITS_CASE, + .msb_set = true, + .lower_bound = false, + .extra_bits = 0, + .expected = {.tv_sec = -1LL, .tv_nsec = 0L}, + }, + + { + .test_case_name = LOWER_BOUND_NONNEG_NO_EXTRA_BITS_CASE, + .msb_set = false, + .lower_bound = true, + .extra_bits = 0, + .expected = {0LL, 0L}, + }, + + { + .test_case_name = UPPER_BOUND_NONNEG_NO_EXTRA_BITS_CASE, + .msb_set = false, + .lower_bound = false, + .extra_bits = 0, + .expected = {.tv_sec = 0x7fffffffLL, .tv_nsec = 0L}, + }, + + { + .test_case_name = LOWER_BOUND_NEG_LO_1_CASE, + .msb_set = true, + .lower_bound = true, + .extra_bits = 1, + .expected = {.tv_sec = 0x80000000LL, .tv_nsec = 0L}, + }, + + { + .test_case_name = UPPER_BOUND_NEG_LO_1_CASE, + .msb_set = true, + .lower_bound = false, + .extra_bits = 1, + .expected = {.tv_sec = 0xffffffffLL, .tv_nsec = 0L}, + }, + + { + .test_case_name = LOWER_BOUND_NONNEG_LO_1_CASE, + .msb_set = false, + .lower_bound = true, + .extra_bits = 1, + .expected = {.tv_sec = 0x100000000LL, .tv_nsec = 0L}, + }, + + { + .test_case_name = UPPER_BOUND_NONNEG_LO_1_CASE, + .msb_set = false, + .lower_bound = false, + .extra_bits = 1, + .expected = {.tv_sec = 0x17fffffffLL, .tv_nsec = 0L}, + }, + + { + .test_case_name = LOWER_BOUND_NEG_HI_1_CASE, + .msb_set = true, + .lower_bound = true, + .extra_bits = 2, + .expected = {.tv_sec = 0x180000000LL, .tv_nsec = 0L}, + }, + + { + .test_case_name = UPPER_BOUND_NEG_HI_1_CASE, + .msb_set = true, + .lower_bound = false, + .extra_bits = 2, + .expected = {.tv_sec = 0x1ffffffffLL, .tv_nsec = 0L}, + }, + + { + .test_case_name = LOWER_BOUND_NONNEG_HI_1_CASE, + .msb_set = false, + .lower_bound = true, + .extra_bits = 2, + .expected = {.tv_sec = 0x200000000LL, .tv_nsec = 0L}, + }, + + { + .test_case_name = UPPER_BOUND_NONNEG_HI_1_CASE, + .msb_set = false, + .lower_bound = false, + .extra_bits = 2, + .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 0L}, + }, + + { + .test_case_name = UPPER_BOUND_NONNEG_HI_1_NS_1_CASE, + .msb_set = false, + .lower_bound = false, + .extra_bits = 6, + .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 1L}, + }, + + { + .test_case_name = LOWER_BOUND_NONNEG_HI_1_NS_MAX_CASE, + .msb_set = false, + .lower_bound = true, + .extra_bits = 0xFFFFFFFF, + .expected = {.tv_sec = 0x300000000LL, + .tv_nsec = MAX_NANOSECONDS}, + }, + + { + .test_case_name = LOWER_BOUND_NONNEG_EXTRA_BITS_1_CASE, + .msb_set = false, + .lower_bound = true, + .extra_bits = 3, + .expected = {.tv_sec = 0x300000000LL, .tv_nsec = 0L}, + }, + + { + .test_case_name = UPPER_BOUND_NONNEG_EXTRA_BITS_1_CASE, + .msb_set = false, + .lower_bound = false, + .extra_bits = 3, + .expected = {.tv_sec = 0x37fffffffLL, .tv_nsec = 0L}, + } +}; + static time64_t get_32bit_time(const struct timestamp_expectation * const test) { if (test->msb_set) { @@ -101,166 +232,41 @@ static time64_t get_32bit_time(const struct timestamp_expectation * const test) */ static void inode_test_xtimestamp_decoding(struct kunit *test) { - const struct timestamp_expectation test_data[] = { - { - .test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE, - .msb_set = true, - .lower_bound = true, - .extra_bits = 0, - .expected = {.tv_sec = -0x80000000LL, .tv_nsec = 0L}, - }, - - { - .test_case_name = UPPER_BOUND_NEG_NO_EXTRA_BITS_CASE, - .msb_set = true, - .lower_bound = false, - .extra_bits = 0, - .expected = {.tv_sec = -1LL, .tv_nsec = 0L}, - }, - - { - .test_case_name = LOWER_BOUND_NONNEG_NO_EXTRA_BITS_CASE, - .msb_set = false, - .lower_bound = true, - .extra_bits = 0, - .expected = {0LL, 0L}, - }, - - { - .test_case_name = UPPER_BOUND_NONNEG_NO_EXTRA_BITS_CASE, - .msb_set = false, - .lower_bound = false, - .extra_bits = 0, - .expected = {.tv_sec = 0x7fffffffLL, .tv_nsec = 0L}, - }, - - { - .test_case_name = LOWER_BOUND_NEG_LO_1_CASE, - .msb_set = true, - .lower_bound = true, - .extra_bits = 1, - .expected = {.tv_sec = 0x80000000LL, .tv_nsec = 0L}, - }, - - { - .test_case_name = UPPER_BOUND_NEG_LO_1_CASE, - .msb_set = true, - .lower_bound = false, - .extra_bits = 1, - .expected = {.tv_sec = 0xffffffffLL, .tv_nsec = 0L}, - }, - - { - .test_case_name = LOWER_BOUND_NONNEG_LO_1_CASE, - .msb_set = false, - .lower_bound = true, - .extra_bits = 1, - .expected = {.tv_sec = 0x100000000LL, .tv_nsec = 0L}, - }, - - { - .test_case_name = UPPER_BOUND_NONNEG_LO_1_CASE, - .msb_set = false, - .lower_bound = false, - .extra_bits = 1, - .expected = {.tv_sec = 0x17fffffffLL, .tv_nsec = 0L}, - }, - - { - .test_case_name = LOWER_BOUND_NEG_HI_1_CASE, - .msb_set = true, - .lower_bound = true, - .extra_bits = 2, - .expected = {.tv_sec = 0x180000000LL, .tv_nsec = 0L}, - }, - - { - .test_case_name = UPPER_BOUND_NEG_HI_1_CASE, - .msb_set = true, - .lower_bound = false, - .extra_bits = 2, - .expected = {.tv_sec = 0x1ffffffffLL, .tv_nsec = 0L}, - }, - - { - .test_case_name = LOWER_BOUND_NONNEG_HI_1_CASE, - .msb_set = false, - .lower_bound = true, - .extra_bits = 2, - .expected = {.tv_sec = 0x200000000LL, .tv_nsec = 0L}, - }, - - { - .test_case_name = UPPER_BOUND_NONNEG_HI_1_CASE, - .msb_set = false, - .lower_bound = false, - .extra_bits = 2, - .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 0L}, - }, - - { - .test_case_name = UPPER_BOUND_NONNEG_HI_1_NS_1_CASE, - .msb_set = false, - .lower_bound = false, - .extra_bits = 6, - .expected = {.tv_sec = 0x27fffffffLL, .tv_nsec = 1L}, - }, - - { - .test_case_name = LOWER_BOUND_NONNEG_HI_1_NS_MAX_CASE, - .msb_set = false, - .lower_bound = true, - .extra_bits = 0xFFFFFFFF, - .expected = {.tv_sec = 0x300000000LL, - .tv_nsec = MAX_NANOSECONDS}, - }, - - { - .test_case_name = LOWER_BOUND_NONNEG_EXTRA_BITS_1_CASE, - .msb_set = false, - .lower_bound = true, - .extra_bits = 3, - .expected = {.tv_sec = 0x300000000LL, .tv_nsec = 0L}, - }, - - { - .test_case_name = UPPER_BOUND_NONNEG_EXTRA_BITS_1_CASE, - .msb_set = false, - .lower_bound = false, - .extra_bits = 3, - .expected = {.tv_sec = 0x37fffffffLL, .tv_nsec = 0L}, - } - }; - struct timespec64 timestamp; - int i; - - for (i = 0; i < ARRAY_SIZE(test_data); ++i) { - timestamp.tv_sec = get_32bit_time(&test_data[i]); - ext4_decode_extra_time(×tamp, - cpu_to_le32(test_data[i].extra_bits)); - - KUNIT_EXPECT_EQ_MSG(test, - test_data[i].expected.tv_sec, - timestamp.tv_sec, - CASE_NAME_FORMAT, - test_data[i].test_case_name, - test_data[i].msb_set, - test_data[i].lower_bound, - test_data[i].extra_bits); - KUNIT_EXPECT_EQ_MSG(test, - test_data[i].expected.tv_nsec, - timestamp.tv_nsec, - CASE_NAME_FORMAT, - test_data[i].test_case_name, - test_data[i].msb_set, - test_data[i].lower_bound, - test_data[i].extra_bits); - } + + struct timestamp_expectation *test_param = + (struct timestamp_expectation *)(test->param_values); + + timestamp.tv_sec = get_32bit_time(test_param); + ext4_decode_extra_time(×tamp, + cpu_to_le32(test_param->extra_bits)); + + KUNIT_EXPECT_EQ_MSG(test, + test_param->expected.tv_sec, + timestamp.tv_sec, + CASE_NAME_FORMAT, + test_param->test_case_name, + test_param->msb_set, + test_param->lower_bound, + test_param->extra_bits); + KUNIT_EXPECT_EQ_MSG(test, + test_param->expected.tv_nsec, + timestamp.tv_nsec, + CASE_NAME_FORMAT, + test_param->test_case_name, + test_param->msb_set, + test_param->lower_bound, + test_param->extra_bits); +} + +static void *generate_params(struct kunit *test, void *prev) +{ + return KUNIT_PARAM_GENERATOR_HELPER(test, prev, test_data, + struct timestamp_expectation); } static struct kunit_case ext4_inode_test_cases[] = { - KUNIT_CASE(inode_test_xtimestamp_decoding), + KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding, generate_params), {} }; -- 2.25.1 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature 2020-10-23 15:06 ` [Linux-kernel-mentees] [PATCH v2 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature Arpitha Raghunandan @ 2020-10-26 18:11 ` Iurii Zaikin via Linux-kernel-mentees 2020-10-26 18:19 ` Arpitha Raghunandan 0 siblings, 1 reply; 7+ messages in thread From: Iurii Zaikin via Linux-kernel-mentees @ 2020-10-26 18:11 UTC (permalink / raw) To: Arpitha Raghunandan Cc: Marco Elver, Theodore Ts'o, Brendan Higgins, Linux Kernel Mailing List, Andreas Dilger, open list:KERNEL SELFTEST FRAMEWORK, Ext4 Developers List, linux-kernel-mentees, KUnit Development > +static struct timestamp_expectation test_data[] = { Can you mark this and the rest of the hardcoded values as the const they are? _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature 2020-10-26 18:11 ` Iurii Zaikin via Linux-kernel-mentees @ 2020-10-26 18:19 ` Arpitha Raghunandan 0 siblings, 0 replies; 7+ messages in thread From: Arpitha Raghunandan @ 2020-10-26 18:19 UTC (permalink / raw) To: Iurii Zaikin Cc: Marco Elver, Theodore Ts'o, Brendan Higgins, Linux Kernel Mailing List, Andreas Dilger, open list:KERNEL SELFTEST FRAMEWORK, Ext4 Developers List, linux-kernel-mentees, KUnit Development On 26/10/20 11:41 pm, Iurii Zaikin wrote: >> +static struct timestamp_expectation test_data[] = { > Can you mark this and the rest of the hardcoded values as the const they are? > Sure, I will make this change for the next version. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing 2020-10-23 15:05 [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing Arpitha Raghunandan 2020-10-23 15:06 ` [Linux-kernel-mentees] [PATCH v2 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature Arpitha Raghunandan @ 2020-10-23 18:48 ` Marco Elver via Linux-kernel-mentees 2020-10-23 18:56 ` Marco Elver via Linux-kernel-mentees 2020-10-24 5:23 ` Arpitha Raghunandan 1 sibling, 2 replies; 7+ messages in thread From: Marco Elver via Linux-kernel-mentees @ 2020-10-23 18:48 UTC (permalink / raw) To: Arpitha Raghunandan Cc: linux-ext4, tytso, brendanhiggins, linux-kernel, adilger.kernel, linux-kselftest, yzaikin, linux-kernel-mentees, kunit-dev On Fri, Oct 23, 2020 at 08:35PM +0530, Arpitha Raghunandan wrote: > Implementation of support for parameterized testing in KUnit. Already looks much cleaner, thanks for using this approach! I think the commit message needs a brief summary of the approach. > Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com> > --- > Changes v1->v2: > - Use of a generator method to access test case parameters > > include/kunit/test.h | 45 ++++++++++++++++++++++++++++++++++++++++++++ > lib/kunit/test.c | 20 +++++++++++++++++++- > 2 files changed, 64 insertions(+), 1 deletion(-) > > diff --git a/include/kunit/test.h b/include/kunit/test.h > index a423fffefea0..c417ac140326 100644 > --- a/include/kunit/test.h > +++ b/include/kunit/test.h > @@ -141,6 +141,7 @@ struct kunit; > struct kunit_case { > void (*run_case)(struct kunit *test); > const char *name; > + void* (*generate_params)(struct kunit *test, void *prev); Would adding documentation above this field be the right place, or somewhere else? In any case, some explanation of the protocol would be good. > /* private: internal use only. */ > bool success; > @@ -162,6 +163,9 @@ static inline char *kunit_status_to_string(bool status) > * &struct kunit_case for an example on how to use it. > */ > #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } > +#define KUNIT_CASE_PARAM(test_name, gen_params) \ > + { .run_case = test_name, .name = #test_name, \ > + .generate_params = gen_params } > > /** > * struct kunit_suite - describes a related collection of &struct kunit_case > @@ -208,6 +212,15 @@ struct kunit { > const char *name; /* Read only after initialization! */ > char *log; /* Points at case log after initialization */ > struct kunit_try_catch try_catch; > + /* param_values points to test case parameters in parameterized tests */ > + void *param_values; > + /* > + * current_param stores the index of the parameter in > + * the array of parameters in parameterized tests. > + * current_param + 1 is printed to indicate the parameter > + * that causes the test to fail in case of test failure. > + */ > + int current_param; > /* > * success starts as true, and may only be set to false during a > * test case; thus, it is safe to update this across multiple > @@ -1742,4 +1755,36 @@ do { \ > fmt, \ > ##__VA_ARGS__) > > +/** > + * kunit_param_generator_helper() - Helper method for test parameter generators > + * required in parameterized tests. > + * @test: The test context object. > + * @prev_param: a pointer to the previous test parameter, NULL for first parameter. > + * @param_array: a user-supplied pointer to an array of test parameters. > + * @array_size: number of test parameters in the array. > + * @type_size: size of one test parameter. > + */ > +static inline void *kunit_param_generator_helper(struct kunit *test, I don't think this needs to be inline, but see my other suggestion below, which might make this function obsolete. > + void *prev_param, > + void *param_array, > + size_t array_size, > + size_t type_size) > +{ > + KUNIT_ASSERT_EQ(test, (prev_param - param_array) % type_size, 0); > + > + if (!prev_param) > + return param_array; > + > + KUNIT_ASSERT_GE(test, prev_param, param_array); > + > + if (prev_param + type_size < param_array + (array_size * type_size)) > + return prev_param + type_size; > + else > + return NULL; > +} > + > +#define KUNIT_PARAM_GENERATOR_HELPER(test, prev_param, param_array, param_type) \ > + kunit_param_generator_helper(test, prev_param, param_array, \ > + ARRAY_SIZE(param_array), sizeof(param_type)) You do not need param_type, you can use the same trick that ARRAY_SIZE uses: #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) So you could use sizeof((param_aray)[0]) instead of sizeof(param_type). ARRAY_SIZE already checks for you that it's a real array via __must_be_array(). The other question is, will kunit_param_generator_helper() find much use without the KUNIT_PARAM_GENERATOR_HELPER() macro? If I have some complicated generator protocol to generate params, then I'd just directly write the generator function. If your intent is to simplify the common-case array based generators, why not just have a macro generate the generator function? More specifically, have this macro here: +#define KUNIT_ARRAY_PARAM(name, array) \ + static void *name##_gen_params(struct kunit *test, void *prev) \ + { \ + typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \ + return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL; \ + } [ It is entirely untested, but if it works verbatim you'll probably need my Co-developed-by: Marco Elver <elver@google.com> Signed-off-by: Marco Elver <elver@google.com> just in case... ] Then, it can be used as follows: static int num_cpus[] = {1, 2, 3, 4, 5}; KUNIT_ARRAY_PARAM(num_cpus, num_cpus); Then somewhere else: KUNIT_CASE_PARAM(some_test, num_cpus_gen_params); > #endif /* _KUNIT_TEST_H */ > diff --git a/lib/kunit/test.c b/lib/kunit/test.c > index 750704abe89a..0e6ffe6384a7 100644 > --- a/lib/kunit/test.c > +++ b/lib/kunit/test.c > @@ -127,6 +127,11 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite, > } > EXPORT_SYMBOL_GPL(kunit_test_case_num); > > +static void kunit_print_failed_param(struct kunit *test) > +{ > + kunit_err(test, "\n\tTest failed at parameter: %d\n", test->current_param + 1); > +} > + > static void kunit_print_string_stream(struct kunit *test, > struct string_stream *stream) > { > @@ -168,6 +173,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) > assert->format(assert, stream); > > kunit_print_string_stream(test, stream); > + if (test->param_values) > + kunit_print_failed_param(test); > > WARN_ON(string_stream_destroy(stream)); > } > @@ -239,7 +246,18 @@ static void kunit_run_case_internal(struct kunit *test, > } > } > > - test_case->run_case(test); > + if (!test_case->generate_params) { > + test_case->run_case(test); > + } else { > + test->param_values = test_case->generate_params(test, NULL); > + test->current_param = 0; > + > + while (test->param_values) { > + test_case->run_case(test); > + test->param_values = test_case->generate_params(test, test->param_values); > + test->current_param++; > + } > + } > } > > static void kunit_case_internal_cleanup(struct kunit *test) Otherwise looks fine. Thanks, -- Marco _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing 2020-10-23 18:48 ` [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing Marco Elver via Linux-kernel-mentees @ 2020-10-23 18:56 ` Marco Elver via Linux-kernel-mentees 2020-10-24 5:23 ` Arpitha Raghunandan 1 sibling, 0 replies; 7+ messages in thread From: Marco Elver via Linux-kernel-mentees @ 2020-10-23 18:56 UTC (permalink / raw) To: Arpitha Raghunandan Cc: linux-ext4, Theodore Ts'o, Brendan Higgins, LKML, Andreas Dilger, open list:KERNEL SELFTEST FRAMEWORK, Iurii Zaikin, linux-kernel-mentees, KUnit Development On Fri, 23 Oct 2020 at 20:48, Marco Elver <elver@google.com> wrote: [...] > > + */ > > +static inline void *kunit_param_generator_helper(struct kunit *test, > > I don't think this needs to be inline, but see my other suggestion > below, which might make this function obsolete. Ah sorry, it's in a header so we might get complaints if it's not inline. But in any case, if you use the KUNIT_ARRAY_PARAM() macro I proposed, this function will become obsolete. Thanks, -- Marco _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing 2020-10-23 18:48 ` [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing Marco Elver via Linux-kernel-mentees 2020-10-23 18:56 ` Marco Elver via Linux-kernel-mentees @ 2020-10-24 5:23 ` Arpitha Raghunandan 1 sibling, 0 replies; 7+ messages in thread From: Arpitha Raghunandan @ 2020-10-24 5:23 UTC (permalink / raw) To: Marco Elver Cc: linux-ext4, tytso, brendanhiggins, linux-kernel, adilger.kernel, linux-kselftest, yzaikin, linux-kernel-mentees, kunit-dev On 24/10/20 12:18 am, Marco Elver wrote: > On Fri, Oct 23, 2020 at 08:35PM +0530, Arpitha Raghunandan wrote: >> Implementation of support for parameterized testing in KUnit. > > Already looks much cleaner, thanks for using this approach! > > I think the commit message needs a brief summary of the approach. > Okay, I will add a more detailed commit message for the next version. >> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com> >> --- >> Changes v1->v2: >> - Use of a generator method to access test case parameters >> >> include/kunit/test.h | 45 ++++++++++++++++++++++++++++++++++++++++++++ >> lib/kunit/test.c | 20 +++++++++++++++++++- >> 2 files changed, 64 insertions(+), 1 deletion(-) >> >> diff --git a/include/kunit/test.h b/include/kunit/test.h >> index a423fffefea0..c417ac140326 100644 >> --- a/include/kunit/test.h >> +++ b/include/kunit/test.h >> @@ -141,6 +141,7 @@ struct kunit; >> struct kunit_case { >> void (*run_case)(struct kunit *test); >> const char *name; >> + void* (*generate_params)(struct kunit *test, void *prev); > > Would adding documentation above this field be the right place, or > somewhere else? In any case, some explanation of the protocol would be > good. > I will include this. >> /* private: internal use only. */ >> bool success; >> @@ -162,6 +163,9 @@ static inline char *kunit_status_to_string(bool status) >> * &struct kunit_case for an example on how to use it. >> */ >> #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } >> +#define KUNIT_CASE_PARAM(test_name, gen_params) \ >> + { .run_case = test_name, .name = #test_name, \ >> + .generate_params = gen_params } >> >> /** >> * struct kunit_suite - describes a related collection of &struct kunit_case >> @@ -208,6 +212,15 @@ struct kunit { >> const char *name; /* Read only after initialization! */ >> char *log; /* Points at case log after initialization */ >> struct kunit_try_catch try_catch; >> + /* param_values points to test case parameters in parameterized tests */ >> + void *param_values; >> + /* >> + * current_param stores the index of the parameter in >> + * the array of parameters in parameterized tests. >> + * current_param + 1 is printed to indicate the parameter >> + * that causes the test to fail in case of test failure. >> + */ >> + int current_param; >> /* >> * success starts as true, and may only be set to false during a >> * test case; thus, it is safe to update this across multiple >> @@ -1742,4 +1755,36 @@ do { \ >> fmt, \ >> ##__VA_ARGS__) >> >> +/** >> + * kunit_param_generator_helper() - Helper method for test parameter generators >> + * required in parameterized tests. >> + * @test: The test context object. >> + * @prev_param: a pointer to the previous test parameter, NULL for first parameter. >> + * @param_array: a user-supplied pointer to an array of test parameters. >> + * @array_size: number of test parameters in the array. >> + * @type_size: size of one test parameter. >> + */ >> +static inline void *kunit_param_generator_helper(struct kunit *test, > > I don't think this needs to be inline, but see my other suggestion > below, which might make this function obsolete. > >> + void *prev_param, >> + void *param_array, >> + size_t array_size, >> + size_t type_size) >> +{ >> + KUNIT_ASSERT_EQ(test, (prev_param - param_array) % type_size, 0); >> + >> + if (!prev_param) >> + return param_array; >> + >> + KUNIT_ASSERT_GE(test, prev_param, param_array); >> + >> + if (prev_param + type_size < param_array + (array_size * type_size)) >> + return prev_param + type_size; >> + else >> + return NULL; >> +} >> + >> +#define KUNIT_PARAM_GENERATOR_HELPER(test, prev_param, param_array, param_type) \ >> + kunit_param_generator_helper(test, prev_param, param_array, \ >> + ARRAY_SIZE(param_array), sizeof(param_type)) > > You do not need param_type, you can use the same trick that ARRAY_SIZE > uses: > > #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr)) > > So you could use sizeof((param_aray)[0]) instead of sizeof(param_type). > ARRAY_SIZE already checks for you that it's a real array via > __must_be_array(). > > > The other question is, will kunit_param_generator_helper() find much use > without the KUNIT_PARAM_GENERATOR_HELPER() macro? If I have some > complicated generator protocol to generate params, then I'd just > directly write the generator function. If your intent is to simplify the > common-case array based generators, why not just have a macro generate > the generator function? > > More specifically, have this macro here: > > +#define KUNIT_ARRAY_PARAM(name, array) \ > + static void *name##_gen_params(struct kunit *test, void *prev) \ > + { \ > + typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \ > + return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL; \ > + } > > [ It is entirely untested, but if it works verbatim you'll probably need my > > Co-developed-by: Marco Elver <elver@google.com> > Signed-off-by: Marco Elver <elver@google.com> > > just in case... ] > > Then, it can be used as follows: > > static int num_cpus[] = {1, 2, 3, 4, 5}; > KUNIT_ARRAY_PARAM(num_cpus, num_cpus); > > Then somewhere else: > > KUNIT_CASE_PARAM(some_test, num_cpus_gen_params); > Yes, a macro can be used to generate the generator function. I will work with this for the next version. >> #endif /* _KUNIT_TEST_H */ >> diff --git a/lib/kunit/test.c b/lib/kunit/test.c >> index 750704abe89a..0e6ffe6384a7 100644 >> --- a/lib/kunit/test.c >> +++ b/lib/kunit/test.c >> @@ -127,6 +127,11 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite, >> } >> EXPORT_SYMBOL_GPL(kunit_test_case_num); >> >> +static void kunit_print_failed_param(struct kunit *test) >> +{ >> + kunit_err(test, "\n\tTest failed at parameter: %d\n", test->current_param + 1); >> +} >> + >> static void kunit_print_string_stream(struct kunit *test, >> struct string_stream *stream) >> { >> @@ -168,6 +173,8 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert) >> assert->format(assert, stream); >> >> kunit_print_string_stream(test, stream); >> + if (test->param_values) >> + kunit_print_failed_param(test); >> >> WARN_ON(string_stream_destroy(stream)); >> } >> @@ -239,7 +246,18 @@ static void kunit_run_case_internal(struct kunit *test, >> } >> } >> >> - test_case->run_case(test); >> + if (!test_case->generate_params) { >> + test_case->run_case(test); >> + } else { >> + test->param_values = test_case->generate_params(test, NULL); >> + test->current_param = 0; >> + >> + while (test->param_values) { >> + test_case->run_case(test); >> + test->param_values = test_case->generate_params(test, test->param_values); >> + test->current_param++; >> + } >> + } >> } >> >> static void kunit_case_internal_cleanup(struct kunit *test) > > Otherwise looks fine. > > Thanks, > -- Marco > Thanks! _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-26 18:19 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-23 15:05 [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing Arpitha Raghunandan 2020-10-23 15:06 ` [Linux-kernel-mentees] [PATCH v2 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature Arpitha Raghunandan 2020-10-26 18:11 ` Iurii Zaikin via Linux-kernel-mentees 2020-10-26 18:19 ` Arpitha Raghunandan 2020-10-23 18:48 ` [Linux-kernel-mentees] [PATCH v2 1/2] kunit: Support for Parameterized Testing Marco Elver via Linux-kernel-mentees 2020-10-23 18:56 ` Marco Elver via Linux-kernel-mentees 2020-10-24 5:23 ` Arpitha Raghunandan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox