On Mon, Oct 2, 2023 at 9:42 AM Michal Wajdeczko
<michal.wajdeczko@xxxxxxxxx> wrote:
>
>
>
> On 28.09.2023 22:52, Rae Moar wrote:
> > On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko
> > <michal.wajdeczko@xxxxxxxxx> wrote:
> >>
> >> A kunit suite is a top level test from the KTAP point of view but
> >> all suite diagnostic messages are printed at the subtest level:
> >>
> >> $ ./tools/testing/kunit/kunit.py run --raw_output \
> >> --kunitconfig ./lib/kunit/.kunitconfig "example.*simple*"
> >>
> >> KTAP version 1
> >> 1..1
> >> # example: initializing suite
> >> # example: failed to initialize (-ENODEV)
> >> not ok 1 example
> >>
> >> KTAP version 1
> >> 1..1
> >> # example: initializing suite
> >> KTAP version 1
> >> # Subtest: example
> >> # module: kunit_example_test
> >> 1..1
> >> # example_simple_test: initializing
> >> # example_simple_test: cleaning up
> >> ok 1 example_simple_test
> >> # example: exiting suite
> >> ok 1 example
> >>
> >> Replace hardcoded indent in kunit_printk() with flexible
> >> indentation based on the argument type (test or suite):
> >>
> >> KTAP version 1
> >> 1..1
> >> # example: initializing suite
> >> # example: failed to initialize (-ENODEV)
> >> not ok 1 example
> >>
> >> KTAP version 1
> >> 1..1
> >> # example: initializing suite
> >> KTAP version 1
> >> # Subtest: example
> >> # module: kunit_example_test
> >> 1..1
> >> # example_simple_test: initializing
> >> # example_simple_test: cleaning up
> >> ok 1 example_simple_test
> >> # example: exiting suite
> >> ok 1 example
> >
> > Hi!
> >
> > I am happy to see this change to improve the indentation of
> > parameterized tests. It has been bugging me for a bit. This seems to
> > be working well but I just had a few comments to potentially discuss.
> >
> > Thanks!
> >
> > -Rae
> >
Hello!
Thanks for getting back to me.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> >> Cc: David Gow <davidgow@xxxxxxxxxx>
> >> Cc: Rae Moar <rmoar@xxxxxxxxxx>
> >> ---
> >> include/kunit/test.h | 24 ++++++++++++++++++++++--
> >> lib/kunit/test.c | 7 -------
> >> 2 files changed, 22 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/include/kunit/test.h b/include/kunit/test.h
> >> index 20ed9f9275c9..158876c4cb43 100644
> >> --- a/include/kunit/test.h
> >> +++ b/include/kunit/test.h
> >> @@ -509,6 +509,21 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
> >> kunit_try_catch_throw(&((test_or_suite)->try_catch)); \
> >> } while (0)
> >>
> >> +/* Currently supported test levels */
> >> +enum {
> >> + KUNIT_LEVEL_SUITE = 0,
> >> + KUNIT_LEVEL_CASE,
> >> + KUNIT_LEVEL_CASE_PARAM,
> >> +};
> >
> > I do find this slightly confusing to have a KUNIT_LEVEL_SUITE as the
> > suite output occurs on multiple levels. Plus, I also don't see any
> > uses of the SUITE level or the PARAM level anymore. Do you have any
> > ideas on this?
>
> This enum was just promoted as-is from the test.c as I didn't want to
> use magic 0 or 1 in below kunit_level() macro.
>
> Note that the KUNIT_LEVEL_SUITE is now used indirectly whenever you call
> any of kunit_printk() with suite param like here:
>
> ./kunit-example-test.c:60: kunit_info(suite, "initializing suite\n");
> ./kunit-example-test.c:71: kunit_info(suite, "exiting suite\n");
>
Oops sorry for missing this instance.
> as this will result in calls to kunit_indent_level(suite) and
> kunit_level(suite) macro.
>
> And KUNIT_LEVEL_CASE_PARAM is maybe a leftover, as now we change test
> levels using direct increase/decrease statements, but still IMO this
> enum nicely reflects what kind of levels we currently do support at this
> point. But could be dropped if needed.
Given the suite level is being used, I am on board with keeping the
enum as is. Plus, although the param level is not explicitly being
used, it does correspond with the value of the test->level field when
running parameterized tests.
>
> Regarding _"suite output occurs on multiple levels"_ I found that also
> confusing, but IMO this is due to the kunit implementation details
> and/or KTAP design decisions as it looks that "suite" is treated
> sometimes as "subtest" and sometimes as a top level "test".
>
> That makes a little trouble while trying to implement printing in a
> consistent manner. Maybe we should consider introducing concept of the
> "executor" with its own level attribute? Then mapping will be like this:
>
> KTAP version 1 <- kunit executor (level=0)
> 1..1 <- kunit executor (level=0)
> # Test: example <- kunit executor (level=0) ??
> # module: kunit_example_test <- kunit executor (level=0) ??
> # example: initializing suite <- suite (test level=0)
> KTAP version 1 <- suite executor (level=1)
> # Subtest: example <- suite executor (level=1)
> # module: kunit_example_test <- suite executor (level=1)
> 1..2 <- suite executor (level=1)
> # test_1: initializing <- test_case (test level=1)
> # test_1: cleaning up <- test_case (test level=1)
> # test_1: pass:1 fail:0 skip:0 tota <- suite executor (level=1)
> ok 1 test_1 <- suite executor (level=1)
> KTAP version 1 <- params executor (level=2)
> # Subtest: test_2 <- params executor (level=2)
> 1..2 <- params executor (level=2)
> # test_2: initializing <- test_case (test level=2)
> # test_2: cleaning up <- test_case (test level=2)
> ok 1 example value 1 <- params executor (level=2)
> # test_2: initializing <- test_case (test level=2)
> # test_2: cleaning up <- test_case (test level=2)
> ok 2 example value 0 <- params executor (level=2)
> # test_2: pass:2 fail:0 skip:0 tota <- suite executor (level=1)
> ok 2 test_2 <- suite executor (level=1)
> # example: exiting suite <- suite (test level=0)
> # example: pass:2 fail:0 skip:0 total:2 <- kunit executor (level=0)
> # Totals: pass:3 fail:0 skip:0 total:3 <- kunit executor (level=0)
> ok 1 example <- kunit executor (level=0)
>
> Then any suite and/or test level will be just based on executor.level
> This could be done as follow-up improvement.
>
I like this concept of a general executor or test object. I would be
interested in David's opinion on this.
However, this may be a future task for when we change the overall
KUnit test implementation to be more general (remove the strict
concept of suite vs test case).
Perhaps for now we should keep your current implementation.
> >
> > We could consider just using the test->level field introduced in the
> > next patch to manage the levels. Or I wouldn't mind keeping this just
> > for clarity?
>
> We still need some definition for initial level, no?
> And at this point in series, we still need at least two defs.
>
To use the test->level field we would need to alter the overall KUnit
implementation to use a general test structure. Since that is not our
current structure, let's keep the original design.
> >
> >> +
> >> +#define kunit_level(test_or_suite) \
> >> + _Generic((test_or_suite), \
> >> + struct kunit_suite * : KUNIT_LEVEL_SUITE, \
> >> + struct kunit * : KUNIT_LEVEL_CASE)
> >> +
> >
> > I am not super familiar with using _Generic so I would want David's opinion.
> >
> > Also I can think of cases where it would be helpful to add an option
> > for struct kunit_case *, however, that may be an addition for the
> > future.
>
> I had entry for struct kunit_test_case* but removed that as it was never
> used in current code (no calls to kunit_log() with test_case)
>
That seems fine to me. We can always add this in later.
> >
> > And then additionally, this macro seems to be only used for the struct
> > kunit * case. Will we plan to use this in the future for suites?
>
> As pointed above we already use it for test and suite:
>
> ./kunit-example-test.c:60: kunit_info(suite, "initializing suite\n");
> ./kunit-example-test.c:71: kunit_info(suite, "exiting suite\n");
>
> >
> >> +#define kunit_indent_level(test_or_suite) \
> >> + (KUNIT_INDENT_LEN * kunit_level(test_or_suite))
> >> +
> >> /*
> >> * printk and log to per-test or per-suite log buffer. Logging only done
> >> * if CONFIG_KUNIT_DEBUGFS is 'y'; if it is 'n', no log is allocated/used.
> >> @@ -520,9 +535,14 @@ void __printf(2, 3) kunit_log_append(struct string_stream *log, const char *fmt,
> >> ##__VA_ARGS__); \
> >> } while (0)
> >>
> >> +#define kunit_log_indent(lvl, test_or_suite, fmt, ...) \
> >> + kunit_log(lvl, test_or_suite, "%*s" fmt, \
> >> + kunit_indent_level(test_or_suite), "", \
> >> + ##__VA_ARGS__)
> >> +
> >> #define kunit_printk(lvl, test, fmt, ...) \
> >> - kunit_log(lvl, test, KUNIT_SUBTEST_INDENT "# %s: " fmt, \
> >> - (test)->name, ##__VA_ARGS__)
> >> + kunit_log_indent(lvl, test, "# %s: " fmt, \
> >> + (test)->name, ##__VA_ARGS__)
> >>
> >
> > I wonder if we could consider removing the need to use
> > KUNIT_SUBTEST_INDENT in all locations. I am primarily thinking about
> > its uses in debugfs.c and test.c.
> >
> > For example in debugfs.c:
> > pr_info(KUNIT_SUBTEST_INDENT "KTAP version 1\n");
> >
> > Although, as this is a suite object that is printing at the test case
> > level, I am not quite sure how to use the existing macros.
>
> We could add some wrapper like kunit_pr() that takes suite/test/executor
> from which we can derive right indent level, but that would be another
> follow up task once we agree on location and use of .level attribute.
>
I think it would be great to offer the kunit_pr option or some general
macro for formatting given the level.
Thanks!
-Rae
> Michal
>
> >
> >
> >> /**
> >> * kunit_info() - Prints an INFO level message associated with @test.
> >> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> >> index fb5981ce578d..d10e6d610e20 100644
> >> --- a/lib/kunit/test.c
> >> +++ b/lib/kunit/test.c
> >> @@ -135,13 +135,6 @@ size_t kunit_suite_num_test_cases(struct kunit_suite *suite)
> >> }
> >> EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
> >>
> >> -/* Currently supported test levels */
> >> -enum {
> >> - KUNIT_LEVEL_SUITE = 0,
> >> - KUNIT_LEVEL_CASE,
> >> - KUNIT_LEVEL_CASE_PARAM,
> >> -};
> >> -
> >> static void kunit_print_suite_start(struct kunit_suite *suite)
> >> {
> >> /*
> >> --
> >> 2.25.1
> >>
