Re: [PATCH 2/4] kunit: Fix indentation level of suite messages

On Mon, Sep 25, 2023 at 1:58 PM Michal Wajdeczko michal.wajdeczko@intel.com 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@intel.com Cc: David Gow davidgow@google.com Cc: Rae Moar rmoar@google.com

---

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