On Mon, Oct 2, 2023 at 9:43 AM Michal Wajdeczko
<michal.wajdeczko@xxxxxxxxx> wrote:
>
>
>
> 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
Hello!
I understand now. It is unfortunate that the previous indentation
causes a checkpatch warning. I suppose KUnit should fix the
indentation of the file as a whole in a separate patch then.
Since the issue of the indentation is resolved, I am happy with this
current patch.
Thanks!
-Rae
>
> > 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
> >>
