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

[Arpitha Raghunandan <98.arpi@gmail.com>]

On 09/10/20 11:50 pm, Brendan Higgins wrote:
> Arpitha, please add Iurii on future revisions. He authored this test.
> Okay!

> 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?
> 
Yes, this is better. I will make this change.

>>                 {
>>                         .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