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
>
>>
>> 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");
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.
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.
>
> 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.
>
>> +
>> +#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)
>
> 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.
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
>>
