On 28.09.2023 22:53, Rae Moar wrote:
> On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko
> <michal.wajdeczko@xxxxxxxxx> wrote:
>>
>> When running parametrized test cases, diagnostic messages
>> are not properly aligned with the test result lines:
>>
>> $ ./tools/testing/kunit/kunit.py run --raw_output \
>> --kunitconfig ./lib/kunit/.kunitconfig *.example_params*
>>
>> KTAP version 1
>> 1..1
>> # example: initializing suite
>> KTAP version 1
>> # Subtest: example
>> # module: kunit_example_test
>> 1..1
>> KTAP version 1
>> # Subtest: example_params_test
>> # example_params_test: initializing
>> # example_params_test: cleaning up
>> ok 1 example value 3 # SKIP unsupported param value 3
>> # example_params_test: initializing
>> # example_params_test: cleaning up
>> ok 2 example value 2
>> # example_params_test: initializing
>> # example_params_test: cleaning up
>> ok 3 example value 1
>> # example_params_test: initializing
>> # example_params_test: cleaning up
>> ok 4 example value 0 # SKIP unsupported param value 0
>> # example_params_test: pass:2 fail:0 skip:2 total:4
>> ok 1 example_params_test
>> # example: exiting suite
>> # Totals: pass:2 fail:0 skip:2 total:4
>> ok 1 example
>>
>> Add test level attribute and use it to generate proper indent
>> at the runtime:
>>
>> KTAP version 1
>> 1..1
>> # example: initializing suite
>> KTAP version 1
>> # Subtest: example
>> # module: kunit_example_test
>> 1..1
>> KTAP version 1
>> # Subtest: example_params_test
>> # example_params_test: initializing
>> # example_params_test: cleaning up
>> ok 1 example value 3 # SKIP unsupported param value 3
>> # example_params_test: initializing
>> # example_params_test: cleaning up
>> ok 2 example value 2
>> # example_params_test: initializing
>> # example_params_test: cleaning up
>> ok 3 example value 1
>> # example_params_test: initializing
>> # example_params_test: cleaning up
>> ok 4 example value 0 # SKIP unsupported param value 0
>> # example_params_test: pass:2 fail:0 skip:2 total:4
>> ok 1 example_params_test
>> # example: exiting suite
>> # Totals: pass:2 fail:0 skip:2 total:4
>> ok 1 example
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>> Cc: David Gow <davidgow@xxxxxxxxxx>
>> Cc: Rae Moar <rmoar@xxxxxxxxxx>
>
> Hello!
>
> Great to see these changes! Just a few comments below.
>
> Thanks!
> -Rae
>
>> ---
>> include/kunit/test.h | 3 ++-
>> lib/kunit/test.c | 52 ++++++++++++++++++++------------------------
>> 2 files changed, 26 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/kunit/test.h b/include/kunit/test.h
>> index 158876c4cb43..4804d539e10f 100644
>> --- a/include/kunit/test.h
>> +++ b/include/kunit/test.h
>> @@ -276,6 +276,7 @@ struct kunit {
>> void *priv;
>>
>> /* private: internal use only. */
>> + unsigned int level; /* Helps in proper log indent */
>> const char *name; /* Read only after initialization! */
>> struct string_stream *log; /* Points at case log after initialization */
>> struct kunit_try_catch try_catch;
>> @@ -519,7 +520,7 @@ enum {
>> #define kunit_level(test_or_suite) \
>> _Generic((test_or_suite), \
>> struct kunit_suite * : KUNIT_LEVEL_SUITE, \
>> - struct kunit * : KUNIT_LEVEL_CASE)
>> + struct kunit * : ((struct kunit *)(test_or_suite))->level)
>>
>> #define kunit_indent_level(test_or_suite) \
>> (KUNIT_INDENT_LEN * kunit_level(test_or_suite))
>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
>> index d10e6d610e20..43c3efc286e4 100644
>> --- a/lib/kunit/test.c
>> +++ b/lib/kunit/test.c
>> @@ -99,14 +99,13 @@ static void kunit_print_test_stats(struct kunit *test,
>> if (!kunit_should_print_stats(stats))
>> return;
>>
>> - kunit_log(KERN_INFO, test,
>> - KUNIT_SUBTEST_INDENT
>> - "# %s: pass:%lu fail:%lu skip:%lu total:%lu",
>> - test->name,
>> - stats.passed,
>> - stats.failed,
>> - stats.skipped,
>> - stats.total);
>> + kunit_log_indent(KERN_INFO, test,
>> + "# %s: pass:%lu fail:%lu skip:%lu total:%lu",
>> + test->name,
>> + stats.passed,
>> + stats.failed,
>> + stats.skipped,
>> + stats.total);
>
> I would prefer if we keep the same indentation level as before.
note that then scripts/checkpatch.pl will complain with:
CHECK: Alignment should match open parenthesis
#109: FILE: lib/kunit/test.c:103:
+ kunit_log_indent(KERN_INFO, test,
"# %s: pass:%lu fail:%lu skip:%lu total:%lu",
CHECK: Alignment should match open parenthesis
#141: FILE: lib/kunit/test.c:178:
+ kunit_log_indent(KERN_INFO, test,
+ "%s %zd %s%s%s",
are you ok with that?
Michal
> Otherwise looks good.
>
>> }
>>
>> /* Append formatted message to log. */
>> @@ -154,7 +153,6 @@ static void kunit_print_suite_start(struct kunit_suite *suite)
>> }
>>
>> static void kunit_print_ok_not_ok(struct kunit *test,
>> - unsigned int test_level,
>> enum kunit_status status,
>> size_t test_number,
>> const char *description,
>> @@ -163,12 +161,6 @@ static void kunit_print_ok_not_ok(struct kunit *test,
>> const char *directive_header = (status == KUNIT_SKIPPED) ? " # SKIP " : "";
>> const char *directive_body = (status == KUNIT_SKIPPED) ? directive : "";
>>
>> - /*
>> - * When test is NULL assume that results are from the suite
>> - * and today suite results are expected at level 0 only.
>> - */
>> - WARN(!test && test_level, "suite test level can't be %u!\n", test_level);
>> -
>> /*
>> * We do not log the test suite results as doing so would
>> * mean debugfs display would consist of an incorrect test
>> @@ -182,12 +174,11 @@ static void kunit_print_ok_not_ok(struct kunit *test,
>> test_number, description, directive_header,
>> directive_body);
>> else
>> - kunit_log(KERN_INFO, test,
>> - "%*s%s %zd %s%s%s",
>> - KUNIT_INDENT_LEN * test_level, "",
>> - kunit_status_to_ok_not_ok(status),
>> - test_number, description, directive_header,
>> - directive_body);
>> + kunit_log_indent(KERN_INFO, test,
>> + "%s %zd %s%s%s",
>> + kunit_status_to_ok_not_ok(status),
>> + test_number, description, directive_header,
>> + directive_body);
>
> Again, I would prefer we keep the same indentation as before as it
> matches the rest of the file.
>
>
>> }
>>
>> enum kunit_status kunit_suite_has_succeeded(struct kunit_suite *suite)
>> @@ -213,7 +204,7 @@ static size_t kunit_suite_counter = 1;
>>
>> static void kunit_print_suite_end(struct kunit_suite *suite)
>> {
>> - kunit_print_ok_not_ok(NULL, KUNIT_LEVEL_SUITE,
>> + kunit_print_ok_not_ok(NULL,
>> kunit_suite_has_succeeded(suite),
>> kunit_suite_counter++,
>> suite->name,
>> @@ -322,6 +313,7 @@ void kunit_init_test(struct kunit *test, const char *name, struct string_stream
>> {
>> spin_lock_init(&test->lock);
>> INIT_LIST_HEAD(&test->resources);
>> + test->level = KUNIT_LEVEL_CASE;
>> test->name = name;
>> test->log = log;
>> if (test->log)
>> @@ -584,14 +576,15 @@ int kunit_run_tests(struct kunit_suite *suite)
>> kunit_run_case_catch_errors(suite, test_case, &test);
>> kunit_update_stats(¶m_stats, test.status);
>> } else {
>> + /* Parameterized test is one level up from simple test-case. */
>> + test.level++;
>> +
>> /* Get initial param. */
>> param_desc[0] = '\0';
>> test.param_value = test_case->generate_params(NULL, param_desc);
>> test_case->status = KUNIT_SKIPPED;
>> - kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
>> - "KTAP version 1\n");
>> - kunit_log(KERN_INFO, &test, KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
>> - "# Subtest: %s", test_case->name);
>> + kunit_log_indent(KERN_INFO, &test, "KTAP version 1\n");
>> + kunit_log_indent(KERN_INFO, &test, "# Subtest: %s", test_case->name);
>>
>> while (test.param_value) {
>> kunit_run_case_catch_errors(suite, test_case, &test);
>> @@ -601,7 +594,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>> "param-%d", test.param_index);
>> }
>>
>> - kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE_PARAM,
>> + kunit_print_ok_not_ok(&test,
>> test.status,
>> test.param_index + 1,
>> param_desc,
>> @@ -616,13 +609,16 @@ int kunit_run_tests(struct kunit_suite *suite)
>> test.status = KUNIT_SUCCESS;
>> test.status_comment[0] = '\0';
>> }
>> +
>> + /* Return to parent (test-case) level. */
>> + test.level--;
>> }
>>
>> kunit_print_attr((void *)test_case, true, KUNIT_LEVEL_CASE);
>>
>> kunit_print_test_stats(&test, param_stats);
>>
>> - kunit_print_ok_not_ok(&test, KUNIT_LEVEL_CASE, test_case->status,
>> + kunit_print_ok_not_ok(&test, test_case->status,
>> kunit_test_case_num(suite, test_case),
>> test_case->name,
>> test.status_comment);
>> --
>> 2.25.1
>>
