2020-11-06 19:23:44

by Arpitha Raghunandan

[permalink] [raw]
Subject: [PATCH v6 1/2] kunit: Support for Parameterized Testing

Implementation of support for parameterized testing in KUnit.
This approach requires the creation of a test case using the
KUNIT_CASE_PARAM macro that accepts a generator function as input.
This generator function should return the next parameter given the
previous parameter in parameterized tests. It also provides
a macro to generate common-case generators.

Signed-off-by: Arpitha Raghunandan <[email protected]>
Co-developed-by: Marco Elver <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
---
Changes v5->v6:
- Fix alignment to maintain consistency
Changes v4->v5:
- Update kernel-doc comments.
- Use const void* for generator return and prev value types.
- Add kernel-doc comment for KUNIT_ARRAY_PARAM.
- Rework parameterized test case execution strategy: each parameter is executed
as if it was its own test case, with its own test initialization and cleanup
(init and exit are called, etc.). However, we cannot add new test cases per TAP
protocol once we have already started execution. Instead, log the result of
each parameter run as a diagnostic comment.
Changes v3->v4:
- Rename kunit variables
- Rename generator function helper macro
- Add documentation for generator approach
- Display test case name in case of failure along with param index
Changes v2->v3:
- Modifictaion of generator macro and method
Changes v1->v2:
- Use of a generator method to access test case parameters

include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
2 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index db1b0ae666c4..16616d3974f9 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -107,6 +107,7 @@ struct kunit;
*
* @run_case: the function representing the actual test case.
* @name: the name of the test case.
+ * @generate_params: the generator function for parameterized tests.
*
* A test case is a function with the signature,
* ``void (*)(struct kunit *)``
@@ -141,6 +142,7 @@ struct kunit;
struct kunit_case {
void (*run_case)(struct kunit *test);
const char *name;
+ const void* (*generate_params)(const void *prev);

/* private: internal use only. */
bool success;
@@ -163,6 +165,22 @@ static inline char *kunit_status_to_string(bool status)
*/
#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }

+/**
+ * KUNIT_CASE_PARAM - A helper for creation a parameterized &struct kunit_case
+ *
+ * @test_name: a reference to a test case function.
+ * @gen_params: a reference to a parameter generator function.
+ *
+ * The generator function ``const void* gen_params(const void *prev)`` is used
+ * to lazily generate a series of arbitrarily typed values that fit into a
+ * void*. The argument @prev is the previously returned value, which should be
+ * used to derive the next value; @prev is set to NULL on the initial generator
+ * call. When no more values are available, the generator must return NULL.
+ */
+#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 +226,10 @@ struct kunit {
const char *name; /* Read only after initialization! */
char *log; /* Points at case log after initialization */
struct kunit_try_catch try_catch;
+ /* param_value is the current parameter value for a test case. */
+ const void *param_value;
+ /* param_index stores the index of the parameter in parameterized tests. */
+ int param_index;
/*
* 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 +1764,18 @@ do { \
fmt, \
##__VA_ARGS__)

+/**
+ * KUNIT_ARRAY_PARAM() - Define test parameter generator from an array.
+ * @name: prefix for the test parameter generator function.
+ * @array: array of test parameters.
+ *
+ * Define function @name_gen_params which uses @array to generate parameters.
+ */
+#define KUNIT_ARRAY_PARAM(name, array) \
+ static const void *name##_gen_params(const void *prev) \
+ { \
+ typeof((array)[0]) * __next = prev ? ((typeof(__next)) prev) + 1 : (array); \
+ return __next - (array) < ARRAY_SIZE((array)) ? __next : NULL; \
+ }
+
#endif /* _KUNIT_TEST_H */
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 750704abe89a..329fee9e0634 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -325,29 +325,25 @@ static void kunit_catch_run_case(void *data)
* occur in a test case and reports them as failures.
*/
static void kunit_run_case_catch_errors(struct kunit_suite *suite,
- struct kunit_case *test_case)
+ struct kunit_case *test_case,
+ struct kunit *test)
{
struct kunit_try_catch_context context;
struct kunit_try_catch *try_catch;
- struct kunit test;

- kunit_init_test(&test, test_case->name, test_case->log);
- try_catch = &test.try_catch;
+ kunit_init_test(test, test_case->name, test_case->log);
+ try_catch = &test->try_catch;

kunit_try_catch_init(try_catch,
- &test,
+ test,
kunit_try_run_case,
kunit_catch_run_case);
- context.test = &test;
+ context.test = test;
context.suite = suite;
context.test_case = test_case;
kunit_try_catch_run(try_catch, &context);

- test_case->success = test.success;
-
- kunit_print_ok_not_ok(&test, true, test_case->success,
- kunit_test_case_num(suite, test_case),
- test_case->name);
+ test_case->success = test->success;
}

int kunit_run_tests(struct kunit_suite *suite)
@@ -356,8 +352,32 @@ int kunit_run_tests(struct kunit_suite *suite)

kunit_print_subtest_start(suite);

- kunit_suite_for_each_test_case(suite, test_case)
- kunit_run_case_catch_errors(suite, test_case);
+ kunit_suite_for_each_test_case(suite, test_case) {
+ struct kunit test = { .param_value = NULL, .param_index = 0 };
+ bool test_success = true;
+
+ if (test_case->generate_params)
+ test.param_value = test_case->generate_params(NULL);
+
+ do {
+ kunit_run_case_catch_errors(suite, test_case, &test);
+ test_success &= test_case->success;
+
+ if (test_case->generate_params) {
+ kunit_log(KERN_INFO, &test,
+ KUNIT_SUBTEST_INDENT
+ "# %s: param-%d %s",
+ test_case->name, test.param_index,
+ kunit_status_to_string(test.success));
+ test.param_value = test_case->generate_params(test.param_value);
+ test.param_index++;
+ }
+ } while (test.param_value);
+
+ kunit_print_ok_not_ok(&test, true, test_success,
+ kunit_test_case_num(suite, test_case),
+ test_case->name);
+ }

kunit_print_subtest_end(suite);

--
2.25.1


2020-11-06 19:23:45

by Arpitha Raghunandan

[permalink] [raw]
Subject: [PATCH v6 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature

Modify fs/ext4/inode-test.c to use the parameterized testing
feature of KUnit.

Signed-off-by: Arpitha Raghunandan <[email protected]>
---
Changes v5->v6:
- No change to this patch of the patch series
Changes v4->v5:
- No change to this patch of the patch series
Changes v3->v4:
- Modification based on latest implementation of KUnit parameterized testing
Changes v2->v3:
- Marked hardcoded test data const
- Modification based on latest implementation of KUnit parameterized testing
Changes v1->v2:
- Modification based on latest implementation of KUnit parameterized testing

fs/ext4/inode-test.c | 314 ++++++++++++++++++++++---------------------
1 file changed, 158 insertions(+), 156 deletions(-)

diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
index d62d802c9c12..ebf1b1af4f1d 100644
--- a/fs/ext4/inode-test.c
+++ b/fs/ext4/inode-test.c
@@ -80,6 +80,139 @@ struct timestamp_expectation {
bool lower_bound;
};

+static 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},
+ }
+};
+
+KUNIT_ARRAY_PARAM(ext4_inode, test_data);
+
static time64_t get_32bit_time(const struct timestamp_expectation * const test)
{
if (test->msb_set) {
@@ -101,166 +234,35 @@ 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(&timestamp,
- 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_value);
+
+ timestamp.tv_sec = get_32bit_time(test_param);
+ ext4_decode_extra_time(&timestamp,
+ 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 struct kunit_case ext4_inode_test_cases[] = {
- KUNIT_CASE(inode_test_xtimestamp_decoding),
+ KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding, ext4_inode_gen_params),
{}
};

--
2.25.1

2020-11-06 19:38:12

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Fri, 6 Nov 2020 at 20:22, Arpitha Raghunandan <[email protected]> wrote:
> Implementation of support for parameterized testing in KUnit.
> This approach requires the creation of a test case using the
> KUNIT_CASE_PARAM macro that accepts a generator function as input.
> This generator function should return the next parameter given the
> previous parameter in parameterized tests. It also provides
> a macro to generate common-case generators.
>
> Signed-off-by: Arpitha Raghunandan <[email protected]>
> Co-developed-by: Marco Elver <[email protected]>
> Signed-off-by: Marco Elver <[email protected]>
> ---
[...]
>
> include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
> lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
> 2 files changed, 69 insertions(+), 13 deletions(-)

Looks good, thank you!

Others: Please take another look.

Thanks,
-- Marco

2020-11-07 05:04:30

by David Gow

[permalink] [raw]
Subject: Re: [PATCH v6 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing feature

On Sat, Nov 7, 2020 at 3:23 AM Arpitha Raghunandan <[email protected]> wrote:
>
> Modify fs/ext4/inode-test.c to use the parameterized testing
> feature of KUnit.
>
> Signed-off-by: Arpitha Raghunandan <[email protected]>
> ---

This looks good to me. Thanks!

Reviewed-by: David Gow <[email protected]>

-- David

2020-11-07 10:07:26

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Sat, 7 Nov 2020 at 05:58, David Gow <[email protected]> wrote:
> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <[email protected]> wrote:
> >
> > Implementation of support for parameterized testing in KUnit.
> > This approach requires the creation of a test case using the
> > KUNIT_CASE_PARAM macro that accepts a generator function as input.
> > This generator function should return the next parameter given the
> > previous parameter in parameterized tests. It also provides
> > a macro to generate common-case generators.
> >
> > Signed-off-by: Arpitha Raghunandan <[email protected]>
> > Co-developed-by: Marco Elver <[email protected]>
> > Signed-off-by: Marco Elver <[email protected]>
> > ---
>
> This looks good to me! A couple of minor thoughts about the output
> format below, but I'm quite happy to have this as-is regardless.
>
> Reviewed-by: David Gow <[email protected]>
>
> Cheers,
> -- David
>
> > Changes v5->v6:
> > - Fix alignment to maintain consistency
> > Changes v4->v5:
> > - Update kernel-doc comments.
> > - Use const void* for generator return and prev value types.
> > - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
> > - Rework parameterized test case execution strategy: each parameter is executed
> > as if it was its own test case, with its own test initialization and cleanup
> > (init and exit are called, etc.). However, we cannot add new test cases per TAP
> > protocol once we have already started execution. Instead, log the result of
> > each parameter run as a diagnostic comment.
> > Changes v3->v4:
> > - Rename kunit variables
> > - Rename generator function helper macro
> > - Add documentation for generator approach
> > - Display test case name in case of failure along with param index
> > Changes v2->v3:
> > - Modifictaion of generator macro and method
> > Changes v1->v2:
> > - Use of a generator method to access test case parameters
> >
> > include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
> > lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
> > 2 files changed, 69 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > index db1b0ae666c4..16616d3974f9 100644
> > --- a/include/kunit/test.h
> > +++ b/include/kunit/test.h
> > @@ -107,6 +107,7 @@ struct kunit;
[...]
> > - kunit_suite_for_each_test_case(suite, test_case)
> > - kunit_run_case_catch_errors(suite, test_case);
> > + kunit_suite_for_each_test_case(suite, test_case) {
> > + struct kunit test = { .param_value = NULL, .param_index = 0 };
> > + bool test_success = true;
> > +
> > + if (test_case->generate_params)
> > + test.param_value = test_case->generate_params(NULL);
> > +
> > + do {
> > + kunit_run_case_catch_errors(suite, test_case, &test);
> > + test_success &= test_case->success;
> > +
> > + if (test_case->generate_params) {
> > + kunit_log(KERN_INFO, &test,
> > + KUNIT_SUBTEST_INDENT
> > + "# %s: param-%d %s",
>
> Would it make sense to have this imitate the TAP format a bit more?
> So, have "# [ok|not ok] - [name]" as the format? [name] could be
> something like "[test_case->name]:param-[index]" or similar.
> If we keep it commented out and don't indent it further, it won't
> formally be a nested test (though if we wanted to support those later,
> it'd be easy to add), but I think it would be nicer to be consistent
> here.

The previous attempt [1] at something similar failed because it seems
we'd need to teach kunit-tool new tricks [2], too.
[1] https://lkml.kernel.org/r/[email protected]
[2] https://lkml.kernel.org/r/[email protected]

So if we go with a different format, we might need a patch before this
one to make kunit-tool compatible with that type of diagnostic.

Currently I think we have the following proposals for a format:

1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
this works well, because no changes to kunit-tool are required, and it
also picks up the diagnostic context for the case and displays that on
test failure.

2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
As-is, this needs a patch for kunit-tool as well. I just checked, and
if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
(note the space after ':') it works without changing kunit-tool. ;-)

3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
which I had played with earlier but kunit-tool is definitely not yet
happy with.

So my current preference is (2) with the extra space (no change to
kunit-tool required). WDYT?

> My other suggestion -- albeit one outside the scope of this initial
> version -- would be to allow the "param-%d" name to be overridden
> somehow by a test. For example, the ext4 inode test has names for all
> its test cases: it'd be nice to be able to display those instead (even
> if they're not formatted as identifiers as-is).

Right, I was thinking about this, but it'd need a way to optionally
pass another function that converts const void* params to readable
strings. But as you say, we should do that as a follow-up patch later
because it might require a few more iterations.

[...]

Thanks,
-- Marco

2020-11-09 06:51:11

by Arpitha Raghunandan

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On 07/11/20 3:36 pm, Marco Elver wrote:
> On Sat, 7 Nov 2020 at 05:58, David Gow <[email protected]> wrote:
>> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <[email protected]> wrote:
>>>
>>> Implementation of support for parameterized testing in KUnit.
>>> This approach requires the creation of a test case using the
>>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
>>> This generator function should return the next parameter given the
>>> previous parameter in parameterized tests. It also provides
>>> a macro to generate common-case generators.
>>>
>>> Signed-off-by: Arpitha Raghunandan <[email protected]>
>>> Co-developed-by: Marco Elver <[email protected]>
>>> Signed-off-by: Marco Elver <[email protected]>
>>> ---
>>
>> This looks good to me! A couple of minor thoughts about the output
>> format below, but I'm quite happy to have this as-is regardless.
>>
>> Reviewed-by: David Gow <[email protected]>
>>
>> Cheers,
>> -- David
>>
>>> Changes v5->v6:
>>> - Fix alignment to maintain consistency
>>> Changes v4->v5:
>>> - Update kernel-doc comments.
>>> - Use const void* for generator return and prev value types.
>>> - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
>>> - Rework parameterized test case execution strategy: each parameter is executed
>>> as if it was its own test case, with its own test initialization and cleanup
>>> (init and exit are called, etc.). However, we cannot add new test cases per TAP
>>> protocol once we have already started execution. Instead, log the result of
>>> each parameter run as a diagnostic comment.
>>> Changes v3->v4:
>>> - Rename kunit variables
>>> - Rename generator function helper macro
>>> - Add documentation for generator approach
>>> - Display test case name in case of failure along with param index
>>> Changes v2->v3:
>>> - Modifictaion of generator macro and method
>>> Changes v1->v2:
>>> - Use of a generator method to access test case parameters
>>>
>>> include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
>>> lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
>>> 2 files changed, 69 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>>> index db1b0ae666c4..16616d3974f9 100644
>>> --- a/include/kunit/test.h
>>> +++ b/include/kunit/test.h
>>> @@ -107,6 +107,7 @@ struct kunit;
> [...]
>>> - kunit_suite_for_each_test_case(suite, test_case)
>>> - kunit_run_case_catch_errors(suite, test_case);
>>> + kunit_suite_for_each_test_case(suite, test_case) {
>>> + struct kunit test = { .param_value = NULL, .param_index = 0 };
>>> + bool test_success = true;
>>> +
>>> + if (test_case->generate_params)
>>> + test.param_value = test_case->generate_params(NULL);
>>> +
>>> + do {
>>> + kunit_run_case_catch_errors(suite, test_case, &test);
>>> + test_success &= test_case->success;
>>> +
>>> + if (test_case->generate_params) {
>>> + kunit_log(KERN_INFO, &test,
>>> + KUNIT_SUBTEST_INDENT
>>> + "# %s: param-%d %s",
>>
>> Would it make sense to have this imitate the TAP format a bit more?
>> So, have "# [ok|not ok] - [name]" as the format? [name] could be
>> something like "[test_case->name]:param-[index]" or similar.
>> If we keep it commented out and don't indent it further, it won't
>> formally be a nested test (though if we wanted to support those later,
>> it'd be easy to add), but I think it would be nicer to be consistent
>> here.
>
> The previous attempt [1] at something similar failed because it seems
> we'd need to teach kunit-tool new tricks [2], too.
> [1] https://lkml.kernel.org/r/[email protected]
> [2] https://lkml.kernel.org/r/[email protected]
>
> So if we go with a different format, we might need a patch before this
> one to make kunit-tool compatible with that type of diagnostic.
>
> Currently I think we have the following proposals for a format:
>
> 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> this works well, because no changes to kunit-tool are required, and it
> also picks up the diagnostic context for the case and displays that on
> test failure.
>
> 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> As-is, this needs a patch for kunit-tool as well. I just checked, and
> if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> (note the space after ':') it works without changing kunit-tool. ;-)
>
> 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> which I had played with earlier but kunit-tool is definitely not yet
> happy with.
>
> So my current preference is (2) with the extra space (no change to
> kunit-tool required). WDYT?
>

Which format do we finally go with?

>> My other suggestion -- albeit one outside the scope of this initial
>> version -- would be to allow the "param-%d" name to be overridden
>> somehow by a test. For example, the ext4 inode test has names for all
>> its test cases: it'd be nice to be able to display those instead (even
>> if they're not formatted as identifiers as-is).
>
> Right, I was thinking about this, but it'd need a way to optionally
> pass another function that converts const void* params to readable
> strings. But as you say, we should do that as a follow-up patch later
> because it might require a few more iterations.
>
> [...]
>
> Thanks,
> -- Marco
>

2020-11-10 07:23:17

by David Gow

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Mon, Nov 9, 2020 at 2:49 PM Arpitha Raghunandan <[email protected]> wrote:
>
> On 07/11/20 3:36 pm, Marco Elver wrote:
> > On Sat, 7 Nov 2020 at 05:58, David Gow <[email protected]> wrote:
> >> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <[email protected]> wrote:
> >>>
> >>> Implementation of support for parameterized testing in KUnit.
> >>> This approach requires the creation of a test case using the
> >>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
> >>> This generator function should return the next parameter given the
> >>> previous parameter in parameterized tests. It also provides
> >>> a macro to generate common-case generators.
> >>>
> >>> Signed-off-by: Arpitha Raghunandan <[email protected]>
> >>> Co-developed-by: Marco Elver <[email protected]>
> >>> Signed-off-by: Marco Elver <[email protected]>
> >>> ---
> >>
> >> This looks good to me! A couple of minor thoughts about the output
> >> format below, but I'm quite happy to have this as-is regardless.
> >>
> >> Reviewed-by: David Gow <[email protected]>
> >>
> >> Cheers,
> >> -- David
> >>
> >>> Changes v5->v6:
> >>> - Fix alignment to maintain consistency
> >>> Changes v4->v5:
> >>> - Update kernel-doc comments.
> >>> - Use const void* for generator return and prev value types.
> >>> - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
> >>> - Rework parameterized test case execution strategy: each parameter is executed
> >>> as if it was its own test case, with its own test initialization and cleanup
> >>> (init and exit are called, etc.). However, we cannot add new test cases per TAP
> >>> protocol once we have already started execution. Instead, log the result of
> >>> each parameter run as a diagnostic comment.
> >>> Changes v3->v4:
> >>> - Rename kunit variables
> >>> - Rename generator function helper macro
> >>> - Add documentation for generator approach
> >>> - Display test case name in case of failure along with param index
> >>> Changes v2->v3:
> >>> - Modifictaion of generator macro and method
> >>> Changes v1->v2:
> >>> - Use of a generator method to access test case parameters
> >>>
> >>> include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
> >>> lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
> >>> 2 files changed, 69 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >>> index db1b0ae666c4..16616d3974f9 100644
> >>> --- a/include/kunit/test.h
> >>> +++ b/include/kunit/test.h
> >>> @@ -107,6 +107,7 @@ struct kunit;
> > [...]
> >>> - kunit_suite_for_each_test_case(suite, test_case)
> >>> - kunit_run_case_catch_errors(suite, test_case);
> >>> + kunit_suite_for_each_test_case(suite, test_case) {
> >>> + struct kunit test = { .param_value = NULL, .param_index = 0 };
> >>> + bool test_success = true;
> >>> +
> >>> + if (test_case->generate_params)
> >>> + test.param_value = test_case->generate_params(NULL);
> >>> +
> >>> + do {
> >>> + kunit_run_case_catch_errors(suite, test_case, &test);
> >>> + test_success &= test_case->success;
> >>> +
> >>> + if (test_case->generate_params) {
> >>> + kunit_log(KERN_INFO, &test,
> >>> + KUNIT_SUBTEST_INDENT
> >>> + "# %s: param-%d %s",
> >>
> >> Would it make sense to have this imitate the TAP format a bit more?
> >> So, have "# [ok|not ok] - [name]" as the format? [name] could be
> >> something like "[test_case->name]:param-[index]" or similar.
> >> If we keep it commented out and don't indent it further, it won't
> >> formally be a nested test (though if we wanted to support those later,
> >> it'd be easy to add), but I think it would be nicer to be consistent
> >> here.
> >
> > The previous attempt [1] at something similar failed because it seems
> > we'd need to teach kunit-tool new tricks [2], too.
> > [1] https://lkml.kernel.org/r/[email protected]
> > [2] https://lkml.kernel.org/r/[email protected]
> >
> > So if we go with a different format, we might need a patch before this
> > one to make kunit-tool compatible with that type of diagnostic.
> >
> > Currently I think we have the following proposals for a format:
> >
> > 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> > this works well, because no changes to kunit-tool are required, and it
> > also picks up the diagnostic context for the case and displays that on
> > test failure.
> >
> > 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> > As-is, this needs a patch for kunit-tool as well. I just checked, and
> > if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> > (note the space after ':') it works without changing kunit-tool. ;-)
> >
> > 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> > which I had played with earlier but kunit-tool is definitely not yet
> > happy with.
> >
> > So my current preference is (2) with the extra space (no change to
> > kunit-tool required). WDYT?
> >

Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
what comes after the comment character except if it's an explicit
subtest declaration or a crash. I'll try to put a patch together to
fix it, but I'd rather not delay this just for that.

In any thought about this a bit more, It turns out that the proposed
KTAP spec[1] discourages the use of ':', except as part of a subtest
declaration, or perhaps an as-yet-unspecified fully-qualified test
name. The latter is what I was going for, but if it's actively
breaking kunit_tool, we might want to hold off on it.

If we were to try to treat these as subtests in accordance with that
spec, the way we'd want to use one of these options:
A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
test case name, but otherwise treats things exactly the same way we
treat existing subtests.

B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
"subtest", just gives repeated results with the same name.

C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
-- This is equivalent to my suggestion with a separator of ":", or 2
above with a separator of ": ". The in-progress spec doesn't yet
specify how these fully-qualified names would work, other than that
they'd use a colon somewhere, and if we comment it out, ": " is
required.

>
> Which format do we finally go with?
>

I'm actually going to make another wild suggestion for this, which is
a combination of (1) and (A):
"# [test_case->name]: [ok|not ok] [index] - param-[index]"

This gives us a KTAP-compliant result line, except prepended with "#
[test_case->name]: ", which makes it formally a diagnostic line,
rather than an actual subtest. Putting the test name at the start
matches what kunit_tool is expecting at the moment. If we then want to
turn it into a proper subtest, we can just get rid of that prefix (and
add the appropriate counts elsewhere).

Does that sound good?


> [...]

Thanks,
-- David

[1]: https://lore.kernel.org/linux-kselftest/CY4PR13MB1175B804E31E502221BC8163FD830@CY4PR13MB1175.namprd13.prod.outlook.com/T/

2020-11-10 10:36:14

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Tue, 10 Nov 2020 at 08:21, David Gow <[email protected]> wrote:
[...]
> > >
> > > The previous attempt [1] at something similar failed because it seems
> > > we'd need to teach kunit-tool new tricks [2], too.
> > > [1] https://lkml.kernel.org/r/[email protected]
> > > [2] https://lkml.kernel.org/r/[email protected]
> > >
> > > So if we go with a different format, we might need a patch before this
> > > one to make kunit-tool compatible with that type of diagnostic.
> > >
> > > Currently I think we have the following proposals for a format:
> > >
> > > 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> > > this works well, because no changes to kunit-tool are required, and it
> > > also picks up the diagnostic context for the case and displays that on
> > > test failure.
> > >
> > > 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> > > As-is, this needs a patch for kunit-tool as well. I just checked, and
> > > if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> > > (note the space after ':') it works without changing kunit-tool. ;-)
> > >
> > > 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> > > which I had played with earlier but kunit-tool is definitely not yet
> > > happy with.
> > >
> > > So my current preference is (2) with the extra space (no change to
> > > kunit-tool required). WDYT?
> > >
>
> Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
> what comes after the comment character except if it's an explicit
> subtest declaration or a crash. I'll try to put a patch together to
> fix it, but I'd rather not delay this just for that.
>
> In any thought about this a bit more, It turns out that the proposed
> KTAP spec[1] discourages the use of ':', except as part of a subtest
> declaration, or perhaps an as-yet-unspecified fully-qualified test
> name. The latter is what I was going for, but if it's actively
> breaking kunit_tool, we might want to hold off on it.
>
> If we were to try to treat these as subtests in accordance with that
> spec, the way we'd want to use one of these options:
> A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
> test case name, but otherwise treats things exactly the same way we
> treat existing subtests.
>
> B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
> "subtest", just gives repeated results with the same name.
>
> C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> -- This is equivalent to my suggestion with a separator of ":", or 2
> above with a separator of ": ". The in-progress spec doesn't yet
> specify how these fully-qualified names would work, other than that
> they'd use a colon somewhere, and if we comment it out, ": " is
> required.
>
> >
> > Which format do we finally go with?
> >
>
> I'm actually going to make another wild suggestion for this, which is
> a combination of (1) and (A):
> "# [test_case->name]: [ok|not ok] [index] - param-[index]"
>
> This gives us a KTAP-compliant result line, except prepended with "#
> [test_case->name]: ", which makes it formally a diagnostic line,
> rather than an actual subtest. Putting the test name at the start
> matches what kunit_tool is expecting at the moment. If we then want to
> turn it into a proper subtest, we can just get rid of that prefix (and
> add the appropriate counts elsewhere).
>
> Does that sound good?

Sounds reasonable to me! The repetition of [index] seems unnecessary
for now, but I think if we at some point have a way to get a string
representation of a param, we can substitute param-[index] with a
string that represents the param.

Note that once we want to make it a real subtest, we'd need to run the
generator twice, once to get the number of params and then to run the
tests. If we require that param generators are deterministic in the
number of params generated, this is not a problem.

Thanks,
-- Marco

2020-11-10 16:35:29

by Arpitha Raghunandan

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On 10/11/20 4:05 pm, Marco Elver wrote:
> On Tue, 10 Nov 2020 at 08:21, David Gow <[email protected]> wrote:
> [...]
>>>>
>>>> The previous attempt [1] at something similar failed because it seems
>>>> we'd need to teach kunit-tool new tricks [2], too.
>>>> [1] https://lkml.kernel.org/r/[email protected]
>>>> [2] https://lkml.kernel.org/r/[email protected]
>>>>
>>>> So if we go with a different format, we might need a patch before this
>>>> one to make kunit-tool compatible with that type of diagnostic.
>>>>
>>>> Currently I think we have the following proposals for a format:
>>>>
>>>> 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
>>>> this works well, because no changes to kunit-tool are required, and it
>>>> also picks up the diagnostic context for the case and displays that on
>>>> test failure.
>>>>
>>>> 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
>>>> As-is, this needs a patch for kunit-tool as well. I just checked, and
>>>> if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
>>>> (note the space after ':') it works without changing kunit-tool. ;-)
>>>>
>>>> 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
>>>> which I had played with earlier but kunit-tool is definitely not yet
>>>> happy with.
>>>>
>>>> So my current preference is (2) with the extra space (no change to
>>>> kunit-tool required). WDYT?
>>>>
>>
>> Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
>> what comes after the comment character except if it's an explicit
>> subtest declaration or a crash. I'll try to put a patch together to
>> fix it, but I'd rather not delay this just for that.
>>
>> In any thought about this a bit more, It turns out that the proposed
>> KTAP spec[1] discourages the use of ':', except as part of a subtest
>> declaration, or perhaps an as-yet-unspecified fully-qualified test
>> name. The latter is what I was going for, but if it's actively
>> breaking kunit_tool, we might want to hold off on it.
>>
>> If we were to try to treat these as subtests in accordance with that
>> spec, the way we'd want to use one of these options:
>> A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
>> test case name, but otherwise treats things exactly the same way we
>> treat existing subtests.
>>
>> B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
>> "subtest", just gives repeated results with the same name.
>>
>> C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
>> -- This is equivalent to my suggestion with a separator of ":", or 2
>> above with a separator of ": ". The in-progress spec doesn't yet
>> specify how these fully-qualified names would work, other than that
>> they'd use a colon somewhere, and if we comment it out, ": " is
>> required.
>>
>>>
>>> Which format do we finally go with?
>>>
>>
>> I'm actually going to make another wild suggestion for this, which is
>> a combination of (1) and (A):
>> "# [test_case->name]: [ok|not ok] [index] - param-[index]"
>>
>> This gives us a KTAP-compliant result line, except prepended with "#
>> [test_case->name]: ", which makes it formally a diagnostic line,
>> rather than an actual subtest. Putting the test name at the start
>> matches what kunit_tool is expecting at the moment. If we then want to
>> turn it into a proper subtest, we can just get rid of that prefix (and
>> add the appropriate counts elsewhere).
>>
>> Does that sound good?
>
> Sounds reasonable to me! The repetition of [index] seems unnecessary
> for now, but I think if we at some point have a way to get a string
> representation of a param, we can substitute param-[index] with a
> string that represents the param.
>

So, with this the inode-test.c will have the following output, right?

TAP version 14
1..7
# Subtest: ext4_inode_test
1..1
# inode_test_xtimestamp_decoding: ok 0 - param-0
# inode_test_xtimestamp_decoding: ok 1 - param-1
# inode_test_xtimestamp_decoding: ok 2 - param-2
# inode_test_xtimestamp_decoding: ok 3 - param-3
# inode_test_xtimestamp_decoding: ok 4 - param-4
# inode_test_xtimestamp_decoding: ok 5 - param-5
# inode_test_xtimestamp_decoding: ok 6 - param-6
# inode_test_xtimestamp_decoding: ok 7 - param-7
# inode_test_xtimestamp_decoding: ok 8 - param-8
# inode_test_xtimestamp_decoding: ok 9 - param-9
# inode_test_xtimestamp_decoding: ok 10 - param-10
# inode_test_xtimestamp_decoding: ok 11 - param-11
# inode_test_xtimestamp_decoding: ok 12 - param-12
# inode_test_xtimestamp_decoding: ok 13 - param-13
# inode_test_xtimestamp_decoding: ok 14 - param-14
# inode_test_xtimestamp_decoding: ok 15 - param-15
ok 1 - inode_test_xtimestamp_decoding
ok 1 - ext4_inode_test

I will send another patch with this change.
Thanks!

> Note that once we want to make it a real subtest, we'd need to run the
> generator twice, once to get the number of params and then to run the
> tests. If we require that param generators are deterministic in the
> number of params generated, this is not a problem.
>
> Thanks,
> -- Marco
>

2020-11-10 16:41:51

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Tue, 10 Nov 2020 at 17:32, Arpitha Raghunandan <[email protected]> wrote:
>
> On 10/11/20 4:05 pm, Marco Elver wrote:
> > On Tue, 10 Nov 2020 at 08:21, David Gow <[email protected]> wrote:
> > [...]
> >>>>
> >>>> The previous attempt [1] at something similar failed because it seems
> >>>> we'd need to teach kunit-tool new tricks [2], too.
> >>>> [1] https://lkml.kernel.org/r/[email protected]
> >>>> [2] https://lkml.kernel.org/r/[email protected]
> >>>>
> >>>> So if we go with a different format, we might need a patch before this
> >>>> one to make kunit-tool compatible with that type of diagnostic.
> >>>>
> >>>> Currently I think we have the following proposals for a format:
> >>>>
> >>>> 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> >>>> this works well, because no changes to kunit-tool are required, and it
> >>>> also picks up the diagnostic context for the case and displays that on
> >>>> test failure.
> >>>>
> >>>> 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> >>>> As-is, this needs a patch for kunit-tool as well. I just checked, and
> >>>> if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> >>>> (note the space after ':') it works without changing kunit-tool. ;-)
> >>>>
> >>>> 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> >>>> which I had played with earlier but kunit-tool is definitely not yet
> >>>> happy with.
> >>>>
> >>>> So my current preference is (2) with the extra space (no change to
> >>>> kunit-tool required). WDYT?
> >>>>
> >>
> >> Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
> >> what comes after the comment character except if it's an explicit
> >> subtest declaration or a crash. I'll try to put a patch together to
> >> fix it, but I'd rather not delay this just for that.
> >>
> >> In any thought about this a bit more, It turns out that the proposed
> >> KTAP spec[1] discourages the use of ':', except as part of a subtest
> >> declaration, or perhaps an as-yet-unspecified fully-qualified test
> >> name. The latter is what I was going for, but if it's actively
> >> breaking kunit_tool, we might want to hold off on it.
> >>
> >> If we were to try to treat these as subtests in accordance with that
> >> spec, the way we'd want to use one of these options:
> >> A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
> >> test case name, but otherwise treats things exactly the same way we
> >> treat existing subtests.
> >>
> >> B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
> >> "subtest", just gives repeated results with the same name.
> >>
> >> C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> >> -- This is equivalent to my suggestion with a separator of ":", or 2
> >> above with a separator of ": ". The in-progress spec doesn't yet
> >> specify how these fully-qualified names would work, other than that
> >> they'd use a colon somewhere, and if we comment it out, ": " is
> >> required.
> >>
> >>>
> >>> Which format do we finally go with?
> >>>
> >>
> >> I'm actually going to make another wild suggestion for this, which is
> >> a combination of (1) and (A):
> >> "# [test_case->name]: [ok|not ok] [index] - param-[index]"
> >>
> >> This gives us a KTAP-compliant result line, except prepended with "#
> >> [test_case->name]: ", which makes it formally a diagnostic line,
> >> rather than an actual subtest. Putting the test name at the start
> >> matches what kunit_tool is expecting at the moment. If we then want to
> >> turn it into a proper subtest, we can just get rid of that prefix (and
> >> add the appropriate counts elsewhere).
> >>
> >> Does that sound good?
> >
> > Sounds reasonable to me! The repetition of [index] seems unnecessary
> > for now, but I think if we at some point have a way to get a string
> > representation of a param, we can substitute param-[index] with a
> > string that represents the param.
> >
>
> So, with this the inode-test.c will have the following output, right?
>
> TAP version 14
> 1..7
> # Subtest: ext4_inode_test
> 1..1
> # inode_test_xtimestamp_decoding: ok 0 - param-0

So the params should certainly be 0-indexed, but I think TAP starts
counting at 1, so maybe this should be:

# inode_test_xtimestamp_decoding: ok 1 - param-0

and so on... ?

Right now it doesn't matter, but it will if we make these subsubtests
as David suggested.

> # inode_test_xtimestamp_decoding: ok 1 - param-1
> # inode_test_xtimestamp_decoding: ok 2 - param-2
> # inode_test_xtimestamp_decoding: ok 3 - param-3
> # inode_test_xtimestamp_decoding: ok 4 - param-4
> # inode_test_xtimestamp_decoding: ok 5 - param-5
> # inode_test_xtimestamp_decoding: ok 6 - param-6
> # inode_test_xtimestamp_decoding: ok 7 - param-7
> # inode_test_xtimestamp_decoding: ok 8 - param-8
> # inode_test_xtimestamp_decoding: ok 9 - param-9
> # inode_test_xtimestamp_decoding: ok 10 - param-10
> # inode_test_xtimestamp_decoding: ok 11 - param-11
> # inode_test_xtimestamp_decoding: ok 12 - param-12
> # inode_test_xtimestamp_decoding: ok 13 - param-13
> # inode_test_xtimestamp_decoding: ok 14 - param-14
> # inode_test_xtimestamp_decoding: ok 15 - param-15
> ok 1 - inode_test_xtimestamp_decoding
> ok 1 - ext4_inode_test
>
> I will send another patch with this change.
> Thanks!

Yes that looks right, but see the comment above ^

Thanks,
-- Marco

2020-11-10 16:52:15

by Arpitha Raghunandan

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On 10/11/20 10:11 pm, Marco Elver wrote:
> On Tue, 10 Nov 2020 at 17:32, Arpitha Raghunandan <[email protected]> wrote:
>>
>> On 10/11/20 4:05 pm, Marco Elver wrote:
>>> On Tue, 10 Nov 2020 at 08:21, David Gow <[email protected]> wrote:
>>> [...]
>>>>>>
>>>>>> The previous attempt [1] at something similar failed because it seems
>>>>>> we'd need to teach kunit-tool new tricks [2], too.
>>>>>> [1] https://lkml.kernel.org/r/[email protected]
>>>>>> [2] https://lkml.kernel.org/r/[email protected]
>>>>>>
>>>>>> So if we go with a different format, we might need a patch before this
>>>>>> one to make kunit-tool compatible with that type of diagnostic.
>>>>>>
>>>>>> Currently I think we have the following proposals for a format:
>>>>>>
>>>>>> 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
>>>>>> this works well, because no changes to kunit-tool are required, and it
>>>>>> also picks up the diagnostic context for the case and displays that on
>>>>>> test failure.
>>>>>>
>>>>>> 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
>>>>>> As-is, this needs a patch for kunit-tool as well. I just checked, and
>>>>>> if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
>>>>>> (note the space after ':') it works without changing kunit-tool. ;-)
>>>>>>
>>>>>> 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
>>>>>> which I had played with earlier but kunit-tool is definitely not yet
>>>>>> happy with.
>>>>>>
>>>>>> So my current preference is (2) with the extra space (no change to
>>>>>> kunit-tool required). WDYT?
>>>>>>
>>>>
>>>> Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
>>>> what comes after the comment character except if it's an explicit
>>>> subtest declaration or a crash. I'll try to put a patch together to
>>>> fix it, but I'd rather not delay this just for that.
>>>>
>>>> In any thought about this a bit more, It turns out that the proposed
>>>> KTAP spec[1] discourages the use of ':', except as part of a subtest
>>>> declaration, or perhaps an as-yet-unspecified fully-qualified test
>>>> name. The latter is what I was going for, but if it's actively
>>>> breaking kunit_tool, we might want to hold off on it.
>>>>
>>>> If we were to try to treat these as subtests in accordance with that
>>>> spec, the way we'd want to use one of these options:
>>>> A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
>>>> test case name, but otherwise treats things exactly the same way we
>>>> treat existing subtests.
>>>>
>>>> B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
>>>> "subtest", just gives repeated results with the same name.
>>>>
>>>> C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
>>>> -- This is equivalent to my suggestion with a separator of ":", or 2
>>>> above with a separator of ": ". The in-progress spec doesn't yet
>>>> specify how these fully-qualified names would work, other than that
>>>> they'd use a colon somewhere, and if we comment it out, ": " is
>>>> required.
>>>>
>>>>>
>>>>> Which format do we finally go with?
>>>>>
>>>>
>>>> I'm actually going to make another wild suggestion for this, which is
>>>> a combination of (1) and (A):
>>>> "# [test_case->name]: [ok|not ok] [index] - param-[index]"
>>>>
>>>> This gives us a KTAP-compliant result line, except prepended with "#
>>>> [test_case->name]: ", which makes it formally a diagnostic line,
>>>> rather than an actual subtest. Putting the test name at the start
>>>> matches what kunit_tool is expecting at the moment. If we then want to
>>>> turn it into a proper subtest, we can just get rid of that prefix (and
>>>> add the appropriate counts elsewhere).
>>>>
>>>> Does that sound good?
>>>
>>> Sounds reasonable to me! The repetition of [index] seems unnecessary
>>> for now, but I think if we at some point have a way to get a string
>>> representation of a param, we can substitute param-[index] with a
>>> string that represents the param.
>>>
>>
>> So, with this the inode-test.c will have the following output, right?
>>
>> TAP version 14
>> 1..7
>> # Subtest: ext4_inode_test
>> 1..1
>> # inode_test_xtimestamp_decoding: ok 0 - param-0
>
> So the params should certainly be 0-indexed, but I think TAP starts
> counting at 1, so maybe this should be:
>
> # inode_test_xtimestamp_decoding: ok 1 - param-0
>
> and so on... ?
>
> Right now it doesn't matter, but it will if we make these subsubtests
> as David suggested.
>

Okay yes, I will make the first index (following [ok|not ok]) start with 1
and let the params remain 0-indexed.
Thanks!

>> # inode_test_xtimestamp_decoding: ok 1 - param-1
>> # inode_test_xtimestamp_decoding: ok 2 - param-2
>> # inode_test_xtimestamp_decoding: ok 3 - param-3
>> # inode_test_xtimestamp_decoding: ok 4 - param-4
>> # inode_test_xtimestamp_decoding: ok 5 - param-5
>> # inode_test_xtimestamp_decoding: ok 6 - param-6
>> # inode_test_xtimestamp_decoding: ok 7 - param-7
>> # inode_test_xtimestamp_decoding: ok 8 - param-8
>> # inode_test_xtimestamp_decoding: ok 9 - param-9
>> # inode_test_xtimestamp_decoding: ok 10 - param-10
>> # inode_test_xtimestamp_decoding: ok 11 - param-11
>> # inode_test_xtimestamp_decoding: ok 12 - param-12
>> # inode_test_xtimestamp_decoding: ok 13 - param-13
>> # inode_test_xtimestamp_decoding: ok 14 - param-14
>> # inode_test_xtimestamp_decoding: ok 15 - param-15
>> ok 1 - inode_test_xtimestamp_decoding
>> ok 1 - ext4_inode_test
>>
>> I will send another patch with this change.
>> Thanks!
>
> Yes that looks right, but see the comment above ^
>
> Thanks,
> -- Marco
>

2020-11-10 18:44:57

by Bird, Tim

[permalink] [raw]
Subject: RE: [PATCH v6 1/2] kunit: Support for Parameterized Testing



> -----Original Message-----
> From: David Gow <[email protected]>
>
> On Mon, Nov 9, 2020 at 2:49 PM Arpitha Raghunandan <[email protected]> wrote:
> >
> > On 07/11/20 3:36 pm, Marco Elver wrote:
> > > On Sat, 7 Nov 2020 at 05:58, David Gow <[email protected]> wrote:
> > >> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <[email protected]> wrote:
> > >>>
> > >>> Implementation of support for parameterized testing in KUnit.
> > >>> This approach requires the creation of a test case using the
> > >>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
> > >>> This generator function should return the next parameter given the
> > >>> previous parameter in parameterized tests. It also provides
> > >>> a macro to generate common-case generators.
> > >>>
> > >>> Signed-off-by: Arpitha Raghunandan <[email protected]>
> > >>> Co-developed-by: Marco Elver <[email protected]>
> > >>> Signed-off-by: Marco Elver <[email protected]>
> > >>> ---
> > >>
> > >> This looks good to me! A couple of minor thoughts about the output
> > >> format below, but I'm quite happy to have this as-is regardless.
> > >>
> > >> Reviewed-by: David Gow <[email protected]>
> > >>
> > >> Cheers,
> > >> -- David
> > >>
> > >>> Changes v5->v6:
> > >>> - Fix alignment to maintain consistency
> > >>> Changes v4->v5:
> > >>> - Update kernel-doc comments.
> > >>> - Use const void* for generator return and prev value types.
> > >>> - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
> > >>> - Rework parameterized test case execution strategy: each parameter is executed
> > >>> as if it was its own test case, with its own test initialization and cleanup
> > >>> (init and exit are called, etc.). However, we cannot add new test cases per TAP
> > >>> protocol once we have already started execution. Instead, log the result of
> > >>> each parameter run as a diagnostic comment.
> > >>> Changes v3->v4:
> > >>> - Rename kunit variables
> > >>> - Rename generator function helper macro
> > >>> - Add documentation for generator approach
> > >>> - Display test case name in case of failure along with param index
> > >>> Changes v2->v3:
> > >>> - Modifictaion of generator macro and method
> > >>> Changes v1->v2:
> > >>> - Use of a generator method to access test case parameters
> > >>>
> > >>> include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
> > >>> lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
> > >>> 2 files changed, 69 insertions(+), 13 deletions(-)
> > >>>
> > >>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > >>> index db1b0ae666c4..16616d3974f9 100644
> > >>> --- a/include/kunit/test.h
> > >>> +++ b/include/kunit/test.h
> > >>> @@ -107,6 +107,7 @@ struct kunit;
> > > [...]
> > >>> - kunit_suite_for_each_test_case(suite, test_case)
> > >>> - kunit_run_case_catch_errors(suite, test_case);
> > >>> + kunit_suite_for_each_test_case(suite, test_case) {
> > >>> + struct kunit test = { .param_value = NULL, .param_index = 0 };
> > >>> + bool test_success = true;
> > >>> +
> > >>> + if (test_case->generate_params)
> > >>> + test.param_value = test_case->generate_params(NULL);
> > >>> +
> > >>> + do {
> > >>> + kunit_run_case_catch_errors(suite, test_case, &test);
> > >>> + test_success &= test_case->success;
> > >>> +
> > >>> + if (test_case->generate_params) {
> > >>> + kunit_log(KERN_INFO, &test,
> > >>> + KUNIT_SUBTEST_INDENT
> > >>> + "# %s: param-%d %s",
> > >>
> > >> Would it make sense to have this imitate the TAP format a bit more?
> > >> So, have "# [ok|not ok] - [name]" as the format? [name] could be
> > >> something like "[test_case->name]:param-[index]" or similar.
> > >> If we keep it commented out and don't indent it further, it won't
> > >> formally be a nested test (though if we wanted to support those later,
> > >> it'd be easy to add), but I think it would be nicer to be consistent
> > >> here.
> > >
> > > The previous attempt [1] at something similar failed because it seems
> > > we'd need to teach kunit-tool new tricks [2], too.
> > > [1] https://lkml.kernel.org/r/[email protected]
> > > [2] https://lkml.kernel.org/r/[email protected]
> > >
> > > So if we go with a different format, we might need a patch before this
> > > one to make kunit-tool compatible with that type of diagnostic.
> > >
> > > Currently I think we have the following proposals for a format:
> > >
> > > 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> > > this works well, because no changes to kunit-tool are required, and it
> > > also picks up the diagnostic context for the case and displays that on
> > > test failure.
> > >
> > > 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> > > As-is, this needs a patch for kunit-tool as well. I just checked, and
> > > if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> > > (note the space after ':') it works without changing kunit-tool. ;-)
> > >
> > > 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> > > which I had played with earlier but kunit-tool is definitely not yet
> > > happy with.
> > >
> > > So my current preference is (2) with the extra space (no change to
> > > kunit-tool required). WDYT?
> > >
>
> Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
> what comes after the comment character except if it's an explicit
> subtest declaration or a crash. I'll try to put a patch together to
> fix it, but I'd rather not delay this just for that.
>
> In any thought about this a bit more, It turns out that the proposed
> KTAP spec[1] discourages the use of ':', except as part of a subtest
> declaration, or perhaps an as-yet-unspecified fully-qualified test
> name. The latter is what I was going for, but if it's actively
> breaking kunit_tool, we might want to hold off on it.
>
> If we were to try to treat these as subtests in accordance with that
> spec, the way we'd want to use one of these options:
> A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
> test case name, but otherwise treats things exactly the same way we
> treat existing subtests.
>
> B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
> "subtest", just gives repeated results with the same name.
>
> C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> -- This is equivalent to my suggestion with a separator of ":", or 2
> above with a separator of ": ". The in-progress spec doesn't yet
> specify how these fully-qualified names would work, other than that
> they'd use a colon somewhere, and if we comment it out, ": " is
> required.
>
> >
> > Which format do we finally go with?
> >
>
> I'm actually going to make another wild suggestion for this, which is
> a combination of (1) and (A):
> "# [test_case->name]: [ok|not ok] [index] - param-[index]"

I strongly object to putting actual testcase results in comments.
I'd rather that we found a way to include the parameter in the
sub-test-case name, rather than require all parsers to know about
specially-formatted comments. There are tools outside
the kernel that parse these lines.

>
> This gives us a KTAP-compliant result line, except prepended with "#
> [test_case->name]: ", which makes it formally a diagnostic line,
> rather than an actual subtest. Putting the test name at the start
> matches what kunit_tool is expecting at the moment. If we then want to
> turn it into a proper subtest, we can just get rid of that prefix (and
> add the appropriate counts elsewhere).
>
> Does that sound good?
No.

I strongly prefer option C above:
"[ok|not ok] [index] - [test_case->name][separator]param-[index]"

Except of course the second index is not the same as the first, so it
would be:
"[ok|not ok] [index] - [test_case->name][separator]param-[param-index]"

If ':' is problematical as a separator, let's choose something else.
What are the allowed and disallowed characters in the testcase name now?
How bad would it be to use something like % or &?

Unless the separator is #, I think most parsers are going to just treat the whole
string from after the '[index] -' to a following '#' as a testcase name, and it
should get parsed (and presented) OK. I'm not sure what kunit_tool's problem is.

-- Tim

2020-11-10 23:29:44

by David Gow

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Wed, Nov 11, 2020 at 1:02 AM Bird, Tim <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: David Gow <[email protected]>
> >
> > On Mon, Nov 9, 2020 at 2:49 PM Arpitha Raghunandan <[email protected]> wrote:
> > >
> > > On 07/11/20 3:36 pm, Marco Elver wrote:
> > > > On Sat, 7 Nov 2020 at 05:58, David Gow <[email protected]> wrote:
> > > >> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <[email protected]> wrote:
> > > >>>
> > > >>> Implementation of support for parameterized testing in KUnit.
> > > >>> This approach requires the creation of a test case using the
> > > >>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
> > > >>> This generator function should return the next parameter given the
> > > >>> previous parameter in parameterized tests. It also provides
> > > >>> a macro to generate common-case generators.
> > > >>>
> > > >>> Signed-off-by: Arpitha Raghunandan <[email protected]>
> > > >>> Co-developed-by: Marco Elver <[email protected]>
> > > >>> Signed-off-by: Marco Elver <[email protected]>
> > > >>> ---
> > > >>
> > > >> This looks good to me! A couple of minor thoughts about the output
> > > >> format below, but I'm quite happy to have this as-is regardless.
> > > >>
> > > >> Reviewed-by: David Gow <[email protected]>
> > > >>
> > > >> Cheers,
> > > >> -- David
> > > >>
> > > >>> Changes v5->v6:
> > > >>> - Fix alignment to maintain consistency
> > > >>> Changes v4->v5:
> > > >>> - Update kernel-doc comments.
> > > >>> - Use const void* for generator return and prev value types.
> > > >>> - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
> > > >>> - Rework parameterized test case execution strategy: each parameter is executed
> > > >>> as if it was its own test case, with its own test initialization and cleanup
> > > >>> (init and exit are called, etc.). However, we cannot add new test cases per TAP
> > > >>> protocol once we have already started execution. Instead, log the result of
> > > >>> each parameter run as a diagnostic comment.
> > > >>> Changes v3->v4:
> > > >>> - Rename kunit variables
> > > >>> - Rename generator function helper macro
> > > >>> - Add documentation for generator approach
> > > >>> - Display test case name in case of failure along with param index
> > > >>> Changes v2->v3:
> > > >>> - Modifictaion of generator macro and method
> > > >>> Changes v1->v2:
> > > >>> - Use of a generator method to access test case parameters
> > > >>>
> > > >>> include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
> > > >>> lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
> > > >>> 2 files changed, 69 insertions(+), 13 deletions(-)
> > > >>>
> > > >>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > >>> index db1b0ae666c4..16616d3974f9 100644
> > > >>> --- a/include/kunit/test.h
> > > >>> +++ b/include/kunit/test.h
> > > >>> @@ -107,6 +107,7 @@ struct kunit;
> > > > [...]
> > > >>> - kunit_suite_for_each_test_case(suite, test_case)
> > > >>> - kunit_run_case_catch_errors(suite, test_case);
> > > >>> + kunit_suite_for_each_test_case(suite, test_case) {
> > > >>> + struct kunit test = { .param_value = NULL, .param_index = 0 };
> > > >>> + bool test_success = true;
> > > >>> +
> > > >>> + if (test_case->generate_params)
> > > >>> + test.param_value = test_case->generate_params(NULL);
> > > >>> +
> > > >>> + do {
> > > >>> + kunit_run_case_catch_errors(suite, test_case, &test);
> > > >>> + test_success &= test_case->success;
> > > >>> +
> > > >>> + if (test_case->generate_params) {
> > > >>> + kunit_log(KERN_INFO, &test,
> > > >>> + KUNIT_SUBTEST_INDENT
> > > >>> + "# %s: param-%d %s",
> > > >>
> > > >> Would it make sense to have this imitate the TAP format a bit more?
> > > >> So, have "# [ok|not ok] - [name]" as the format? [name] could be
> > > >> something like "[test_case->name]:param-[index]" or similar.
> > > >> If we keep it commented out and don't indent it further, it won't
> > > >> formally be a nested test (though if we wanted to support those later,
> > > >> it'd be easy to add), but I think it would be nicer to be consistent
> > > >> here.
> > > >
> > > > The previous attempt [1] at something similar failed because it seems
> > > > we'd need to teach kunit-tool new tricks [2], too.
> > > > [1] https://lkml.kernel.org/r/[email protected]
> > > > [2] https://lkml.kernel.org/r/[email protected]
> > > >
> > > > So if we go with a different format, we might need a patch before this
> > > > one to make kunit-tool compatible with that type of diagnostic.
> > > >
> > > > Currently I think we have the following proposals for a format:
> > > >
> > > > 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> > > > this works well, because no changes to kunit-tool are required, and it
> > > > also picks up the diagnostic context for the case and displays that on
> > > > test failure.
> > > >
> > > > 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> > > > As-is, this needs a patch for kunit-tool as well. I just checked, and
> > > > if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> > > > (note the space after ':') it works without changing kunit-tool. ;-)
> > > >
> > > > 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> > > > which I had played with earlier but kunit-tool is definitely not yet
> > > > happy with.
> > > >
> > > > So my current preference is (2) with the extra space (no change to
> > > > kunit-tool required). WDYT?
> > > >
> >
> > Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
> > what comes after the comment character except if it's an explicit
> > subtest declaration or a crash. I'll try to put a patch together to
> > fix it, but I'd rather not delay this just for that.
> >
> > In any thought about this a bit more, It turns out that the proposed
> > KTAP spec[1] discourages the use of ':', except as part of a subtest
> > declaration, or perhaps an as-yet-unspecified fully-qualified test
> > name. The latter is what I was going for, but if it's actively
> > breaking kunit_tool, we might want to hold off on it.
> >
> > If we were to try to treat these as subtests in accordance with that
> > spec, the way we'd want to use one of these options:
> > A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
> > test case name, but otherwise treats things exactly the same way we
> > treat existing subtests.
> >
> > B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
> > "subtest", just gives repeated results with the same name.
> >
> > C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> > -- This is equivalent to my suggestion with a separator of ":", or 2
> > above with a separator of ": ". The in-progress spec doesn't yet
> > specify how these fully-qualified names would work, other than that
> > they'd use a colon somewhere, and if we comment it out, ": " is
> > required.
> >
> > >
> > > Which format do we finally go with?
> > >
> >
> > I'm actually going to make another wild suggestion for this, which is
> > a combination of (1) and (A):
> > "# [test_case->name]: [ok|not ok] [index] - param-[index]"
>
> I strongly object to putting actual testcase results in comments.
> I'd rather that we found a way to include the parameter in the
> sub-test-case name, rather than require all parsers to know about
> specially-formatted comments. There are tools outside
> the kernel that parse these lines.
>

I wasn't intending to treat these as actual testcases yet: from
KUnit's point of view, they're definitely just supposed to be
human-readable diagnostic comments for the one combined testcase.

This largely stems from KUnit being pretty rigid in its test
hierarchy: it has test suites (the root-level testcases), which
contain tests (the first-level subtests). Basically, arbitrary nesting
of tests isn't supported at all by any of the KUnit tools, code,
documentation, etc. So, if we do actually want to treat these
individual parameters as sub-subtests, it'll require a lot of work on
the KUnit side to be able to parse and represent those results.

So, as planned at the moment, these lines won't be parsed at all (just
passed to the user), and should KUnit support more complicated test
hierarchies down the line, we can remove the "# [test_case->name]"
prefix, add the test plan lines, etc, and have this be picked up by
parsers then.


> >
> > This gives us a KTAP-compliant result line, except prepended with "#
> > [test_case->name]: ", which makes it formally a diagnostic line,
> > rather than an actual subtest. Putting the test name at the start
> > matches what kunit_tool is expecting at the moment. If we then want to
> > turn it into a proper subtest, we can just get rid of that prefix (and
> > add the appropriate counts elsewhere).
> >
> > Does that sound good?
> No.
>
> I strongly prefer option C above:
> "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
>
> Except of course the second index is not the same as the first, so it
> would be:
> "[ok|not ok] [index] - [test_case->name][separator]param-[param-index]"

So, the second index would be the same as the first (at most with an
off-by-one to account for different indexing if we wished) in what I
was thinking.

I think this boils down to how we treat these tests and parameters:
- There is one TAP/KUnit test per-parameter, probably with a name like
"test_case:param-n". There's no "container" test.
- There is a test and result for the whole test, and per-parameter
tests and results are nested in that as subtests.
- There is a single test, and any per-parameter information is simply
diagnostic data for the one test, not to be parsed.

The current code follows the last of these options, and it was my view
that by having that diagnostic data be in a similar format to a test
result line, it'd be easier to convert this to the second option while
having a familiar format for people reading the results manually. Only
the first option should need separate indices for the result and the
parameter.

> If ':' is problematical as a separator, let's choose something else.
> What are the allowed and disallowed characters in the testcase name now?
> How bad would it be to use something like % or &?
>
> Unless the separator is #, I think most parsers are going to just treat the whole
> string from after the '[index] -' to a following '#' as a testcase name, and it
> should get parsed (and presented) OK. I'm not sure what kunit_tool's problem is.
>

kunit_tool has a bug when parsing the comments / diagnostic lines,
which requires a ": " to be present. This is a bug, which is being
fixed[1], so while it's _nice_ to not trigger it, it's not really an
important long-term goal of the format. In any case, this kunit_tool
issue only affects the comment lines: if the per-parameter result line
is an actual result, rather than just a diagnostic, this shouldn't be
a problem.

In any case, I still prefer my proposed option for now -- noting that
these per-parameter results are not actually supposed to be parsed --
with then the possibility of expanding them to actual nested results
later should we wish. But if the idea of having TAP-like lines in
diagnostics seems too dangerous (e.g. because people will try to parse
them anyway), then I think the options we have are to stick to the
output format given in the current version of this patch (which
doesn't resemble a TAP result), make each parameterised version its
own test (without a "container test", which would require a bit of
extra work while computing test plans), or to hold this whole feature
back until we can support arbitrary test hierarchies in KUnit.
Personally, I'd rather not hold this feature back, and prefer to have
a single combined result available, so would just stick with v6 if
so...

Does that make sense?

Cheers,
-- David

2020-11-11 16:57:17

by Bird, Tim

[permalink] [raw]
Subject: RE: [PATCH v6 1/2] kunit: Support for Parameterized Testing



> -----Original Message-----
> From: David Gow <[email protected]>
>
> On Wed, Nov 11, 2020 at 1:02 AM Bird, Tim <[email protected]> wrote:
> >
> > > -----Original Message-----
> > > From: David Gow <[email protected]>
> > >
> > > On Mon, Nov 9, 2020 at 2:49 PM Arpitha Raghunandan <[email protected]> wrote:
> > > >
> > > > On 07/11/20 3:36 pm, Marco Elver wrote:
> > > > > On Sat, 7 Nov 2020 at 05:58, David Gow <[email protected]> wrote:
> > > > >> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <[email protected]> wrote:
> > > > >>>
> > > > >>> Implementation of support for parameterized testing in KUnit.
> > > > >>> This approach requires the creation of a test case using the
> > > > >>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
> > > > >>> This generator function should return the next parameter given the
> > > > >>> previous parameter in parameterized tests. It also provides
> > > > >>> a macro to generate common-case generators.
> > > > >>>
> > > > >>> Signed-off-by: Arpitha Raghunandan <[email protected]>
> > > > >>> Co-developed-by: Marco Elver <[email protected]>
> > > > >>> Signed-off-by: Marco Elver <[email protected]>
> > > > >>> ---
> > > > >>
> > > > >> This looks good to me! A couple of minor thoughts about the output
> > > > >> format below, but I'm quite happy to have this as-is regardless.
> > > > >>
> > > > >> Reviewed-by: David Gow <[email protected]>
> > > > >>
> > > > >> Cheers,
> > > > >> -- David
> > > > >>
> > > > >>> Changes v5->v6:
> > > > >>> - Fix alignment to maintain consistency
> > > > >>> Changes v4->v5:
> > > > >>> - Update kernel-doc comments.
> > > > >>> - Use const void* for generator return and prev value types.
> > > > >>> - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
> > > > >>> - Rework parameterized test case execution strategy: each parameter is executed
> > > > >>> as if it was its own test case, with its own test initialization and cleanup
> > > > >>> (init and exit are called, etc.). However, we cannot add new test cases per TAP
> > > > >>> protocol once we have already started execution. Instead, log the result of
> > > > >>> each parameter run as a diagnostic comment.
> > > > >>> Changes v3->v4:
> > > > >>> - Rename kunit variables
> > > > >>> - Rename generator function helper macro
> > > > >>> - Add documentation for generator approach
> > > > >>> - Display test case name in case of failure along with param index
> > > > >>> Changes v2->v3:
> > > > >>> - Modifictaion of generator macro and method
> > > > >>> Changes v1->v2:
> > > > >>> - Use of a generator method to access test case parameters
> > > > >>>
> > > > >>> include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
> > > > >>> lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
> > > > >>> 2 files changed, 69 insertions(+), 13 deletions(-)
> > > > >>>
> > > > >>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > > >>> index db1b0ae666c4..16616d3974f9 100644
> > > > >>> --- a/include/kunit/test.h
> > > > >>> +++ b/include/kunit/test.h
> > > > >>> @@ -107,6 +107,7 @@ struct kunit;
> > > > > [...]
> > > > >>> - kunit_suite_for_each_test_case(suite, test_case)
> > > > >>> - kunit_run_case_catch_errors(suite, test_case);
> > > > >>> + kunit_suite_for_each_test_case(suite, test_case) {
> > > > >>> + struct kunit test = { .param_value = NULL, .param_index = 0 };
> > > > >>> + bool test_success = true;
> > > > >>> +
> > > > >>> + if (test_case->generate_params)
> > > > >>> + test.param_value = test_case->generate_params(NULL);
> > > > >>> +
> > > > >>> + do {
> > > > >>> + kunit_run_case_catch_errors(suite, test_case, &test);
> > > > >>> + test_success &= test_case->success;
> > > > >>> +
> > > > >>> + if (test_case->generate_params) {
> > > > >>> + kunit_log(KERN_INFO, &test,
> > > > >>> + KUNIT_SUBTEST_INDENT
> > > > >>> + "# %s: param-%d %s",
> > > > >>
> > > > >> Would it make sense to have this imitate the TAP format a bit more?
> > > > >> So, have "# [ok|not ok] - [name]" as the format? [name] could be
> > > > >> something like "[test_case->name]:param-[index]" or similar.
> > > > >> If we keep it commented out and don't indent it further, it won't
> > > > >> formally be a nested test (though if we wanted to support those later,
> > > > >> it'd be easy to add), but I think it would be nicer to be consistent
> > > > >> here.
> > > > >
> > > > > The previous attempt [1] at something similar failed because it seems
> > > > > we'd need to teach kunit-tool new tricks [2], too.
> > > > > [1] https://lkml.kernel.org/r/[email protected]
> > > > > [2] https://lkml.kernel.org/r/[email protected]
> > > > >
> > > > > So if we go with a different format, we might need a patch before this
> > > > > one to make kunit-tool compatible with that type of diagnostic.
> > > > >
> > > > > Currently I think we have the following proposals for a format:
> > > > >
> > > > > 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> > > > > this works well, because no changes to kunit-tool are required, and it
> > > > > also picks up the diagnostic context for the case and displays that on
> > > > > test failure.
> > > > >
> > > > > 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> > > > > As-is, this needs a patch for kunit-tool as well. I just checked, and
> > > > > if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> > > > > (note the space after ':') it works without changing kunit-tool. ;-)
> > > > >
> > > > > 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> > > > > which I had played with earlier but kunit-tool is definitely not yet
> > > > > happy with.
> > > > >
> > > > > So my current preference is (2) with the extra space (no change to
> > > > > kunit-tool required). WDYT?
> > > > >
> > >
> > > Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
> > > what comes after the comment character except if it's an explicit
> > > subtest declaration or a crash. I'll try to put a patch together to
> > > fix it, but I'd rather not delay this just for that.
> > >
> > > In any thought about this a bit more, It turns out that the proposed
> > > KTAP spec[1] discourages the use of ':', except as part of a subtest
> > > declaration, or perhaps an as-yet-unspecified fully-qualified test
> > > name. The latter is what I was going for, but if it's actively
> > > breaking kunit_tool, we might want to hold off on it.
> > >
> > > If we were to try to treat these as subtests in accordance with that
> > > spec, the way we'd want to use one of these options:
> > > A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
> > > test case name, but otherwise treats things exactly the same way we
> > > treat existing subtests.
> > >
> > > B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
> > > "subtest", just gives repeated results with the same name.
> > >
> > > C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> > > -- This is equivalent to my suggestion with a separator of ":", or 2
> > > above with a separator of ": ". The in-progress spec doesn't yet
> > > specify how these fully-qualified names would work, other than that
> > > they'd use a colon somewhere, and if we comment it out, ": " is
> > > required.
> > >
> > > >
> > > > Which format do we finally go with?
> > > >
> > >
> > > I'm actually going to make another wild suggestion for this, which is
> > > a combination of (1) and (A):
> > > "# [test_case->name]: [ok|not ok] [index] - param-[index]"
> >
> > I strongly object to putting actual testcase results in comments.
> > I'd rather that we found a way to include the parameter in the
> > sub-test-case name, rather than require all parsers to know about
> > specially-formatted comments. There are tools outside
> > the kernel that parse these lines.
> >
>
> I wasn't intending to treat these as actual testcases yet: from
> KUnit's point of view, they're definitely just supposed to be
> human-readable diagnostic comments for the one combined testcase.
>
> This largely stems from KUnit being pretty rigid in its test
> hierarchy: it has test suites (the root-level testcases), which
> contain tests (the first-level subtests). Basically, arbitrary nesting
> of tests isn't supported at all by any of the KUnit tools, code,
> documentation, etc. So, if we do actually want to treat these
> individual parameters as sub-subtests, it'll require a lot of work on
> the KUnit side to be able to parse and represent those results.
>
> So, as planned at the moment, these lines won't be parsed at all (just
> passed to the user), and should KUnit support more complicated test
> hierarchies down the line, we can remove the "# [test_case->name]"
> prefix, add the test plan lines, etc, and have this be picked up by
> parsers then.
>
>
> > >
> > > This gives us a KTAP-compliant result line, except prepended with "#
> > > [test_case->name]: ", which makes it formally a diagnostic line,
> > > rather than an actual subtest. Putting the test name at the start
> > > matches what kunit_tool is expecting at the moment. If we then want to
> > > turn it into a proper subtest, we can just get rid of that prefix (and
> > > add the appropriate counts elsewhere).
> > >
> > > Does that sound good?
> > No.
> >
> > I strongly prefer option C above:
> > "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> >
> > Except of course the second index is not the same as the first, so it
> > would be:
> > "[ok|not ok] [index] - [test_case->name][separator]param-[param-index]"
>
> So, the second index would be the same as the first (at most with an
> off-by-one to account for different indexing if we wished) in what I
> was thinking.
>
> I think this boils down to how we treat these tests and parameters:
> - There is one TAP/KUnit test per-parameter, probably with a name like
> "test_case:param-n". There's no "container" test.
> - There is a test and result for the whole test, and per-parameter
> tests and results are nested in that as subtests.
> - There is a single test, and any per-parameter information is simply
> diagnostic data for the one test, not to be parsed.
>
> The current code follows the last of these options, and it was my view
> that by having that diagnostic data be in a similar format to a test
> result line, it'd be easier to convert this to the second option while
> having a familiar format for people reading the results manually. Only
> the first option should need separate indices for the result and the
> parameter.
>
> > If ':' is problematical as a separator, let's choose something else.
> > What are the allowed and disallowed characters in the testcase name now?
> > How bad would it be to use something like % or &?
> >
> > Unless the separator is #, I think most parsers are going to just treat the whole
> > string from after the '[index] -' to a following '#' as a testcase name, and it
> > should get parsed (and presented) OK. I'm not sure what kunit_tool's problem is.
> >
>
> kunit_tool has a bug when parsing the comments / diagnostic lines,
> which requires a ": " to be present. This is a bug, which is being
> fixed[1], so while it's _nice_ to not trigger it, it's not really an
> important long-term goal of the format. In any case, this kunit_tool
> issue only affects the comment lines: if the per-parameter result line
> is an actual result, rather than just a diagnostic, this shouldn't be
> a problem.
>
> In any case, I still prefer my proposed option for now -- noting that
> these per-parameter results are not actually supposed to be parsed --
> with then the possibility of expanding them to actual nested results
> later should we wish. But if the idea of having TAP-like lines in
> diagnostics seems too dangerous (e.g. because people will try to parse
> them anyway), then I think the options we have are to stick to the
> output format given in the current version of this patch (which
> doesn't resemble a TAP result), make each parameterised version its
> own test (without a "container test", which would require a bit of
> extra work while computing test plans), or to hold this whole feature
> back until we can support arbitrary test hierarchies in KUnit.
It seems like you're missing a 4th option, which is just tack the parameter
name on, without using a colon, and have these testcases treated
as unique within the context of the super-test. Is there some reason
these can't be expressed as individual testcases in the parent test?

> Personally, I'd rather not hold this feature back, and prefer to have
> a single combined result available, so would just stick with v6 if
> so...
>
> Does that make sense?

I understand what you are saying, but this seems like a step backwards.
We already know that having just numbers to represent a test case is not
very human friendly. The same will go for these parameter case numbers.
I admit to not following this kunit test parameterization effort, so I don't
know the background of how the numbers relate to the parameters.
But there must be some actual semantic meaning to each of the parameter
cases. Not conveying that meaning as part of the test case name seems like
a missed opportunity.

I'm at a little of a loss as to why, if you have valid testcase results, you would
shy away from putting them into a format that is machine-readable. If it's because
the tooling is not there, then IMHO you should fix the tooling.

I realize that perfect is the enemy of good enough, and that there's value for humans
to see these testcase results and manually interpret them, even if they are put into
a syntax that automated parsers will ignore. However, I do think there's a danger that
this syntax will get canonicalized. Personally, I'd rather see the testcases
with parameters show up in the parsable results. I'd rather sacrifice the hierarchy
than the results.

Just my 2 cents.
-- Tim

2020-11-12 08:20:16

by David Gow

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Thu, Nov 12, 2020 at 12:55 AM Bird, Tim <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: David Gow <[email protected]>
> >
> > On Wed, Nov 11, 2020 at 1:02 AM Bird, Tim <[email protected]> wrote:
> > >
> > > > -----Original Message-----
> > > > From: David Gow <[email protected]>
> > > >
> > > > On Mon, Nov 9, 2020 at 2:49 PM Arpitha Raghunandan <[email protected]> wrote:
> > > > >
> > > > > On 07/11/20 3:36 pm, Marco Elver wrote:
> > > > > > On Sat, 7 Nov 2020 at 05:58, David Gow <[email protected]> wrote:
> > > > > >> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <[email protected]> wrote:
> > > > > >>>
> > > > > >>> Implementation of support for parameterized testing in KUnit.
> > > > > >>> This approach requires the creation of a test case using the
> > > > > >>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
> > > > > >>> This generator function should return the next parameter given the
> > > > > >>> previous parameter in parameterized tests. It also provides
> > > > > >>> a macro to generate common-case generators.
> > > > > >>>
> > > > > >>> Signed-off-by: Arpitha Raghunandan <[email protected]>
> > > > > >>> Co-developed-by: Marco Elver <[email protected]>
> > > > > >>> Signed-off-by: Marco Elver <[email protected]>
> > > > > >>> ---
> > > > > >>
> > > > > >> This looks good to me! A couple of minor thoughts about the output
> > > > > >> format below, but I'm quite happy to have this as-is regardless.
> > > > > >>
> > > > > >> Reviewed-by: David Gow <[email protected]>
> > > > > >>
> > > > > >> Cheers,
> > > > > >> -- David
> > > > > >>
> > > > > >>> Changes v5->v6:
> > > > > >>> - Fix alignment to maintain consistency
> > > > > >>> Changes v4->v5:
> > > > > >>> - Update kernel-doc comments.
> > > > > >>> - Use const void* for generator return and prev value types.
> > > > > >>> - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
> > > > > >>> - Rework parameterized test case execution strategy: each parameter is executed
> > > > > >>> as if it was its own test case, with its own test initialization and cleanup
> > > > > >>> (init and exit are called, etc.). However, we cannot add new test cases per TAP
> > > > > >>> protocol once we have already started execution. Instead, log the result of
> > > > > >>> each parameter run as a diagnostic comment.
> > > > > >>> Changes v3->v4:
> > > > > >>> - Rename kunit variables
> > > > > >>> - Rename generator function helper macro
> > > > > >>> - Add documentation for generator approach
> > > > > >>> - Display test case name in case of failure along with param index
> > > > > >>> Changes v2->v3:
> > > > > >>> - Modifictaion of generator macro and method
> > > > > >>> Changes v1->v2:
> > > > > >>> - Use of a generator method to access test case parameters
> > > > > >>>
> > > > > >>> include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
> > > > > >>> lib/kunit/test.c | 46 +++++++++++++++++++++++++++++++-------------
> > > > > >>> 2 files changed, 69 insertions(+), 13 deletions(-)
> > > > > >>>
> > > > > >>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > > > >>> index db1b0ae666c4..16616d3974f9 100644
> > > > > >>> --- a/include/kunit/test.h
> > > > > >>> +++ b/include/kunit/test.h
> > > > > >>> @@ -107,6 +107,7 @@ struct kunit;
> > > > > > [...]
> > > > > >>> - kunit_suite_for_each_test_case(suite, test_case)
> > > > > >>> - kunit_run_case_catch_errors(suite, test_case);
> > > > > >>> + kunit_suite_for_each_test_case(suite, test_case) {
> > > > > >>> + struct kunit test = { .param_value = NULL, .param_index = 0 };
> > > > > >>> + bool test_success = true;
> > > > > >>> +
> > > > > >>> + if (test_case->generate_params)
> > > > > >>> + test.param_value = test_case->generate_params(NULL);
> > > > > >>> +
> > > > > >>> + do {
> > > > > >>> + kunit_run_case_catch_errors(suite, test_case, &test);
> > > > > >>> + test_success &= test_case->success;
> > > > > >>> +
> > > > > >>> + if (test_case->generate_params) {
> > > > > >>> + kunit_log(KERN_INFO, &test,
> > > > > >>> + KUNIT_SUBTEST_INDENT
> > > > > >>> + "# %s: param-%d %s",
> > > > > >>
> > > > > >> Would it make sense to have this imitate the TAP format a bit more?
> > > > > >> So, have "# [ok|not ok] - [name]" as the format? [name] could be
> > > > > >> something like "[test_case->name]:param-[index]" or similar.
> > > > > >> If we keep it commented out and don't indent it further, it won't
> > > > > >> formally be a nested test (though if we wanted to support those later,
> > > > > >> it'd be easy to add), but I think it would be nicer to be consistent
> > > > > >> here.
> > > > > >
> > > > > > The previous attempt [1] at something similar failed because it seems
> > > > > > we'd need to teach kunit-tool new tricks [2], too.
> > > > > > [1] https://lkml.kernel.org/r/[email protected]
> > > > > > [2] https://lkml.kernel.org/r/[email protected]
> > > > > >
> > > > > > So if we go with a different format, we might need a patch before this
> > > > > > one to make kunit-tool compatible with that type of diagnostic.
> > > > > >
> > > > > > Currently I think we have the following proposals for a format:
> > > > > >
> > > > > > 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> > > > > > this works well, because no changes to kunit-tool are required, and it
> > > > > > also picks up the diagnostic context for the case and displays that on
> > > > > > test failure.
> > > > > >
> > > > > > 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> > > > > > As-is, this needs a patch for kunit-tool as well. I just checked, and
> > > > > > if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> > > > > > (note the space after ':') it works without changing kunit-tool. ;-)
> > > > > >
> > > > > > 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> > > > > > which I had played with earlier but kunit-tool is definitely not yet
> > > > > > happy with.
> > > > > >
> > > > > > So my current preference is (2) with the extra space (no change to
> > > > > > kunit-tool required). WDYT?
> > > > > >
> > > >
> > > > Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
> > > > what comes after the comment character except if it's an explicit
> > > > subtest declaration or a crash. I'll try to put a patch together to
> > > > fix it, but I'd rather not delay this just for that.
> > > >
> > > > In any thought about this a bit more, It turns out that the proposed
> > > > KTAP spec[1] discourages the use of ':', except as part of a subtest
> > > > declaration, or perhaps an as-yet-unspecified fully-qualified test
> > > > name. The latter is what I was going for, but if it's actively
> > > > breaking kunit_tool, we might want to hold off on it.
> > > >
> > > > If we were to try to treat these as subtests in accordance with that
> > > > spec, the way we'd want to use one of these options:
> > > > A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
> > > > test case name, but otherwise treats things exactly the same way we
> > > > treat existing subtests.
> > > >
> > > > B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
> > > > "subtest", just gives repeated results with the same name.
> > > >
> > > > C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> > > > -- This is equivalent to my suggestion with a separator of ":", or 2
> > > > above with a separator of ": ". The in-progress spec doesn't yet
> > > > specify how these fully-qualified names would work, other than that
> > > > they'd use a colon somewhere, and if we comment it out, ": " is
> > > > required.
> > > >
> > > > >
> > > > > Which format do we finally go with?
> > > > >
> > > >
> > > > I'm actually going to make another wild suggestion for this, which is
> > > > a combination of (1) and (A):
> > > > "# [test_case->name]: [ok|not ok] [index] - param-[index]"
> > >
> > > I strongly object to putting actual testcase results in comments.
> > > I'd rather that we found a way to include the parameter in the
> > > sub-test-case name, rather than require all parsers to know about
> > > specially-formatted comments. There are tools outside
> > > the kernel that parse these lines.
> > >
> >
> > I wasn't intending to treat these as actual testcases yet: from
> > KUnit's point of view, they're definitely just supposed to be
> > human-readable diagnostic comments for the one combined testcase.
> >
> > This largely stems from KUnit being pretty rigid in its test
> > hierarchy: it has test suites (the root-level testcases), which
> > contain tests (the first-level subtests). Basically, arbitrary nesting
> > of tests isn't supported at all by any of the KUnit tools, code,
> > documentation, etc. So, if we do actually want to treat these
> > individual parameters as sub-subtests, it'll require a lot of work on
> > the KUnit side to be able to parse and represent those results.
> >
> > So, as planned at the moment, these lines won't be parsed at all (just
> > passed to the user), and should KUnit support more complicated test
> > hierarchies down the line, we can remove the "# [test_case->name]"
> > prefix, add the test plan lines, etc, and have this be picked up by
> > parsers then.
> >
> >
> > > >
> > > > This gives us a KTAP-compliant result line, except prepended with "#
> > > > [test_case->name]: ", which makes it formally a diagnostic line,
> > > > rather than an actual subtest. Putting the test name at the start
> > > > matches what kunit_tool is expecting at the moment. If we then want to
> > > > turn it into a proper subtest, we can just get rid of that prefix (and
> > > > add the appropriate counts elsewhere).
> > > >
> > > > Does that sound good?
> > > No.
> > >
> > > I strongly prefer option C above:
> > > "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> > >
> > > Except of course the second index is not the same as the first, so it
> > > would be:
> > > "[ok|not ok] [index] - [test_case->name][separator]param-[param-index]"
> >
> > So, the second index would be the same as the first (at most with an
> > off-by-one to account for different indexing if we wished) in what I
> > was thinking.
> >
> > I think this boils down to how we treat these tests and parameters:
> > - There is one TAP/KUnit test per-parameter, probably with a name like
> > "test_case:param-n". There's no "container" test.
> > - There is a test and result for the whole test, and per-parameter
> > tests and results are nested in that as subtests.
> > - There is a single test, and any per-parameter information is simply
> > diagnostic data for the one test, not to be parsed.
> >
> > The current code follows the last of these options, and it was my view
> > that by having that diagnostic data be in a similar format to a test
> > result line, it'd be easier to convert this to the second option while
> > having a familiar format for people reading the results manually. Only
> > the first option should need separate indices for the result and the
> > parameter.
> >
> > > If ':' is problematical as a separator, let's choose something else.
> > > What are the allowed and disallowed characters in the testcase name now?
> > > How bad would it be to use something like % or &?
> > >
> > > Unless the separator is #, I think most parsers are going to just treat the whole
> > > string from after the '[index] -' to a following '#' as a testcase name, and it
> > > should get parsed (and presented) OK. I'm not sure what kunit_tool's problem is.
> > >
> >
> > kunit_tool has a bug when parsing the comments / diagnostic lines,
> > which requires a ": " to be present. This is a bug, which is being
> > fixed[1], so while it's _nice_ to not trigger it, it's not really an
> > important long-term goal of the format. In any case, this kunit_tool
> > issue only affects the comment lines: if the per-parameter result line
> > is an actual result, rather than just a diagnostic, this shouldn't be
> > a problem.
> >
> > In any case, I still prefer my proposed option for now -- noting that
> > these per-parameter results are not actually supposed to be parsed --
> > with then the possibility of expanding them to actual nested results
> > later should we wish. But if the idea of having TAP-like lines in
> > diagnostics seems too dangerous (e.g. because people will try to parse
> > them anyway), then I think the options we have are to stick to the
> > output format given in the current version of this patch (which
> > doesn't resemble a TAP result), make each parameterised version its
> > own test (without a "container test", which would require a bit of
> > extra work while computing test plans), or to hold this whole feature
> > back until we can support arbitrary test hierarchies in KUnit.
> It seems like you're missing a 4th option, which is just tack the parameter
> name on, without using a colon, and have these testcases treated
> as unique within the context of the super-test. Is there some reason
> these can't be expressed as individual testcases in the parent test?
>

No: there's no fundamental reason why we couldn't do that, though
there are some things which make it suboptiomal, IMHO.

Firstly, there could be a lot of parameters, and that by not grouping
them together it could make dealing with the results a little
unwieldy. The other side of that is that it'll result in tests being
split up and renamed as they're ported to use this, whereas if the
whole test shows up once (with subtests or without), the old test name
can still be there, with a single PASS/FAIL for the whole test.

(It also might be a little tricky with the current implementation to
produce the test plan, as the parameters come from a generator, and I
don't think there's a way of getting the number of parameters ahead of
time. That's a problem with the sub-subtest model, too, though at
least there it's a little more isolated from other tests.)

> > Personally, I'd rather not hold this feature back, and prefer to have
> > a single combined result available, so would just stick with v6 if
> > so...
> >
> > Does that make sense?
>
> I understand what you are saying, but this seems like a step backwards.
> We already know that having just numbers to represent a test case is not
> very human friendly. The same will go for these parameter case numbers.
> I admit to not following this kunit test parameterization effort, so I don't
> know the background of how the numbers relate to the parameters.
> But there must be some actual semantic meaning to each of the parameter
> cases. Not conveying that meaning as part of the test case name seems like
> a missed opportunity.

Yeah: I'm not a big fan of just numbering the parameters either: the
plan is to eventually support naming these. Basically, the goal is to
be able to run the same test code repeatedly with different inputs
(which may be programmatically generated): depending on the testcase /
parameter generator in question, the numbers may mean something
specific, but it's not necessarily the case. Certainly in most cases,
the order of these parameters is unlikely to matter, so having the
number be part of the test name isn't ideal there: it's unlikely to
have semantic meaning, and worst-case could be unstable due to code
changes.


>
> I'm at a little of a loss as to why, if you have valid testcase results, you would
> shy away from putting them into a format that is machine-readable. If it's because
> the tooling is not there, then IMHO you should fix the tooling.

I think the real disconnect here is the ambiguity between treating
each run-through with a different parameter as its own test case,
versus an implementation detail of the single "meta testcase". Since
parameters are not really named/ordered properly, (and the example is
replacing a single test) it feels more like an implementation detail
to me.

> I realize that perfect is the enemy of good enough, and that there's value for humans
> to see these testcase results and manually interpret them, even if they are put into
> a syntax that automated parsers will ignore. However, I do think there's a danger that
> this syntax will get canonicalized. Personally, I'd rather see the testcases
> with parameters show up in the parsable results. I'd rather sacrifice the hierarchy
> than the results.

With the state of things at the moment, I don't think the individual
results for given parameters are as useful as the overall result for
the test (run over all parameters), so for me the hierarchy is more
important than the actual results. There are certainly a lot of things
we can do to make the results more useful (supporting named
parameters, for one), and actually supporting the extra level of
nesting in the tooling would make it possible to have both.

There is of course another possibility -- to just not print the
individual parameter results at all (the parameters will likely show
up in the assertion messages of failures anyway -- especially if, as
in the example, the _MSG() variants are used). That's probably
slightly easier to read than a huge list of parameters which are all
"ok" anyway...

In any case, I'm happy to leave the final decision here to Arpitha and
Marco, so long as we don't actually violate the TAP/KTAP spec and
kunit_tool is able to read at least the top-level result. My
preference is still to go either with the "# [test_case->name]:
[ok|not ok] [index] - param-[index]", or to get rid of the
per-parameter results entirely for now (or just print out a diagnostic
message on failure). In any case, it's a decision we can revisit once
we have support for named parameters, better tooling, or a better idea
of how people are actually using this.

Thanks,
-- David

2020-11-12 12:37:58

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Thu, Nov 12, 2020 at 04:18PM +0800, David Gow wrote:
> On Thu, Nov 12, 2020 at 12:55 AM Bird, Tim <[email protected]> wrote:
[...]
> > > kunit_tool has a bug when parsing the comments / diagnostic lines,
> > > which requires a ": " to be present. This is a bug, which is being
> > > fixed[1], so while it's _nice_ to not trigger it, it's not really an
> > > important long-term goal of the format. In any case, this kunit_tool
> > > issue only affects the comment lines: if the per-parameter result line
> > > is an actual result, rather than just a diagnostic, this shouldn't be
> > > a problem.
> > >
> > > In any case, I still prefer my proposed option for now -- noting that
> > > these per-parameter results are not actually supposed to be parsed --
> > > with then the possibility of expanding them to actual nested results
> > > later should we wish. But if the idea of having TAP-like lines in
> > > diagnostics seems too dangerous (e.g. because people will try to parse
> > > them anyway), then I think the options we have are to stick to the
> > > output format given in the current version of this patch (which
> > > doesn't resemble a TAP result), make each parameterised version its
> > > own test (without a "container test", which would require a bit of
> > > extra work while computing test plans), or to hold this whole feature
> > > back until we can support arbitrary test hierarchies in KUnit.
> > It seems like you're missing a 4th option, which is just tack the parameter
> > name on, without using a colon, and have these testcases treated
> > as unique within the context of the super-test. Is there some reason
> > these can't be expressed as individual testcases in the parent test?
> >
>
> No: there's no fundamental reason why we couldn't do that, though
> there are some things which make it suboptiomal, IMHO.
>
> Firstly, there could be a lot of parameters, and that by not grouping
> them together it could make dealing with the results a little
> unwieldy. The other side of that is that it'll result in tests being
> split up and renamed as they're ported to use this, whereas if the
> whole test shows up once (with subtests or without), the old test name
> can still be there, with a single PASS/FAIL for the whole test.

Agree, it's suboptimal and having the parameterized not be absorbed by
the whole suite would be strongly preferred.

> (It also might be a little tricky with the current implementation to
> produce the test plan, as the parameters come from a generator, and I
> don't think there's a way of getting the number of parameters ahead of
> time. That's a problem with the sub-subtest model, too, though at
> least there it's a little more isolated from other tests.)

The whole point of generators, as I envisage it, is to also provide the
ability for varying parameters dependent on e.g. environment,
configuration, number of CPUs, etc. The current array-based generator is
the simplest possible use-case.

However, we *can* require generators generate a deterministic number of
parameters when called multiple times on the same system.

To that end, I propose a v7 (below) that takes care of getting number of
parameters (and also displays descriptions for each parameter where
available).

Now it is up to you how you want to turn the output from diagnostic
lines into something TAP compliant, because now we have the number of
parameters and can turn it into a subsubtest. But I think kunit-tool
doesn't understand subsubtests yet, so I suggest we take these patches,
and then somebody can prepare kunit-tool.

Or did I miss something else?

> > > Personally, I'd rather not hold this feature back, and prefer to have
> > > a single combined result available, so would just stick with v6 if
> > > so...
> > >
> > > Does that make sense?
> >
> > I understand what you are saying, but this seems like a step backwards.
> > We already know that having just numbers to represent a test case is not
> > very human friendly. The same will go for these parameter case numbers.
> > I admit to not following this kunit test parameterization effort, so I don't
> > know the background of how the numbers relate to the parameters.
> > But there must be some actual semantic meaning to each of the parameter
> > cases. Not conveying that meaning as part of the test case name seems like
> > a missed opportunity.
>
> Yeah: I'm not a big fan of just numbering the parameters either: the
> plan is to eventually support naming these. Basically, the goal is to
> be able to run the same test code repeatedly with different inputs
> (which may be programmatically generated): depending on the testcase /
> parameter generator in question, the numbers may mean something
> specific, but it's not necessarily the case. Certainly in most cases,
> the order of these parameters is unlikely to matter, so having the
> number be part of the test name isn't ideal there: it's unlikely to
> have semantic meaning, and worst-case could be unstable due to code
> changes.

We can name them. Probably a good idea to do it while we can, as I think
the best design is changing the generator function signature.

> > I'm at a little of a loss as to why, if you have valid testcase results, you would
> > shy away from putting them into a format that is machine-readable. If it's because
> > the tooling is not there, then IMHO you should fix the tooling.
>
> I think the real disconnect here is the ambiguity between treating
> each run-through with a different parameter as its own test case,
> versus an implementation detail of the single "meta testcase". Since
> parameters are not really named/ordered properly, (and the example is
> replacing a single test) it feels more like an implementation detail
> to me.
>
> > I realize that perfect is the enemy of good enough, and that there's value for humans
> > to see these testcase results and manually interpret them, even if they are put into
> > a syntax that automated parsers will ignore. However, I do think there's a danger that
> > this syntax will get canonicalized. Personally, I'd rather see the testcases
> > with parameters show up in the parsable results. I'd rather sacrifice the hierarchy
> > than the results.
>
> With the state of things at the moment, I don't think the individual
> results for given parameters are as useful as the overall result for
> the test (run over all parameters), so for me the hierarchy is more
> important than the actual results. There are certainly a lot of things
> we can do to make the results more useful (supporting named
> parameters, for one), and actually supporting the extra level of
> nesting in the tooling would make it possible to have both.

Named parameters are supported in my proposed v7 (see below). I think
it's now up to you what the format should be, as now it's just a matter
of changing 2 prints' formats to some non-diagnostic TAP compliant
output (but I have no idea what ;-)).

> There is of course another possibility -- to just not print the
> individual parameter results at all (the parameters will likely show
> up in the assertion messages of failures anyway -- especially if, as
> in the example, the _MSG() variants are used). That's probably
> slightly easier to read than a huge list of parameters which are all
> "ok" anyway...

To me this is not acceptable, as I'd like to see some progress feedback
for long tests.

> In any case, I'm happy to leave the final decision here to Arpitha and
> Marco, so long as we don't actually violate the TAP/KTAP spec and
> kunit_tool is able to read at least the top-level result. My
> preference is still to go either with the "# [test_case->name]:
> [ok|not ok] [index] - param-[index]", or to get rid of the
> per-parameter results entirely for now (or just print out a diagnostic
> message on failure). In any case, it's a decision we can revisit once
> we have support for named parameters, better tooling, or a better idea
> of how people are actually using this.

Right, so I think we'll be in a better place if we implement: 1)
parameter to description conversion support, 2) counting parameters. So
I decided to see what it looks like, and it wasn't too bad. I just don't
know how you want to fix kunit-tool to make these non-diagnostic lines
and not complain, but as I said, it'd be good to not block these
patches.

It currently looks like this:

| TAP version 14
| 1..6
| # Subtest: ext4_inode_test
| 1..1
| # inode_test_xtimestamp_decoding: Test with 1..16 params
| # inode_test_xtimestamp_decoding: ok 1 - [1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits]
| # inode_test_xtimestamp_decoding: ok 2 - [1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits]
| # inode_test_xtimestamp_decoding: ok 3 - [1970-01-01 Lower bound of 32bit >=0 timestamp, no extra bits]
| # inode_test_xtimestamp_decoding: ok 4 - [2038-01-19 Upper bound of 32bit >=0 timestamp, no extra bits]
| # inode_test_xtimestamp_decoding: ok 5 - [2038-01-19 Lower bound of 32bit <0 timestamp, lo extra sec bit on]
| # inode_test_xtimestamp_decoding: ok 6 - [2106-02-07 Upper bound of 32bit <0 timestamp, lo extra sec bit on]
| # inode_test_xtimestamp_decoding: ok 7 - [2106-02-07 Lower bound of 32bit >=0 timestamp, lo extra sec bit on]
| # inode_test_xtimestamp_decoding: ok 8 - [2174-02-25 Upper bound of 32bit >=0 timestamp, lo extra sec bit on]
| # inode_test_xtimestamp_decoding: ok 9 - [2174-02-25 Lower bound of 32bit <0 timestamp, hi extra sec bit on]
| # inode_test_xtimestamp_decoding: ok 10 - [2242-03-16 Upper bound of 32bit <0 timestamp, hi extra sec bit on]
| # inode_test_xtimestamp_decoding: ok 11 - [2242-03-16 Lower bound of 32bit >=0 timestamp, hi extra sec bit on]
| # inode_test_xtimestamp_decoding: ok 12 - [2310-04-04 Upper bound of 32bit >=0 timestamp, hi extra sec bit on]
| # inode_test_xtimestamp_decoding: ok 13 - [2310-04-04 Upper bound of 32bit>=0 timestamp, hi extra sec bit 1. 1 ns]
| # inode_test_xtimestamp_decoding: ok 14 - [2378-04-22 Lower bound of 32bit>= timestamp. Extra sec bits 1. Max ns]
| # inode_test_xtimestamp_decoding: ok 15 - [2378-04-22 Lower bound of 32bit >=0 timestamp. All extra sec bits on]
| # inode_test_xtimestamp_decoding: ok 16 - [2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on]
| ok 1 - inode_test_xtimestamp_decoding
| ok 1 - ext4_inode_test

Changing the format of the 2 prints to something else TAP-compliant
should be easy enough once kunit-tool supports subsubtests. :-)

I hope this is a reasonable compromise for now.

[Arpitha: I suggest waiting a day or two and see what further comments
we get, and then take the 2 patches and send the v7 if we decide this is
what we want.]

Thanks,
-- Marco

------ >8 ------

From 31a77b18c874ed93f2d4f8b398b56f10e56bcfd9 Mon Sep 17 00:00:00 2001
From: Arpitha Raghunandan <[email protected]>
Date: Sat, 7 Nov 2020 00:51:54 +0530
Subject: [PATCH 1/2] kunit: Support for Parameterized Testing

Implementation of support for parameterized testing in KUnit. This
approach requires the creation of a test case using the
KUNIT_CASE_PARAM() macro that accepts a generator function as input.

This generator function should return the next parameter given the
previous parameter in parameterized tests. It also provides a macro to
generate common-case generators based on arrays. Generators may also
optionally provide a human-readable description of parameters, which is
displayed where available.

Note, currently the result of each parameter run is displayed in
diagnostic lines, and only the overall test case output summarizes
TAP-compliant success or failure of all parameter runs. In future, when
supported by kunit-tool, these can be turned into subsubtest outputs.

Signed-off-by: Arpitha Raghunandan <[email protected]>
Co-developed-by: Marco Elver <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
---
Changes v6->v7:
- Clarify commit message.
- Introduce ability to optionally generate descriptions for parameters;
if no description is provided, we'll still print 'param-N'.
- Change diagnostic line format to:
# <test-case-name>: <ok|not ok> N - [<param description>]
- Before execution of parameterized test case, count number of
parameters and display number of parameters. Currently also as a
diagnostic line, but this may be used in future to generate a subsubtest
plan. A requirement of this change is that generators must generate a
deterministic number of parameters.

Changes v5->v6:
- Fix alignment to maintain consistency

Changes v4->v5:
- Update kernel-doc comments.
- Use const void* for generator return and prev value types.
- Add kernel-doc comment for KUNIT_ARRAY_PARAM.
- Rework parameterized test case execution strategy: each parameter is executed
as if it was its own test case, with its own test initialization and cleanup
(init and exit are called, etc.). However, we cannot add new test cases per TAP
protocol once we have already started execution. Instead, log the result of
each parameter run as a diagnostic comment.

Changes v3->v4:
- Rename kunit variables
- Rename generator function helper macro
- Add documentation for generator approach
- Display test case name in case of failure along with param index

Changes v2->v3:
- Modifictaion of generator macro and method

Changes v1->v2:
- Use of a generator method to access test case parameters
---
include/kunit/test.h | 50 +++++++++++++++++++++++++
lib/kunit/test.c | 89 +++++++++++++++++++++++++++++++++++++-------
2 files changed, 126 insertions(+), 13 deletions(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 9197da792336..5935efb7b533 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -94,6 +94,9 @@ struct kunit;
/* Size of log associated with test. */
#define KUNIT_LOG_SIZE 512

+/* Maximum size of parameter description string. */
+#define KUNIT_PARAM_DESC_SIZE 64
+
/*
* TAP specifies subtest stream indentation of 4 spaces, 8 spaces for a
* sub-subtest. See the "Subtests" section in
@@ -107,6 +110,7 @@ struct kunit;
*
* @run_case: the function representing the actual test case.
* @name: the name of the test case.
+ * @generate_params: the generator function for parameterized tests.
*
* A test case is a function with the signature,
* ``void (*)(struct kunit *)``
@@ -141,6 +145,7 @@ struct kunit;
struct kunit_case {
void (*run_case)(struct kunit *test);
const char *name;
+ const void* (*generate_params)(const void *prev, char *desc);

/* private: internal use only. */
bool success;
@@ -163,6 +168,27 @@ static inline char *kunit_status_to_string(bool status)
*/
#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }

+/**
+ * KUNIT_CASE_PARAM - A helper for creation a parameterized &struct kunit_case
+ *
+ * @test_name: a reference to a test case function.
+ * @gen_params: a reference to a parameter generator function.
+ *
+ * The generator function::
+ *
+ * const void* gen_params(const void *prev, char *desc)
+ *
+ * is used to lazily generate a series of arbitrarily typed values that fit into
+ * a void*. The argument @prev is the previously returned value, which should be
+ * used to derive the next value; @prev is set to NULL on the initial generator
+ * call. When no more values are available, the generator must return NULL.
+ * Optionally write a string into @desc (size of KUNIT_PARAM_DESC_SIZE)
+ * describing the parameter.
+ */
+#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 +234,10 @@ struct kunit {
const char *name; /* Read only after initialization! */
char *log; /* Points at case log after initialization */
struct kunit_try_catch try_catch;
+ /* param_value is the current parameter value for a test case. */
+ const void *param_value;
+ /* param_index stores the index of the parameter in parameterized tests. */
+ int param_index;
/*
* 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 +1772,24 @@ do { \
fmt, \
##__VA_ARGS__)

+/**
+ * KUNIT_ARRAY_PARAM() - Define test parameter generator from an array.
+ * @name: prefix for the test parameter generator function.
+ * @array: array of test parameters.
+ * @get_desc: function to convert param to description; NULL to use default
+ *
+ * Define function @name_gen_params which uses @array to generate parameters.
+ */
+#define KUNIT_ARRAY_PARAM(name, array, get_desc) \
+ static const void *name##_gen_params(const void *prev, char *desc) \
+ { \
+ typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array); \
+ if (__next - (array) < ARRAY_SIZE((array))) { \
+ void (*__get_desc)(typeof(__next), char *) = get_desc; \
+ if (__get_desc) __get_desc(__next, desc); \
+ return __next; \
+ } \
+ return NULL; \
+ }
+
#endif /* _KUNIT_TEST_H */
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 750704abe89a..1a728bfc4da7 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -325,39 +325,102 @@ static void kunit_catch_run_case(void *data)
* occur in a test case and reports them as failures.
*/
static void kunit_run_case_catch_errors(struct kunit_suite *suite,
- struct kunit_case *test_case)
+ struct kunit_case *test_case,
+ struct kunit *test)
{
struct kunit_try_catch_context context;
struct kunit_try_catch *try_catch;
- struct kunit test;

- kunit_init_test(&test, test_case->name, test_case->log);
- try_catch = &test.try_catch;
+ kunit_init_test(test, test_case->name, test_case->log);
+ try_catch = &test->try_catch;

kunit_try_catch_init(try_catch,
- &test,
+ test,
kunit_try_run_case,
kunit_catch_run_case);
- context.test = &test;
+ context.test = test;
context.suite = suite;
context.test_case = test_case;
kunit_try_catch_run(try_catch, &context);

- test_case->success = test.success;
-
- kunit_print_ok_not_ok(&test, true, test_case->success,
- kunit_test_case_num(suite, test_case),
- test_case->name);
+ test_case->success = test->success;
}

int kunit_run_tests(struct kunit_suite *suite)
{
+ char param_desc[KUNIT_PARAM_DESC_SIZE];
struct kunit_case *test_case;

kunit_print_subtest_start(suite);

- kunit_suite_for_each_test_case(suite, test_case)
- kunit_run_case_catch_errors(suite, test_case);
+ kunit_suite_for_each_test_case(suite, test_case) {
+ struct kunit test = { .param_value = NULL, .param_index = 0 };
+ bool test_success = true;
+ int num_params = 0;
+
+ if (test_case->generate_params) {
+ const void *val; /* Only for counting params. */
+
+ /*
+ * Count number of params: requires that param
+ * generators are deterministic in number of params
+ * generated. Always execute at least 1 param:
+ * - 1 or more non-NULL params;
+ * - special case: 1 param which may be NULL.
+ */
+ val = test_case->generate_params(NULL, param_desc);
+ for (num_params = val ? 0 : 1; val; num_params++) {
+ val = test_case->generate_params(val, param_desc);
+ }
+
+ kunit_log(KERN_INFO, &test,
+ KUNIT_SUBTEST_INDENT
+ "# %s: Test with 1..%d params",
+ test_case->name, num_params);
+
+ /* Get initial param. */
+ param_desc[0] = '\0';
+ test.param_value = test_case->generate_params(NULL, param_desc);
+ }
+
+ do {
+ kunit_run_case_catch_errors(suite, test_case, &test);
+ test_success &= test_case->success;
+
+ if (test_case->generate_params) {
+ if (param_desc[0] == '\0') {
+ snprintf(param_desc, sizeof(param_desc),
+ "param-%d", test.param_index);
+ }
+
+ kunit_log(KERN_INFO, &test,
+ KUNIT_SUBTEST_INDENT
+ "# %s: %s %d - [%s]",
+ test_case->name,
+ kunit_status_to_string(test.success),
+ test.param_index + 1, param_desc);
+
+ /* Get next param. */
+ param_desc[0] = '\0';
+ test.param_value = test_case->generate_params(test.param_value, param_desc);
+ test.param_index++;
+
+ /* Assert deterministic number of params. */
+ num_params--;
+ if (!!test.param_value != !!num_params) {
+ kunit_log(KERN_INFO, &test,
+ KUNIT_SUBTEST_INDENT
+ "# %s: Non-deterministic number of params",
+ test_case->name);
+ test_success = false;
+ }
+ }
+ } while (test.param_value);
+
+ kunit_print_ok_not_ok(&test, true, test_success,
+ kunit_test_case_num(suite, test_case),
+ test_case->name);
+ }

kunit_print_subtest_end(suite);

--
2.29.2.222.g5d2a92d10f8-goog


From 1705948e9d1e1095ee56737a6d7a5c6342cdb5ee Mon Sep 17 00:00:00 2001
From: Arpitha Raghunandan <[email protected]>
Date: Sat, 7 Nov 2020 00:52:49 +0530
Subject: [PATCH 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized
testing feature

Modify fs/ext4/inode-test.c to use the parameterized testing
feature of KUnit.

Signed-off-by: Arpitha Raghunandan <[email protected]>
Signed-off-by: Marco Elver <[email protected]>
---
Changes v6->v7:
- Introduce timestamp_expectation_to_desc() to convert param to
description.
Changes v5->v6:
- No change to this patch of the patch series
Changes v4->v5:
- No change to this patch of the patch series
Changes v3->v4:
- Modification based on latest implementation of KUnit parameterized testing
Changes v2->v3:
- Marked hardcoded test data const
- Modification based on latest implementation of KUnit parameterized testing
Changes v1->v2:
- Modification based on latest implementation of KUnit parameterized testing
---
fs/ext4/inode-test.c | 320 ++++++++++++++++++++++---------------------
1 file changed, 164 insertions(+), 156 deletions(-)

diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
index d62d802c9c12..97390ab13681 100644
--- a/fs/ext4/inode-test.c
+++ b/fs/ext4/inode-test.c
@@ -80,6 +80,145 @@ struct timestamp_expectation {
bool lower_bound;
};

+static 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},
+ }
+};
+
+static void timestamp_expectation_to_desc(const struct timestamp_expectation *t,
+ char *desc)
+{
+ strcpy(desc, t->test_case_name);
+}
+
+KUNIT_ARRAY_PARAM(ext4_inode, test_data, timestamp_expectation_to_desc);
+
static time64_t get_32bit_time(const struct timestamp_expectation * const test)
{
if (test->msb_set) {
@@ -101,166 +240,35 @@ 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(&timestamp,
- 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_value);
+
+ timestamp.tv_sec = get_32bit_time(test_param);
+ ext4_decode_extra_time(&timestamp,
+ 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 struct kunit_case ext4_inode_test_cases[] = {
- KUNIT_CASE(inode_test_xtimestamp_decoding),
+ KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding, ext4_inode_gen_params),
{}
};

--
2.29.2.222.g5d2a92d10f8-goog

2020-11-13 05:19:51

by David Gow

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Thu, Nov 12, 2020 at 8:37 PM Marco Elver <[email protected]> wrote:
>
> On Thu, Nov 12, 2020 at 04:18PM +0800, David Gow wrote:
> > On Thu, Nov 12, 2020 at 12:55 AM Bird, Tim <[email protected]> wrote:
> [...]
> > > > kunit_tool has a bug when parsing the comments / diagnostic lines,
> > > > which requires a ": " to be present. This is a bug, which is being
> > > > fixed[1], so while it's _nice_ to not trigger it, it's not really an
> > > > important long-term goal of the format. In any case, this kunit_tool
> > > > issue only affects the comment lines: if the per-parameter result line
> > > > is an actual result, rather than just a diagnostic, this shouldn't be
> > > > a problem.
> > > >
> > > > In any case, I still prefer my proposed option for now -- noting that
> > > > these per-parameter results are not actually supposed to be parsed --
> > > > with then the possibility of expanding them to actual nested results
> > > > later should we wish. But if the idea of having TAP-like lines in
> > > > diagnostics seems too dangerous (e.g. because people will try to parse
> > > > them anyway), then I think the options we have are to stick to the
> > > > output format given in the current version of this patch (which
> > > > doesn't resemble a TAP result), make each parameterised version its
> > > > own test (without a "container test", which would require a bit of
> > > > extra work while computing test plans), or to hold this whole feature
> > > > back until we can support arbitrary test hierarchies in KUnit.
> > > It seems like you're missing a 4th option, which is just tack the parameter
> > > name on, without using a colon, and have these testcases treated
> > > as unique within the context of the super-test. Is there some reason
> > > these can't be expressed as individual testcases in the parent test?
> > >
> >
> > No: there's no fundamental reason why we couldn't do that, though
> > there are some things which make it suboptiomal, IMHO.
> >
> > Firstly, there could be a lot of parameters, and that by not grouping
> > them together it could make dealing with the results a little
> > unwieldy. The other side of that is that it'll result in tests being
> > split up and renamed as they're ported to use this, whereas if the
> > whole test shows up once (with subtests or without), the old test name
> > can still be there, with a single PASS/FAIL for the whole test.
>
> Agree, it's suboptimal and having the parameterized not be absorbed by
> the whole suite would be strongly preferred.
>
> > (It also might be a little tricky with the current implementation to
> > produce the test plan, as the parameters come from a generator, and I
> > don't think there's a way of getting the number of parameters ahead of
> > time. That's a problem with the sub-subtest model, too, though at
> > least there it's a little more isolated from other tests.)
>
> The whole point of generators, as I envisage it, is to also provide the
> ability for varying parameters dependent on e.g. environment,
> configuration, number of CPUs, etc. The current array-based generator is
> the simplest possible use-case.
>
> However, we *can* require generators generate a deterministic number of
> parameters when called multiple times on the same system.

I think this is a reasonable compromise, though it's not actually
essential. As I understand the TAP spec, the test plan is actually
optional (and/or can be at the end of the sequence of tests), though
kunit_tool currently only supports having it at the beginning (which
is strongly preferred by the spec anyway). I think we could get away
with having it at the bottom of the subtest results though, which
would save having to run the generator twice, when subtest support is
added to kunit_tool.

> To that end, I propose a v7 (below) that takes care of getting number of
> parameters (and also displays descriptions for each parameter where
> available).
>
> Now it is up to you how you want to turn the output from diagnostic
> lines into something TAP compliant, because now we have the number of
> parameters and can turn it into a subsubtest. But I think kunit-tool
> doesn't understand subsubtests yet, so I suggest we take these patches,
> and then somebody can prepare kunit-tool.
>

This sounds good to me. The only thing I'm not sure about is the
format of the parameter description: thus far test names be valid C
identifier names, due to the fact they're named after the test
function. I don't think there's a fundamental reason parameters (and
hence, potentially, subsubtests) need to follow that convention as
well, but it does look a bit odd. Equally, the square brackets around
the description shouldn't be necessary according to the TAP spec, but
do seem to make things a little more readable, particuarly with the
names in the ext4 inode test. I'm not too worried about either of
those, though: I'm sure it'll look fine once I've got used to it.



> Or did I miss something else?
>
> > > > Personally, I'd rather not hold this feature back, and prefer to have
> > > > a single combined result available, so would just stick with v6 if
> > > > so...
> > > >
> > > > Does that make sense?
> > >
> > > I understand what you are saying, but this seems like a step backwards.
> > > We already know that having just numbers to represent a test case is not
> > > very human friendly. The same will go for these parameter case numbers.
> > > I admit to not following this kunit test parameterization effort, so I don't
> > > know the background of how the numbers relate to the parameters.
> > > But there must be some actual semantic meaning to each of the parameter
> > > cases. Not conveying that meaning as part of the test case name seems like
> > > a missed opportunity.
> >
> > Yeah: I'm not a big fan of just numbering the parameters either: the
> > plan is to eventually support naming these. Basically, the goal is to
> > be able to run the same test code repeatedly with different inputs
> > (which may be programmatically generated): depending on the testcase /
> > parameter generator in question, the numbers may mean something
> > specific, but it's not necessarily the case. Certainly in most cases,
> > the order of these parameters is unlikely to matter, so having the
> > number be part of the test name isn't ideal there: it's unlikely to
> > have semantic meaning, and worst-case could be unstable due to code
> > changes.
>
> We can name them. Probably a good idea to do it while we can, as I think
> the best design is changing the generator function signature.
>

This works for me. I was thinking of having a separate
parameter_to_string() function, but I think this is better.

> > > I'm at a little of a loss as to why, if you have valid testcase results, you would
> > > shy away from putting them into a format that is machine-readable. If it's because
> > > the tooling is not there, then IMHO you should fix the tooling.
> >
> > I think the real disconnect here is the ambiguity between treating
> > each run-through with a different parameter as its own test case,
> > versus an implementation detail of the single "meta testcase". Since
> > parameters are not really named/ordered properly, (and the example is
> > replacing a single test) it feels more like an implementation detail
> > to me.
> >
> > > I realize that perfect is the enemy of good enough, and that there's value for humans
> > > to see these testcase results and manually interpret them, even if they are put into
> > > a syntax that automated parsers will ignore. However, I do think there's a danger that
> > > this syntax will get canonicalized. Personally, I'd rather see the testcases
> > > with parameters show up in the parsable results. I'd rather sacrifice the hierarchy
> > > than the results.
> >
> > With the state of things at the moment, I don't think the individual
> > results for given parameters are as useful as the overall result for
> > the test (run over all parameters), so for me the hierarchy is more
> > important than the actual results. There are certainly a lot of things
> > we can do to make the results more useful (supporting named
> > parameters, for one), and actually supporting the extra level of
> > nesting in the tooling would make it possible to have both.
>
> Named parameters are supported in my proposed v7 (see below). I think
> it's now up to you what the format should be, as now it's just a matter
> of changing 2 prints' formats to some non-diagnostic TAP compliant
> output (but I have no idea what ;-)).

Thanks -- I think the changes from this v7 to a TAP compliant format
should mostly just be replacing the "# [test_name]" with an extra
level of indentation. We'll just have to add support for it to
kunit_tool's parser (which has been in need of work anyway.)

> > There is of course another possibility -- to just not print the
> > individual parameter results at all (the parameters will likely show
> > up in the assertion messages of failures anyway -- especially if, as
> > in the example, the _MSG() variants are used). That's probably
> > slightly easier to read than a huge list of parameters which are all
> > "ok" anyway...
>
> To me this is not acceptable, as I'd like to see some progress feedback
> for long tests.

Hopefully most tests don't take long enough that this level of
feedback is necessary (and those that do could always log diagnostic
lines themselves), but I'm also happy with these being printed out
here.

> > In any case, I'm happy to leave the final decision here to Arpitha and
> > Marco, so long as we don't actually violate the TAP/KTAP spec and
> > kunit_tool is able to read at least the top-level result. My
> > preference is still to go either with the "# [test_case->name]:
> > [ok|not ok] [index] - param-[index]", or to get rid of the
> > per-parameter results entirely for now (or just print out a diagnostic
> > message on failure). In any case, it's a decision we can revisit once
> > we have support for named parameters, better tooling, or a better idea
> > of how people are actually using this.
>
> Right, so I think we'll be in a better place if we implement: 1)
> parameter to description conversion support, 2) counting parameters. So
> I decided to see what it looks like, and it wasn't too bad. I just don't
> know how you want to fix kunit-tool to make these non-diagnostic lines
> and not complain, but as I said, it'd be good to not block these
> patches.

Yup, I tried this v7, and it looks good to me. The kunit_tool work
will probably be a touch more involved, so I definitely don't want to
hold up supporting this on that.

My only thoughts on the v7 patch are:
- I don't think we actually need the parameter count yet (or perhaps
ever if we go with subtests as planned), so I be okay with getting rid
of that.
- It'd be a possibility to get rid of the square brackets from the
output, and if we still want them, make them part of the test itself:
if this were TAP formatted, those brackets would be part of the
subsubtest name.

>
> It currently looks like this:
>
> | TAP version 14
> | 1..6
> | # Subtest: ext4_inode_test
> | 1..1
> | # inode_test_xtimestamp_decoding: Test with 1..16 params
> | # inode_test_xtimestamp_decoding: ok 1 - [1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits]
> | # inode_test_xtimestamp_decoding: ok 2 - [1969-12-31 Upper bound of 32bit < 0 timestamp, no extra bits]
> | # inode_test_xtimestamp_decoding: ok 3 - [1970-01-01 Lower bound of 32bit >=0 timestamp, no extra bits]
> | # inode_test_xtimestamp_decoding: ok 4 - [2038-01-19 Upper bound of 32bit >=0 timestamp, no extra bits]
> | # inode_test_xtimestamp_decoding: ok 5 - [2038-01-19 Lower bound of 32bit <0 timestamp, lo extra sec bit on]
> | # inode_test_xtimestamp_decoding: ok 6 - [2106-02-07 Upper bound of 32bit <0 timestamp, lo extra sec bit on]
> | # inode_test_xtimestamp_decoding: ok 7 - [2106-02-07 Lower bound of 32bit >=0 timestamp, lo extra sec bit on]
> | # inode_test_xtimestamp_decoding: ok 8 - [2174-02-25 Upper bound of 32bit >=0 timestamp, lo extra sec bit on]
> | # inode_test_xtimestamp_decoding: ok 9 - [2174-02-25 Lower bound of 32bit <0 timestamp, hi extra sec bit on]
> | # inode_test_xtimestamp_decoding: ok 10 - [2242-03-16 Upper bound of 32bit <0 timestamp, hi extra sec bit on]
> | # inode_test_xtimestamp_decoding: ok 11 - [2242-03-16 Lower bound of 32bit >=0 timestamp, hi extra sec bit on]
> | # inode_test_xtimestamp_decoding: ok 12 - [2310-04-04 Upper bound of 32bit >=0 timestamp, hi extra sec bit on]
> | # inode_test_xtimestamp_decoding: ok 13 - [2310-04-04 Upper bound of 32bit>=0 timestamp, hi extra sec bit 1. 1 ns]
> | # inode_test_xtimestamp_decoding: ok 14 - [2378-04-22 Lower bound of 32bit>= timestamp. Extra sec bits 1. Max ns]
> | # inode_test_xtimestamp_decoding: ok 15 - [2378-04-22 Lower bound of 32bit >=0 timestamp. All extra sec bits on]
> | # inode_test_xtimestamp_decoding: ok 16 - [2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on]
> | ok 1 - inode_test_xtimestamp_decoding
> | ok 1 - ext4_inode_test
>
> Changing the format of the 2 prints to something else TAP-compliant
> should be easy enough once kunit-tool supports subsubtests. :-)
>
> I hope this is a reasonable compromise for now.

Yeah: this seems like a great compromise until kunit_tool is improved.

Thanks a bunch,
-- David

2020-11-13 22:40:35

by David Gow

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Fri, Nov 13, 2020 at 6:31 PM Marco Elver <[email protected]> wrote:
>
> On Fri, Nov 13, 2020 at 01:17PM +0800, David Gow wrote:
> > On Thu, Nov 12, 2020 at 8:37 PM Marco Elver <[email protected]> wrote:
> [...]
> > > > (It also might be a little tricky with the current implementation to
> > > > produce the test plan, as the parameters come from a generator, and I
> > > > don't think there's a way of getting the number of parameters ahead of
> > > > time. That's a problem with the sub-subtest model, too, though at
> > > > least there it's a little more isolated from other tests.)
> > >
> > > The whole point of generators, as I envisage it, is to also provide the
> > > ability for varying parameters dependent on e.g. environment,
> > > configuration, number of CPUs, etc. The current array-based generator is
> > > the simplest possible use-case.
> > >
> > > However, we *can* require generators generate a deterministic number of
> > > parameters when called multiple times on the same system.
> >
> > I think this is a reasonable compromise, though it's not actually
> > essential. As I understand the TAP spec, the test plan is actually
> > optional (and/or can be at the end of the sequence of tests), though
> > kunit_tool currently only supports having it at the beginning (which
> > is strongly preferred by the spec anyway). I think we could get away
> > with having it at the bottom of the subtest results though, which
> > would save having to run the generator twice, when subtest support is
> > added to kunit_tool.
>
> I can't find this in the TAP spec, where should I look? Perhaps we
> shouldn't venture too far off the beaten path, given we might not be the
> only ones that want to parse this output.
>

It's in the "Test Lines and the Plan" section:
"The plan is optional but if there is a plan before the test points it
must be the first non-diagnostic line output by the test file. In
certain instances a test file may not know how many test points it
will ultimately be running. In this case the plan can be the last
non-diagnostic line in the output. The plan cannot appear in the
middle of the output, nor can it appear more than once."

My only concern with running through the generator multiple times to
get the count is that it might be slow and/or more difficult if
someone uses a more complicated generator. I can't think of anything
specific yet, though, so we can always do it for now and change it
later if a problematic case occurs.

> > > To that end, I propose a v7 (below) that takes care of getting number of
> > > parameters (and also displays descriptions for each parameter where
> > > available).
> > >
> > > Now it is up to you how you want to turn the output from diagnostic
> > > lines into something TAP compliant, because now we have the number of
> > > parameters and can turn it into a subsubtest. But I think kunit-tool
> > > doesn't understand subsubtests yet, so I suggest we take these patches,
> > > and then somebody can prepare kunit-tool.
> > >
> >
> > This sounds good to me. The only thing I'm not sure about is the
> > format of the parameter description: thus far test names be valid C
> > identifier names, due to the fact they're named after the test
> > function. I don't think there's a fundamental reason parameters (and
> > hence, potentially, subsubtests) need to follow that convention as
> > well, but it does look a bit odd. Equally, the square brackets around
> > the description shouldn't be necessary according to the TAP spec, but
> > do seem to make things a little more readable, particuarly with the
> > names in the ext4 inode test. I'm not too worried about either of
> > those, though: I'm sure it'll look fine once I've got used to it.
>
> The parameter description doesn't need to be a C identifier. At least
> that's what I could immediately glean from TAP v13 spec (I'm looking
> here: https://testanything.org/tap-version-13-specification.html and see
> e.g. "ok 1 - Input file opened" ...).
>

Yeah: it looked a bit weird for everything else to be an identifier
(given that KUnit does require it for tests), but these parameter
descriptions not to be. It's not a problem, though, so let's go ahead
with it.

> [...]
> > > > In any case, I'm happy to leave the final decision here to Arpitha and
> > > > Marco, so long as we don't actually violate the TAP/KTAP spec and
> > > > kunit_tool is able to read at least the top-level result. My
> > > > preference is still to go either with the "# [test_case->name]:
> > > > [ok|not ok] [index] - param-[index]", or to get rid of the
> > > > per-parameter results entirely for now (or just print out a diagnostic
> > > > message on failure). In any case, it's a decision we can revisit once
> > > > we have support for named parameters, better tooling, or a better idea
> > > > of how people are actually using this.
> > >
> > > Right, so I think we'll be in a better place if we implement: 1)
> > > parameter to description conversion support, 2) counting parameters. So
> > > I decided to see what it looks like, and it wasn't too bad. I just don't
> > > know how you want to fix kunit-tool to make these non-diagnostic lines
> > > and not complain, but as I said, it'd be good to not block these
> > > patches.
> >
> > Yup, I tried this v7, and it looks good to me. The kunit_tool work
> > will probably be a touch more involved, so I definitely don't want to
> > hold up supporting this on that.
> >
> > My only thoughts on the v7 patch are:
> > - I don't think we actually need the parameter count yet (or perhaps
> > ever if we go with subtests as planned), so I be okay with getting rid
> > of that.
>
> As noted above, perhaps we should keep it for compatibility with other
> parsers and CI systems we don't have much control over. It'd be a shame
> if 99% of KUnit output can be parsed by some partially compliant parser,
> yet this would break it.

KUnit has only started providing the test plans in some cases pretty
recently, and the spec does make it optional, so I'm not particularly
worried about this breaking parsers. I'm not too worried about it
causing problems to have it either, though, so if you'd rather keep
it, that's fine by me as well.

> > - It'd be a possibility to get rid of the square brackets from the
> > output, and if we still want them, make them part of the test itself:
> > if this were TAP formatted, those brackets would be part of the
> > subsubtest name.
>
> I don't mind. It's just that we can't prescribe a format, and as
> seen below the descriptions include characters -<>=,. which can be
> confusing. But perhaps you're right, so let's remove them.
>
> But as noted, TAP doesn't seem to care. So let's remove them.
>

Yeah: I have a slight preference for removing them, as TAP parsers
would otherwise include them in the parameter name, which looks a
little weird.
Of course, the point is moot until we actually fix kunit_tool and make
these subtests, so there's no fundamental reason we couldn't leave
them in for now, and remove them then if you thought it was
significantly more readable. (Personally, I'd still err on the side of
removing them to avoid any unnecessary churn.)

Cheers,
-- David

2020-11-14 00:16:28

by Marco Elver

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Fri, 13 Nov 2020 at 23:37, David Gow <[email protected]> wrote:
>
> On Fri, Nov 13, 2020 at 6:31 PM Marco Elver <[email protected]> wrote:
> >
> > On Fri, Nov 13, 2020 at 01:17PM +0800, David Gow wrote:
> > > On Thu, Nov 12, 2020 at 8:37 PM Marco Elver <[email protected]> wrote:
> > [...]
> > > > > (It also might be a little tricky with the current implementation to
> > > > > produce the test plan, as the parameters come from a generator, and I
> > > > > don't think there's a way of getting the number of parameters ahead of
> > > > > time. That's a problem with the sub-subtest model, too, though at
> > > > > least there it's a little more isolated from other tests.)
> > > >
> > > > The whole point of generators, as I envisage it, is to also provide the
> > > > ability for varying parameters dependent on e.g. environment,
> > > > configuration, number of CPUs, etc. The current array-based generator is
> > > > the simplest possible use-case.
> > > >
> > > > However, we *can* require generators generate a deterministic number of
> > > > parameters when called multiple times on the same system.
> > >
> > > I think this is a reasonable compromise, though it's not actually
> > > essential. As I understand the TAP spec, the test plan is actually
> > > optional (and/or can be at the end of the sequence of tests), though
> > > kunit_tool currently only supports having it at the beginning (which
> > > is strongly preferred by the spec anyway). I think we could get away
> > > with having it at the bottom of the subtest results though, which
> > > would save having to run the generator twice, when subtest support is
> > > added to kunit_tool.
> >
> > I can't find this in the TAP spec, where should I look? Perhaps we
> > shouldn't venture too far off the beaten path, given we might not be the
> > only ones that want to parse this output.
> >
>
> It's in the "Test Lines and the Plan" section:
> "The plan is optional but if there is a plan before the test points it
> must be the first non-diagnostic line output by the test file. In
> certain instances a test file may not know how many test points it
> will ultimately be running. In this case the plan can be the last
> non-diagnostic line in the output. The plan cannot appear in the
> middle of the output, nor can it appear more than once."

Ah, that's fine then.

> My only concern with running through the generator multiple times to
> get the count is that it might be slow and/or more difficult if
> someone uses a more complicated generator. I can't think of anything
> specific yet, though, so we can always do it for now and change it
> later if a problematic case occurs.

I'm all for simplicity, so if nobody objects, let's just get rid of
the number of parameters and avoid running it twice.

> > > > To that end, I propose a v7 (below) that takes care of getting number of
> > > > parameters (and also displays descriptions for each parameter where
> > > > available).
> > > >
> > > > Now it is up to you how you want to turn the output from diagnostic
> > > > lines into something TAP compliant, because now we have the number of
> > > > parameters and can turn it into a subsubtest. But I think kunit-tool
> > > > doesn't understand subsubtests yet, so I suggest we take these patches,
> > > > and then somebody can prepare kunit-tool.
> > > >
> > >
> > > This sounds good to me. The only thing I'm not sure about is the
> > > format of the parameter description: thus far test names be valid C
> > > identifier names, due to the fact they're named after the test
> > > function. I don't think there's a fundamental reason parameters (and
> > > hence, potentially, subsubtests) need to follow that convention as
> > > well, but it does look a bit odd. Equally, the square brackets around
> > > the description shouldn't be necessary according to the TAP spec, but
> > > do seem to make things a little more readable, particuarly with the
> > > names in the ext4 inode test. I'm not too worried about either of
> > > those, though: I'm sure it'll look fine once I've got used to it.
> >
> > The parameter description doesn't need to be a C identifier. At least
> > that's what I could immediately glean from TAP v13 spec (I'm looking
> > here: https://testanything.org/tap-version-13-specification.html and see
> > e.g. "ok 1 - Input file opened" ...).
> >
>
> Yeah: it looked a bit weird for everything else to be an identifier
> (given that KUnit does require it for tests), but these parameter
> descriptions not to be. It's not a problem, though, so let's go ahead
> with it.
>
> > [...]
> > > > > In any case, I'm happy to leave the final decision here to Arpitha and
> > > > > Marco, so long as we don't actually violate the TAP/KTAP spec and
> > > > > kunit_tool is able to read at least the top-level result. My
> > > > > preference is still to go either with the "# [test_case->name]:
> > > > > [ok|not ok] [index] - param-[index]", or to get rid of the
> > > > > per-parameter results entirely for now (or just print out a diagnostic
> > > > > message on failure). In any case, it's a decision we can revisit once
> > > > > we have support for named parameters, better tooling, or a better idea
> > > > > of how people are actually using this.
> > > >
> > > > Right, so I think we'll be in a better place if we implement: 1)
> > > > parameter to description conversion support, 2) counting parameters. So
> > > > I decided to see what it looks like, and it wasn't too bad. I just don't
> > > > know how you want to fix kunit-tool to make these non-diagnostic lines
> > > > and not complain, but as I said, it'd be good to not block these
> > > > patches.
> > >
> > > Yup, I tried this v7, and it looks good to me. The kunit_tool work
> > > will probably be a touch more involved, so I definitely don't want to
> > > hold up supporting this on that.
> > >
> > > My only thoughts on the v7 patch are:
> > > - I don't think we actually need the parameter count yet (or perhaps
> > > ever if we go with subtests as planned), so I be okay with getting rid
> > > of that.
> >
> > As noted above, perhaps we should keep it for compatibility with other
> > parsers and CI systems we don't have much control over. It'd be a shame
> > if 99% of KUnit output can be parsed by some partially compliant parser,
> > yet this would break it.
>
> KUnit has only started providing the test plans in some cases pretty
> recently, and the spec does make it optional, so I'm not particularly
> worried about this breaking parsers. I'm not too worried about it
> causing problems to have it either, though, so if you'd rather keep
> it, that's fine by me as well.
>
> > > - It'd be a possibility to get rid of the square brackets from the
> > > output, and if we still want them, make them part of the test itself:
> > > if this were TAP formatted, those brackets would be part of the
> > > subsubtest name.
> >
> > I don't mind. It's just that we can't prescribe a format, and as
> > seen below the descriptions include characters -<>=,. which can be
> > confusing. But perhaps you're right, so let's remove them.
> >
> > But as noted, TAP doesn't seem to care. So let's remove them.
> >
>
> Yeah: I have a slight preference for removing them, as TAP parsers
> would otherwise include them in the parameter name, which looks a
> little weird.
> Of course, the point is moot until we actually fix kunit_tool and make
> these subtests, so there's no fundamental reason we couldn't leave
> them in for now, and remove them then if you thought it was
> significantly more readable. (Personally, I'd still err on the side of
> removing them to avoid any unnecessary churn.)

Sounds good.

Arpitha: Do you want to send v7, but with the following modifications
from what I proposed? Assuming nobody objects.

1. Remove the num_params counter and don't print the number of params
anymore, nor do validation that generators are deterministic.
2. Remove the [].
[ I'm happy to send as well, just let me know what you prefer. ]

Thanks,
-- Marco

2020-11-14 01:40:03

by Arpitha Raghunandan

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On 14/11/20 5:44 am, Marco Elver wrote:
> On Fri, 13 Nov 2020 at 23:37, David Gow <[email protected]> wrote:
>>
>> On Fri, Nov 13, 2020 at 6:31 PM Marco Elver <[email protected]> wrote:
>>>
>>> On Fri, Nov 13, 2020 at 01:17PM +0800, David Gow wrote:
>>>> On Thu, Nov 12, 2020 at 8:37 PM Marco Elver <[email protected]> wrote:
>>> [...]
>>>>>> (It also might be a little tricky with the current implementation to
>>>>>> produce the test plan, as the parameters come from a generator, and I
>>>>>> don't think there's a way of getting the number of parameters ahead of
>>>>>> time. That's a problem with the sub-subtest model, too, though at
>>>>>> least there it's a little more isolated from other tests.)
>>>>>
>>>>> The whole point of generators, as I envisage it, is to also provide the
>>>>> ability for varying parameters dependent on e.g. environment,
>>>>> configuration, number of CPUs, etc. The current array-based generator is
>>>>> the simplest possible use-case.
>>>>>
>>>>> However, we *can* require generators generate a deterministic number of
>>>>> parameters when called multiple times on the same system.
>>>>
>>>> I think this is a reasonable compromise, though it's not actually
>>>> essential. As I understand the TAP spec, the test plan is actually
>>>> optional (and/or can be at the end of the sequence of tests), though
>>>> kunit_tool currently only supports having it at the beginning (which
>>>> is strongly preferred by the spec anyway). I think we could get away
>>>> with having it at the bottom of the subtest results though, which
>>>> would save having to run the generator twice, when subtest support is
>>>> added to kunit_tool.
>>>
>>> I can't find this in the TAP spec, where should I look? Perhaps we
>>> shouldn't venture too far off the beaten path, given we might not be the
>>> only ones that want to parse this output.
>>>
>>
>> It's in the "Test Lines and the Plan" section:
>> "The plan is optional but if there is a plan before the test points it
>> must be the first non-diagnostic line output by the test file. In
>> certain instances a test file may not know how many test points it
>> will ultimately be running. In this case the plan can be the last
>> non-diagnostic line in the output. The plan cannot appear in the
>> middle of the output, nor can it appear more than once."
>
> Ah, that's fine then.
>
>> My only concern with running through the generator multiple times to
>> get the count is that it might be slow and/or more difficult if
>> someone uses a more complicated generator. I can't think of anything
>> specific yet, though, so we can always do it for now and change it
>> later if a problematic case occurs.
>
> I'm all for simplicity, so if nobody objects, let's just get rid of
> the number of parameters and avoid running it twice.
>
>>>>> To that end, I propose a v7 (below) that takes care of getting number of
>>>>> parameters (and also displays descriptions for each parameter where
>>>>> available).
>>>>>
>>>>> Now it is up to you how you want to turn the output from diagnostic
>>>>> lines into something TAP compliant, because now we have the number of
>>>>> parameters and can turn it into a subsubtest. But I think kunit-tool
>>>>> doesn't understand subsubtests yet, so I suggest we take these patches,
>>>>> and then somebody can prepare kunit-tool.
>>>>>
>>>>
>>>> This sounds good to me. The only thing I'm not sure about is the
>>>> format of the parameter description: thus far test names be valid C
>>>> identifier names, due to the fact they're named after the test
>>>> function. I don't think there's a fundamental reason parameters (and
>>>> hence, potentially, subsubtests) need to follow that convention as
>>>> well, but it does look a bit odd. Equally, the square brackets around
>>>> the description shouldn't be necessary according to the TAP spec, but
>>>> do seem to make things a little more readable, particuarly with the
>>>> names in the ext4 inode test. I'm not too worried about either of
>>>> those, though: I'm sure it'll look fine once I've got used to it.
>>>
>>> The parameter description doesn't need to be a C identifier. At least
>>> that's what I could immediately glean from TAP v13 spec (I'm looking
>>> here: https://testanything.org/tap-version-13-specification.html and see
>>> e.g. "ok 1 - Input file opened" ...).
>>>
>>
>> Yeah: it looked a bit weird for everything else to be an identifier
>> (given that KUnit does require it for tests), but these parameter
>> descriptions not to be. It's not a problem, though, so let's go ahead
>> with it.
>>
>>> [...]
>>>>>> In any case, I'm happy to leave the final decision here to Arpitha and
>>>>>> Marco, so long as we don't actually violate the TAP/KTAP spec and
>>>>>> kunit_tool is able to read at least the top-level result. My
>>>>>> preference is still to go either with the "# [test_case->name]:
>>>>>> [ok|not ok] [index] - param-[index]", or to get rid of the
>>>>>> per-parameter results entirely for now (or just print out a diagnostic
>>>>>> message on failure). In any case, it's a decision we can revisit once
>>>>>> we have support for named parameters, better tooling, or a better idea
>>>>>> of how people are actually using this.
>>>>>
>>>>> Right, so I think we'll be in a better place if we implement: 1)
>>>>> parameter to description conversion support, 2) counting parameters. So
>>>>> I decided to see what it looks like, and it wasn't too bad. I just don't
>>>>> know how you want to fix kunit-tool to make these non-diagnostic lines
>>>>> and not complain, but as I said, it'd be good to not block these
>>>>> patches.
>>>>
>>>> Yup, I tried this v7, and it looks good to me. The kunit_tool work
>>>> will probably be a touch more involved, so I definitely don't want to
>>>> hold up supporting this on that.
>>>>
>>>> My only thoughts on the v7 patch are:
>>>> - I don't think we actually need the parameter count yet (or perhaps
>>>> ever if we go with subtests as planned), so I be okay with getting rid
>>>> of that.
>>>
>>> As noted above, perhaps we should keep it for compatibility with other
>>> parsers and CI systems we don't have much control over. It'd be a shame
>>> if 99% of KUnit output can be parsed by some partially compliant parser,
>>> yet this would break it.
>>
>> KUnit has only started providing the test plans in some cases pretty
>> recently, and the spec does make it optional, so I'm not particularly
>> worried about this breaking parsers. I'm not too worried about it
>> causing problems to have it either, though, so if you'd rather keep
>> it, that's fine by me as well.
>>
>>>> - It'd be a possibility to get rid of the square brackets from the
>>>> output, and if we still want them, make them part of the test itself:
>>>> if this were TAP formatted, those brackets would be part of the
>>>> subsubtest name.
>>>
>>> I don't mind. It's just that we can't prescribe a format, and as
>>> seen below the descriptions include characters -<>=,. which can be
>>> confusing. But perhaps you're right, so let's remove them.
>>>
>>> But as noted, TAP doesn't seem to care. So let's remove them.
>>>
>>
>> Yeah: I have a slight preference for removing them, as TAP parsers
>> would otherwise include them in the parameter name, which looks a
>> little weird.
>> Of course, the point is moot until we actually fix kunit_tool and make
>> these subtests, so there's no fundamental reason we couldn't leave
>> them in for now, and remove them then if you thought it was
>> significantly more readable. (Personally, I'd still err on the side of
>> removing them to avoid any unnecessary churn.)
>
> Sounds good.
>
> Arpitha: Do you want to send v7, but with the following modifications
> from what I proposed? Assuming nobody objects.
>
> 1. Remove the num_params counter and don't print the number of params
> anymore, nor do validation that generators are deterministic.
> 2. Remove the [].
> [ I'm happy to send as well, just let me know what you prefer. ]
>
> Thanks,
> -- Marco
>

If no objections I will send the v7 with the above changes.
Thanks!

2020-11-14 03:21:05

by David Gow

[permalink] [raw]
Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing

On Sat, Nov 14, 2020 at 9:38 AM Arpitha Raghunandan <[email protected]> wrote:
>
> On 14/11/20 5:44 am, Marco Elver wrote:
> >
> > Arpitha: Do you want to send v7, but with the following modifications
> > from what I proposed? Assuming nobody objects.
> >
> > 1. Remove the num_params counter and don't print the number of params
> > anymore, nor do validation that generators are deterministic.
> > 2. Remove the [].
> > [ I'm happy to send as well, just let me know what you prefer. ]
> >
> > Thanks,
> > -- Marco
> >
>
> If no objections I will send the v7 with the above changes.
> Thanks!

This sounds good to me!

Cheers,
-- David