diff mbox series

[v2,2/2] kunit: improve KTAP compliance of KUnit test output

Message ID 20221121184743.1123556-2-rmoar@google.com (mailing list archive)
State New
Delegated to: Brendan Higgins
Headers show
Series [v2,1/2] kunit: tool: parse KTAP compliant test output | expand

Commit Message

Rae Moar Nov. 21, 2022, 6:47 p.m. UTC 
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@google.com>
Reviewed-by: Daniel Latypov <dlatypov@google.com>
Reviewed-by: David Gow <davidgow@google.com>
---

Changes since v1:
https://lore.kernel.org/all/20221104194705.3245738-1-rmoar@google.com/
- 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

 lib/kunit/executor.c | 6 +++---
 lib/kunit/test.c     | 5 +++--
 2 files changed, 6 insertions(+), 5 deletions(-)

Comments

Daniel Latypov Nov. 21, 2022, 8:32 p.m. UTC | #1 
On Mon, Nov 21, 2022 at 10:48 AM Rae Moar <rmoar@google.com> 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@google.com>
> Reviewed-by: Daniel Latypov <dlatypov@google.com>
> Reviewed-by: David Gow <davidgow@google.com>
> ---
>
> Changes since v1:
> https://lore.kernel.org/all/20221104194705.3245738-1-rmoar@google.com/
> - 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

This patch still looks good to me. In fact, it looks better.

I prefer this updated version since this works a bit better with
debugfs. This way, kunit.py won't just skip over the subtest line when
looking for the initial KTAP header.

Daniel
Anders Roxell Nov. 22, 2022, 9:17 a.m. UTC | #2 
On Mon, 21 Nov 2022 at 19:48, Rae Moar <rmoar@google.com> 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@google.com>
> Reviewed-by: Daniel Latypov <dlatypov@google.com>
> Reviewed-by: David Gow <davidgow@google.com>

I tried this patch, see the full boot log [1] and I can still see some
 tests that output the "old" format with 'ok 1 - kunit_test_1', for example

ok 1 - 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits

Isn't this something that should be converted too?

Cheers,
Anders
[1] http://ix.io/4gwf
Daniel Latypov Nov. 22, 2022, 5:14 p.m. UTC | #3 
On Tue, Nov 22, 2022 at 1:17 AM Anders Roxell <anders.roxell@linaro.org> wrote:
>
> On Mon, 21 Nov 2022 at 19:48, Rae Moar <rmoar@google.com> 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@google.com>
> > Reviewed-by: Daniel Latypov <dlatypov@google.com>
> > Reviewed-by: David Gow <davidgow@google.com>
>
> I tried this patch, see the full boot log [1] and I can still see some
>  tests that output the "old" format with 'ok 1 - kunit_test_1', for example
>
> ok 1 - 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits
>
> Isn't this something that should be converted too?

Yes, thanks for catching that.
That's what I get from only looking over the diff this time since I
thought I remembered the code...

We also want this diff to fix a) debugfs, b) subtests.

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/test.c b/lib/kunit/test.c
index 19344cb501c5..c9d57a1d9524 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -556,7 +556,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);

Daniel
Rae Moar Nov. 22, 2022, 8:19 p.m. UTC | #4 
On Tue, Nov 22, 2022 at 12:14 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Tue, Nov 22, 2022 at 1:17 AM Anders Roxell <anders.roxell@linaro.org> wrote:
> >
> > On Mon, 21 Nov 2022 at 19:48, Rae Moar <rmoar@google.com> 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@google.com>
> > > Reviewed-by: Daniel Latypov <dlatypov@google.com>
> > > Reviewed-by: David Gow <davidgow@google.com>
> >
> > I tried this patch, see the full boot log [1] and I can still see some
> >  tests that output the "old" format with 'ok 1 - kunit_test_1', for example
> >
> > ok 1 - 1901-12-13 Lower bound of 32bit < 0 timestamp, no extra bits
> >
> > Isn't this something that should be converted too?

Hello! Sorry for missing that. You are definitely right. Those results should
be converted as well.

>
> Yes, thanks for catching that.
> That's what I get from only looking over the diff this time since I
> thought I remembered the code...
>
> We also want this diff to fix a) debugfs, b) subtests.
>
> 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/test.c b/lib/kunit/test.c
> index 19344cb501c5..c9d57a1d9524 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -556,7 +556,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);
>
> Daniel

Thanks Daniel!

I think that should do the trick with the addition of adding the "KTAP
version 1" line, which you can do with the following diff:

diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index c9d57a1d9524..1c9d8d962d67 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -543,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);

Going to run through some more examples to make sure this version
works otherwise I will make a v3 with the addition of fixing any merge
conflicts.

Rae
diff mbox series

Patch

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..19344cb501c5 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 : "");