Re: [PATCH v3 2/2] kunit: improve KTAP compliance of KUnit test output

[David Gow]

On Thu, Nov 24, 2022 at 2:26 AM Rae Moar <rmoar@xxxxxxxxxx> wrote:
>
> Change KUnit test output to better comply with KTAP v1 specifications
> found here: https://kernel.org/doc/html/latest/dev-tools/ktap.html.
> 1) Use "KTAP version 1" instead of "TAP version 14" as test output header
> 2) Remove '-' between test number and test name on test result lines
> 2) Add KTAP version lines to each subtest header as well
>
> Note that the new KUnit output still includes the “# Subtest” line now
> located after the KTAP version line. This does not completely match the
> KTAP v1 spec but since it is classified as a diagnostic line, it is not
> expected to be disruptive or break any existing parsers. This
> “# Subtest” line comes from the TAP 14 spec
> (https://testanything.org/tap-version-14-specification.html) and it is
> used to define the test name before the results.
>
> Original output:
>
>  TAP version 14
>  1..1
>    # Subtest: kunit-test-suite
>    1..3
>    ok 1 - kunit_test_1
>    ok 2 - kunit_test_2
>    ok 3 - kunit_test_3
>  # kunit-test-suite: pass:3 fail:0 skip:0 total:3
>  # Totals: pass:3 fail:0 skip:0 total:3
>  ok 1 - kunit-test-suite
>
> New output:
>
>  KTAP version 1
>  1..1
>    KTAP version 1
>    # Subtest: kunit-test-suite
>    1..3
>    ok 1 kunit_test_1
>    ok 2 kunit_test_2
>    ok 3 kunit_test_3
>  # kunit-test-suite: pass:3 fail:0 skip:0 total:3
>  # Totals: pass:3 fail:0 skip:0 total:3
>  ok 1 kunit-test-suite
>
> Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx>
> Reviewed-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
> ---
>
> Changes since v2:
> https://lore.kernel.org/all/20221121184743.1123556-2-rmoar@xxxxxxxxxx/
> - Made fixes discussed on the v2 patch to now correctly output test
>   results after second level testing
>
> Changes since v1:
> https://lore.kernel.org/all/20221104194705.3245738-1-rmoar@xxxxxxxxxx/
> - Switch order of patches to make changes to the parser before making
>   changes to the test output
> - Change location of the new KTAP version line in subtest header to be
>   before the subtest header line
>

Thanks for fixing those. This looks good to me now.

I'm not aware of anyone who's actively parsing KUnit test results
who'd be broken by this (IIRC, all the CI systems are just grepping
for 'ok' / 'not ok' or actually using kunit.py to parse.)

Reviewed-by: David Gow <davidgow@xxxxxxxxxx>

Cheers,
-- David

>  lib/kunit/debugfs.c  | 2 +-
>  lib/kunit/executor.c | 6 +++---
>  lib/kunit/test.c     | 9 ++++++---
>  3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/lib/kunit/debugfs.c b/lib/kunit/debugfs.c
> index 1048ef1b8d6e..de0ee2e03ed6 100644
> --- a/lib/kunit/debugfs.c
> +++ b/lib/kunit/debugfs.c
> @@ -63,7 +63,7 @@ static int debugfs_print_results(struct seq_file *seq, void *v)
>         kunit_suite_for_each_test_case(suite, test_case)
>                 debugfs_print_result(seq, suite, test_case);
>
> -       seq_printf(seq, "%s %d - %s\n",
> +       seq_printf(seq, "%s %d %s\n",
>                    kunit_status_to_ok_not_ok(success), 1, suite->name);
>         return 0;
>  }
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 9bbc422c284b..74982b83707c 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -166,7 +166,7 @@ static void kunit_exec_run_tests(struct suite_set *suite_set)
>  {
>         size_t num_suites = suite_set->end - suite_set->start;
>
> -       pr_info("TAP version 14\n");
> +       pr_info("KTAP version 1\n");
>         pr_info("1..%zu\n", num_suites);
>
>         __kunit_test_suites_init(suite_set->start, num_suites);
> @@ -177,8 +177,8 @@ static void kunit_exec_list_tests(struct suite_set *suite_set)
>         struct kunit_suite * const *suites;
>         struct kunit_case *test_case;
>
> -       /* Hack: print a tap header so kunit.py can find the start of KUnit output. */
> -       pr_info("TAP version 14\n");
> +       /* Hack: print a ktap header so kunit.py can find the start of KUnit output. */
> +       pr_info("KTAP version 1\n");
>
>         for (suites = suite_set->start; suites < suite_set->end; suites++)
>                 kunit_suite_for_each_test_case((*suites), test_case) {
> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> index 90640a43cf62..1c9d8d962d67 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -149,6 +149,7 @@ EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
>
>  static void kunit_print_suite_start(struct kunit_suite *suite)
>  {
> +       kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "KTAP version 1\n");
>         kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s",
>                   suite->name);
>         kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "1..%zd",
> @@ -175,13 +176,13 @@ static void kunit_print_ok_not_ok(void *test_or_suite,
>          * representation.
>          */
>         if (suite)
> -               pr_info("%s %zd - %s%s%s\n",
> +               pr_info("%s %zd %s%s%s\n",
>                         kunit_status_to_ok_not_ok(status),
>                         test_number, description, directive_header,
>                         (status == KUNIT_SKIPPED) ? directive : "");
>         else
>                 kunit_log(KERN_INFO, test,
> -                         KUNIT_SUBTEST_INDENT "%s %zd - %s%s%s",
> +                         KUNIT_SUBTEST_INDENT "%s %zd %s%s%s",
>                           kunit_status_to_ok_not_ok(status),
>                           test_number, description, directive_header,
>                           (status == KUNIT_SKIPPED) ? directive : "");
> @@ -542,6 +543,8 @@ int kunit_run_tests(struct kunit_suite *suite)
>                         /* Get initial param. */
>                         param_desc[0] = '\0';
>                         test.param_value = test_case->generate_params(NULL, param_desc);
> +                       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);
>
> @@ -555,7 +558,7 @@ int kunit_run_tests(struct kunit_suite *suite)
>
>                                 kunit_log(KERN_INFO, &test,
>                                           KUNIT_SUBTEST_INDENT KUNIT_SUBTEST_INDENT
> -                                         "%s %d - %s",
> +                                         "%s %d %s",
>                                           kunit_status_to_ok_not_ok(test.status),
>                                           test.param_index + 1, param_desc);
>
> --
> 2.38.1.584.g0f3c55d4c2-goog
>

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature