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

Signed-off-by: Arpitha Raghunandan 98.arpi@gmail.com --- fs/ext4/inode-test.c | 64 +++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c index d62d802c9c12..691ef0a4ffe1 100644 --- a/fs/ext4/inode-test.c +++ b/fs/ext4/inode-test.c @@ -72,6 +72,8 @@ #define UPPER_BOUND_NONNEG_EXTRA_BITS_1_CASE\ "2446-05-10 Upper bound of 32bit >=0 timestamp. All extra sec bits on"

+#define NUMBER_OF_TESTCASES 16 + struct timestamp_expectation { const char *test_case_name; struct timespec64 expected; @@ -101,7 +103,36 @@ 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[] = { + struct timespec64 timestamp; + + struct timestamp_expectation *test_data = + (struct timestamp_expectation *)get_test_case_parameters(test); + + timestamp.tv_sec = get_32bit_time(test_data); + ext4_decode_extra_time(&timestamp, + cpu_to_le32(test_data->extra_bits)); + + KUNIT_EXPECT_EQ_MSG(test, + test_data->expected.tv_sec, + timestamp.tv_sec, + CASE_NAME_FORMAT, + test_data->test_case_name, + test_data->msb_set, + test_data->lower_bound, + test_data->extra_bits); + KUNIT_EXPECT_EQ_MSG(test, + test_data->expected.tv_nsec, + timestamp.tv_nsec, + CASE_NAME_FORMAT, + test_data->test_case_name, + test_data->msb_set, + test_data->lower_bound, + test_data->extra_bits); +} + +struct timestamp_expectation *get_test_parameters(void) +{ + static struct timestamp_expectation test_data[] = { { .test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE, .msb_set = true, @@ -231,36 +262,13 @@ static void inode_test_xtimestamp_decoding(struct kunit *test) .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); - } + return test_data; }

static struct kunit_case ext4_inode_test_cases[] = { - KUNIT_CASE(inode_test_xtimestamp_decoding), + KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding, + get_test_parameters, NUMBER_OF_TESTCASES, + sizeof(struct timestamp_expectation)), {} };

On Sat, 10 Oct 2020 at 16:54, Arpitha Raghunandan 98.arpi@gmail.com wrote:

Implementation of support for parameterized testing in KUnit.

Signed-off-by: Arpitha Raghunandan 98.arpi@gmail.com

include/kunit/test.h | 29 +++++++++++++++++++++++++++++ lib/kunit/test.c | 44 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/include/kunit/test.h b/include/kunit/test.h index 59f3144f009a..4740d66269b4 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -140,10 +140,14 @@ struct kunit; struct kunit_case { void (*run_case)(struct kunit *test); const char *name;

•   void* (*get_params)(void);
  

•   int max_parameters_count;
  

•   int parameter_size;
  
    /* private: internal use only. */
    bool success;
    char *log;
  

•   bool parameterized;
  

Why do you need this bool? Doesn't get_params being non-NULL tell you if the test case is parameterized?

};

static inline char *kunit_status_to_string(bool status) @@ -162,6 +166,11 @@ static inline char *kunit_status_to_string(bool status) */ #define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name }

+#define KUNIT_CASE_PARAM(test_name, getparams, count, size) \

•           { .run_case = test_name, .name = #test_name,                            \
  

•             .parameterized = true, .get_params = (void* (*)(void))getparams,      \
  

•             .max_parameters_count = count, .parameter_size = size }
  

I think this interface is overly complex. For one, if the only purpose of the getparams function is to return a pointer to some array, then there are only few cases where I see getparams being a function could be useful.

Instead, could we make the getparams function behave like a generator? Because then you do not need count, nor size. Its function signature would be:

void* (*generate_params)(void* prev_param);

The protocol would be:

- The first call to generate_params is passed prev_param of NULL, and returns a pointer to the first parameter P[0].

- Every nth successive call to generate_params is passed the previous parameter P[n-1].

- When no more parameters are available, generate_params returns NULL.

- (generate_params should otherwise be stateless, but this is only relevant if concurrent calls are expected.)

/**

• struct kunit_suite - describes a related collection of &struct kunit_case

@@ -206,6 +215,23 @@ struct kunit { /* private: internal use only. */ const char *name; /* Read only after initialization! */ char *log; /* Points at case log after initialization */

•   bool parameterized; /* True for parameterized tests */
  

•   /* param_values stores the test parameters
  

•    * for parameterized tests.
  

•    */
  

•   void *param_values;
  

•   /* max_parameters_count indicates maximum number of
  

•    * parameters for parameterized tests.
  

•    */
  

•   int max_parameters_count;
  

•   /* iterator_count is used by the iterator method
  

•    * for parameterized tests.
  

•    */
  

•   int iterator_count;
  

•   /* parameter_size indicates size of a single test case
  

•    * for parameterized tests.
  

•    */
  

•   int parameter_size;
  

All of this would become much simpler if you used the generator approach. Likely only 1 field would be required, which is the current param.

    struct kunit_try_catch try_catch;
    /*
     * success starts as true, and may only be set to false during a

@@ -225,6 +251,7 @@ struct kunit { };

void kunit_init_test(struct kunit *test, const char *name, char *log); +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case);

int kunit_run_tests(struct kunit_suite *suite);

@@ -237,6 +264,8 @@ int __kunit_test_suites_init(struct kunit_suite **suites);

void __kunit_test_suites_exit(struct kunit_suite **suites);

+void *get_test_case_parameters(struct kunit *test);

/**

• kunit_test_suites() - used to register one or more &struct kunit_suite

•                  with KUnit.
  

diff --git a/lib/kunit/test.c b/lib/kunit/test.c index c36037200310..ab9e13c81d4a 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -142,6 +142,11 @@ unsigned int kunit_test_case_num(struct kunit_suite *suite, } EXPORT_SYMBOL_GPL(kunit_test_case_num);

+static void kunit_print_failed_param(struct kunit *test) +{

•   kunit_err(test, "\n\tTest failed at parameter: %d\n", test->iterator_count);
  

+}

static void kunit_print_string_stream(struct kunit *test, struct string_stream *stream) { @@ -182,6 +187,9 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)

    assert->format(assert, stream);

•   if (test->parameterized)
  

•           kunit_print_failed_param(test);
  

•   kunit_print_string_stream(test, stream);
  
    WARN_ON(string_stream_destroy(stream));
  

@@ -236,6 +244,18 @@ void kunit_init_test(struct kunit *test, const char *name, char *log) } EXPORT_SYMBOL_GPL(kunit_init_test);

+void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case) +{

•   spin_lock_init(&test->lock);
  

•   INIT_LIST_HEAD(&test->resources);
  

•   test->parameterized = true;
  

•   test->param_values = (void *)(test_case->get_params());
  

•   test->max_parameters_count = test_case->max_parameters_count;
  

•   test->parameter_size = test_case->parameter_size;
  

•   test->iterator_count = 0;
  

+} +EXPORT_SYMBOL_GPL(kunit_init_param_test);

/*

• Initializes and runs test case. Does not clean up or do post validations.

*/ @@ -254,7 +274,14 @@ static void kunit_run_case_internal(struct kunit *test, } }

•   test_case->run_case(test);
  

•   if (!test->parameterized) {
  

•           test_case->run_case(test);
  

•   } else {
  

•           int i;
  

•           for (i = 0; i < test->max_parameters_count; i++)
  

•                   test_case->run_case(test);
  

With a generator approach, here you'd call generate_params. Most likely, you'll need to stash its result somewhere, e.g. test->param, so it can be retrieved by the test case.

•   }
  

}

static void kunit_case_internal_cleanup(struct kunit *test) @@ -343,6 +370,8 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite, struct kunit test;

    kunit_init_test(&test, test_case->name, test_case->log);

•   if (test_case->parameterized)
  

•           kunit_init_param_test(&test, test_case);
    try_catch = &test.try_catch;
  
    kunit_try_catch_init(try_catch,
  

@@ -407,6 +436,19 @@ void __kunit_test_suites_exit(struct kunit_suite **suites) } EXPORT_SYMBOL_GPL(__kunit_test_suites_exit);

+/*

• • Iterator method for the parameterized test cases
• */

+void *get_test_case_parameters(struct kunit *test) +{

•   int index = test->iterator_count * test->parameter_size;
  

•   if (test->iterator_count != test->max_parameters_count)
  

•           test->iterator_count++;
  

This is quite confusing, because if get_test_case_parameters is called multiple times within the same test case, we'll iterate through all the test case params in the same test case? I think this function should not have side-effects (like normal getters).

But if you use the generator approach, you'll likely not need this function anyway.

•   return (test->param_values + index);
  

Braces not needed.

+} +EXPORT_SYMBOL_GPL(get_test_case_parameters);

/*

• Used for static resources and when a kunit_resource * has been created by
• kunit_alloc_resource(). When an init function is supplied, @data is passed

-- 2.25.1