Re: [Linux-kernel-mentees] [RFC v3 2/2] fs: ext4: Modify inode-test.c to use KUnit parameterized testing

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

Arpitha, please add Iurii on future revisions. He authored this test.

On Sat, Oct 3, 2020 at 7:51 AM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
>
> 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 | 69 +++++++++++++++++++++++++++-----------------
>  1 file changed, 42 insertions(+), 27 deletions(-)
>
> diff --git a/fs/ext4/inode-test.c b/fs/ext4/inode-test.c
> index d62d802c9c12..e262fef505b3 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,39 @@ 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_case =

I think test_data or test_parameters makes more sense.

> +               (struct timestamp_expectation *)get_test_case_parameters(test);
> +
> +       timestamp.tv_sec = get_32bit_time(test_case);
> +       ext4_decode_extra_time(&timestamp,
> +                              cpu_to_le32(test_case->extra_bits));
> +
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           test_case->expected.tv_sec,
> +                           timestamp.tv_sec,
> +                           CASE_NAME_FORMAT,
> +                           test_case->test_case_name,
> +                           test_case->msb_set,
> +                           test_case->lower_bound,
> +                           test_case->extra_bits);
> +       KUNIT_EXPECT_EQ_MSG(test,
> +                           test_case->expected.tv_nsec,
> +                           timestamp.tv_nsec,
> +                           CASE_NAME_FORMAT,
> +                           test_case->test_case_name,
> +                           test_case->msb_set,
> +                           test_case->lower_bound,
> +                           test_case->extra_bits);
> +}
> +
> +struct timestamp_expectation *get_test_parameters(void)
> +{
> +       struct timestamp_expectation *test_data = (struct timestamp_expectation *)
> +               kmalloc(sizeof(struct timestamp_expectation) * NUMBER_OF_TESTCASES, GFP_KERNEL);

I don't see this get freed anywhere; you can get around it with
kunit_kmalloc. However, I suspect you won't need this at all given my
next comment...

> +
> +       const struct timestamp_expectation test_data_init[] = {

Can't you just make the scope of this array global or static and then
just return a pointer to an element in the array?

>                 {
>                         .test_case_name = LOWER_BOUND_NEG_NO_EXTRA_BITS_CASE,
>                         .msb_set = true,
> @@ -232,35 +266,16 @@ static void inode_test_xtimestamp_decoding(struct kunit *test)
>                 }
>         };
>
> -       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);
> -       }
> +       memcpy(test_data, test_data_init,
> +               sizeof(struct timestamp_expectation) * ARRAY_SIZE(test_data_init));
> +
> +       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)),
>         {}
>  };
>
> --
> 2.25.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees