Re: [PATCH 3/3] kunit: Update reporting function to support results from subtests

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 13.04.2023 08:38, David Gow wrote:
> On Wed, 12 Apr 2023 at 00:01, Michal Wajdeczko
> <michal.wajdeczko@intel.com> wrote:
>>
>> There is function to report status of either suite or test, but it
>> doesn't support parameterized subtests that have to log report on
>> its own. Extend it to also accept subtest level results to avoid
>> code duplication.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: David Gow <davidgow@google.com>
>> ---
> 
> Thanks: this is definitely an improvement on how we handle this.
> 
> There's definitely more we can do, particularly looking forward to
> supporting more complex test hierarchies in the future, but getting
> everything under kunit_print_ok_not_ok is an improvement regardless of
> when happens down the line.
> 
> My only real concern is that the way the indent is printed is a bit
> subtle and difficult to understand fully on first glance. I've added
> some notes below.
> 
>>  lib/kunit/test.c | 28 +++++++++++++++++-----------
>>  1 file changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
>> index 5679197b5f8a..692fce258c5b 100644
>> --- a/lib/kunit/test.c
>> +++ b/lib/kunit/test.c
>> @@ -154,8 +154,14 @@ static void kunit_print_suite_start(struct kunit_suite *suite)
>>                   kunit_suite_num_test_cases(suite));
>>  }
>>
>> +enum kunit_test_or_suite {
>> +       KUNIT_SUITE = 0,
>> +       KUNIT_TEST,
>> +       KUNIT_SUBTEST,
>> +};
>> +
> 
> As Rae notes, this probably won't be how this code eventually evolves.
> I don't think it's a problem to have it now, though.
> 
>>  static void kunit_print_ok_not_ok(void *test_or_suite,
>> -                                 bool is_test,
>> +                                 enum kunit_test_or_suite is_test,
>>                                   enum kunit_status status,
>>                                   size_t test_number,
>>                                   const char *description,
>> @@ -180,7 +186,9 @@ static void kunit_print_ok_not_ok(void *test_or_suite,
>>                         (status == KUNIT_SKIPPED) ? directive : "");
>>         else
>>                 kunit_log(KERN_INFO, test,
>> -                         KUNIT_SUBTEST_INDENT "%s %zd %s%s%s",
>> +                         "%.*s%s %zd %s%s%s",
>> +                         (int) strlen(KUNIT_SUBTEST_INDENT) * is_test,
>> +                         KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT,
> 
> This feels a little bit _too_ clever here: I feel it at the very least
> needs a comment, and maybe it'd make more sense to either:
> - Make is_test explicitly a "nesting depth" integer, and calculate the
> indent based on that.
> - Have is_test as an enum, and then just explicitly handle each value
> separately. (Like we do with suite vs test).
> 
> I think that the former is probably the right long-term solution (it's
> much more extensible to more levels of nesting), but the latter is
> definitely easier given the differences between suites and tests at
> the moment.
> 
> If we do continue to share a codepath between tests and subtests, I'd
> prefer it if we either didn't use strlen(), or went to some greater

Rae suggested to define KUNIT_INDENT_LEN 4 and I will put it in test.h

> effort to document how that works (hopefully we can guarantee that the
> compiler will treat this as a constant). Equally, a comment or
> something noting that this will read invalid memory if is_test > 2,
> due to the hardcoded two KUNIT_SUBTEST_INDENT, would be nice.

that shouldn't happen as %.*s specifies precision and it's used here to
clamp string with more than needed indents, it will not try to read
beyond terminating \0

but since you plan for arbitrary level of testing I could change that to
a little simpler variant %*s which should always with any level:

		kunit_log(KERN_INFO, test,
			  "%*s%s %zd %s%s%s",
			  KUNIT_INDENT_LEN * test_level, "",

will that be _simple_ enough ?

Thanks,
Michal

> 
> 
>>                           kunit_status_to_ok_not_ok(status),
>>                           test_number, description, directive_header,
>>                           (status == KUNIT_SKIPPED) ? directive : "");
>> @@ -209,7 +217,7 @@ static size_t kunit_suite_counter = 1;
>>
>>  static void kunit_print_suite_end(struct kunit_suite *suite)
>>  {
>> -       kunit_print_ok_not_ok((void *)suite, false,
>> +       kunit_print_ok_not_ok((void *)suite, KUNIT_SUITE,
>>                               kunit_suite_has_succeeded(suite),
>>                               kunit_suite_counter++,
>>                               suite->name,
>> @@ -554,13 +562,11 @@ int kunit_run_tests(struct kunit_suite *suite)
>>                                                  "param-%d", test.param_index);
>>                                 }
>>
>> -                               kunit_log(KERN_INFO, &test,
>> -                                         KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
>> -                                         "%s %d %s%s%s",
>> -                                         kunit_status_to_ok_not_ok(test.status),
>> -                                         test.param_index + 1, param_desc,
>> -                                         test.status == KUNIT_SKIPPED ? " # SKIP " : "",
>> -                                         test.status == KUNIT_SKIPPED ? test.status_comment : "");
>> +                               kunit_print_ok_not_ok(&test, KUNIT_SUBTEST,
>> +                                                     test.status,
>> +                                                     test.param_index + 1,
>> +                                                     param_desc,
>> +                                                     test.status_comment);
>>
>>                                 /* Get next param. */
>>                                 param_desc[0] = '\0';
>> @@ -574,7 +580,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>>
>>                 kunit_print_test_stats(&test, param_stats);
>>
>> -               kunit_print_ok_not_ok(&test, true, test_case->status,
>> +               kunit_print_ok_not_ok(&test, KUNIT_TEST, test_case->status,
>>                                       kunit_test_case_num(suite, test_case),
>>                                       test_case->name,
>>                                       test.status_comment);
>> --
>> 2.25.1
>>
> 
> Otherwise, this all looks good to me. Thanks very much!
> 
> Cheers,
> -- David