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
>
> 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?
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?
> +
> +#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.
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?
> +#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.
> /**
> * 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
>
