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