Re: [Linux-kernel-mentees] [RFC] kunit: Support for Parameterized Testing

[Brendan Higgins via Linux-kernel-mentees <linux-kernel-mentees@lists.linuxfoundation.org>]

On Sat, Sep 19, 2020 at 11:40 PM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>
> This patch has a basic implementation for adding support for parameterized
> testing in KUnit. It is not complete. Currently there is only support
> for integer parameters for test cases. I will generalize this to support
> other data types as well. I have tested this by making some small changes
> to the ext4/inode-test.c test. The main logic of this test hasn't been
> changed. However, I have created an integer array of inputs. The test only
> shows that these values can be accessed through the kunit struct as shown
> by the dmesg output:
>
>     # inode_test_xtimestamp_decoding:
> Parameterized = 1
>     # inode_test_xtimestamp_decoding:
> a[0][0] = 1 and a[0][1] = 2 and a[1][0] = 3 and a[1][1] = 4
>
> While this does not affect the test, it shows that this initial approach
> for parameterized testing works.
> Please let me know any suggestions for this approach and how it can be improved.
>
> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>

All in all, looks like a good start, but I think we might need a
slightly more concrete example.

> ---
>  fs/ext4/inode-test.c   | 22 +++++++++++++++++++++-
>  include/kunit/test.h   | 11 ++++++++++-
>  lib/kunit/kunit-test.c |  4 ++--
>  lib/kunit/test.c       | 15 +++++++++++++--
>  4 files changed, 46 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c

This is maintained by non-KUnit people; this should probably be in
another commit.

> index d62d802c9c12..5ef242f2fef3 100644
> --- a/fs/ext4/inode-test.c
> +++ b/fs/ext4/inode-test.c
> @@ -235,6 +235,11 @@ static void inode_test_xtimestamp_decoding(struct kunit *test)
>         struct timespec64 timestamp;
>         int i;
>
> +       kunit_info(test, "\nParameterized = %d\n", test->parameterized);
> +       kunit_info(test, "\na[0][0] = %d and a[0][1] = %d and a[1][0] = %d and a[1][1] = %d\n",
> +                                               test->param_values[0][0], test->param_values[0][1],
> +                                               test->param_values[1][0], test->param_values[1][1]);
> +

These look like debug statements. Did you forget to remove them?

>         for (i = 0; i < ARRAY_SIZE(test_data); ++i) {
>                 timestamp.tv_sec = get_32bit_time(&test_data[i]);
>                 ext4_decode_extra_time(&timestamp,
> @@ -259,8 +264,23 @@ static void inode_test_xtimestamp_decoding(struct kunit *test)
>         }
>  }
>
> +int **getParams(void)
> +{
> +       int *x = (int *)kmalloc(sizeof(int) * 2, GFP_KERNEL);
> +       int *y = (int *)kmalloc(sizeof(int) * 2, GFP_KERNEL);
> +       int **a = (int **)kmalloc(sizeof(x) * 2, GFP_KERNEL);
> +
> +       x[0] = 1;
> +       x[1] = 2;
> +       y[0] = 3;
> +       y[1] = 4;
> +       a[0] = x;
> +       a[1] = y;
> +       return a;
> +}
> +
>  static struct kunit_case ext4_inode_test_cases[] = {
> -       KUNIT_CASE(inode_test_xtimestamp_decoding),
> +       KUNIT_CASE_PARAM(inode_test_xtimestamp_decoding, getParams),
>         {}
>  };
>
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 59f3144f009a..23e4ff68c4b5 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -140,6 +140,8 @@ struct kunit;
>  struct kunit_case {
>         void (*run_case)(struct kunit *test);
>         const char *name;
> +       bool parameterized;
> +       int** (*get_params)(void);
>
>         /* private: internal use only. */
>         bool success;
> @@ -162,6 +164,10 @@ 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) { .run_case = test_name,                \
> +                                                               .name = #test_name, .parameterized = true,      \
> +                                                               .get_params = getparams }
> +
>  /**
>   * struct kunit_suite - describes a related collection of &struct kunit_case
>   *
> @@ -206,6 +212,8 @@ struct kunit {
>         /* private: internal use only. */
>         const char *name; /* Read only after initialization! */
>         char *log; /* Points at case log after initialization */
> +       bool parameterized;
> +       int **param_values;
>         struct kunit_try_catch try_catch;
>         /*
>          * success starts as true, and may only be set to false during a
> @@ -224,7 +232,8 @@ struct kunit {
>         struct list_head resources; /* Protected by lock. */
>  };
>
> -void kunit_init_test(struct kunit *test, const char *name, char *log);
> +void kunit_init_test(struct kunit *test, const char *name, char *log, bool parameterized);
> +void kunit_init_param_test(struct kunit *test, struct kunit_case *test_case);
>
>  int kunit_run_tests(struct kunit_suite *suite);
>
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 69f902440a0e..b79e287ca19b 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -134,7 +134,7 @@ static void kunit_resource_test_init_resources(struct kunit *test)
>  {
>         struct kunit_test_resource_context *ctx = test->priv;
>
> -       kunit_init_test(&ctx->test, "testing_test_init_test", NULL);
> +       kunit_init_test(&ctx->test, "testing_test_init_test", NULL, false);
>
>         KUNIT_EXPECT_TRUE(test, list_empty(&ctx->test.resources));
>  }
> @@ -370,7 +370,7 @@ static int kunit_resource_test_init(struct kunit *test)
>
>         test->priv = ctx;
>
> -       kunit_init_test(&ctx->test, "test_test_context", NULL);
> +       kunit_init_test(&ctx->test, "test_test_context", NULL, false);
>
>         return 0;
>  }
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index c36037200310..2e061bfe154d 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -224,7 +224,7 @@ void kunit_do_assertion(struct kunit *test,
>  }
>  EXPORT_SYMBOL_GPL(kunit_do_assertion);
>
> -void kunit_init_test(struct kunit *test, const char *name, char *log)
> +void kunit_init_test(struct kunit *test, const char *name, char *log, bool parameterized)
>  {
>         spin_lock_init(&test->lock);
>         INIT_LIST_HEAD(&test->resources);
> @@ -232,10 +232,19 @@ void kunit_init_test(struct kunit *test, const char *name, char *log)
>         test->log = log;
>         if (test->log)
>                 test->log[0] = '\0';
> +       test->parameterized = parameterized;
>         test->success = true;
>  }
>  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->param_values = test_case->get_params();

I don't see where you are using this other than in those debug
statements; can we see something more concrete?

> +}
> +EXPORT_SYMBOL_GPL(kunit_init_param_test);
> +
>  /*
>   * Initializes and runs test case. Does not clean up or do post validations.
>   */
> @@ -342,7 +351,9 @@ static void kunit_run_case_catch_errors(struct kunit_suite *suite,
>         struct kunit_try_catch *try_catch;
>         struct kunit test;
>
> -       kunit_init_test(&test, test_case->name, test_case->log);
> +       kunit_init_test(&test, test_case->name, test_case->log, test_case->parameterized);
> +       if (test_case->parameterized)
> +               kunit_init_param_test(&test, test_case);
>         try_catch = &test.try_catch;
>
>         kunit_try_catch_init(try_catch,
> --
> 2.25.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees