| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABVgOSnUDNvs6mYwVtzXq3+PmO62HG1pP=d_6EQiwOKF_9D6XA@mail.gmail.com>
Date: Tue, 15 Nov 2022 15:27:39 +0800
From: David Gow <davidgow@...gle.com>
To: Rae Moar <rmoar@...gle.com>
Cc: brendanhiggins@...gle.com, dlatypov@...gle.com,
skhan@...uxfoundation.org, mauro.chehab@...ux.intel.com,
kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org,
linux-kselftest@...r.kernel.org,
Isabella Basso <isabbasso@...eup.net>,
Anders Roxell <anders.roxell@...aro.org>
Subject: Re: [PATCH v1 1/2] kunit: improve KTAP compliance of KUnit test output
On Sat, Nov 5, 2022 at 3:48 AM Rae Moar <rmoar@...gle.com> wrote:
>
> Change KUnit test output to comply with KTAP version 1 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
>
> 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
> # Subtest: kunit-test-suite
> KTAP version 1
> 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@...gle.com>
> ---
Thanks very much for doing this. It's always been slightly
embarrassing that we weren't perfectly following our own spec!
I think this series is good enough, but have a minor suggestion: could
we swap the order of these two patches?
I'd rather have the parser changes go in first.
I'd also be curious to see if this is likely to break anyone else's
(K)TAP parsers.
+Isabella, Anders: do these changes break the IGT or LKFT TAP/KTAP
parsing? From a quick look at [1] and [2], we're probably okay??
[1]: https://gitlab.freedesktop.org/isinyaaa/igt-gpu-tools/-/commit/1a84306425e975377eb79c031bf1de147bd44e46
[2]: https://github.com/Linaro/test-definitions/blob/master/automated/linux/kunit/kunit.sh
I also looked into the possibility of moving or removing the Subtest
line, but upon further thought (see below), it's probably best to keep
it as-is here for now. That should be the closest to the current spec,
and we can possibly find a better way to provide the name in KTAPv2.
Reviewed-by: David Gow <davidgow@...gle.com>
Cheers,
-- David
> lib/kunit/executor.c | 6 +++---
> lib/kunit/test.c | 5 +++--
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> 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..b541d59a05c3 100644
> --- a/lib/kunit/test.c
> +++ b/lib/kunit/test.c
> @@ -151,6 +151,7 @@ static void kunit_print_suite_start(struct kunit_suite *suite)
> {
> kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "# Subtest: %s",
> suite->name);
> + kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "KTAP version 1\n");
Would it make sense to have the Subtest line _after_ the KTAP line here?
Given KTAP doesn't specify the "Subtest" line at all, I guess it doesn't matter.
A part of me feels that the "KTAP version 1" line should be the
_first_ line of the subtest output, but that would introduce a line
between it and the test plan, which goes against the spec.
We could also just get rid of the "Subtest" line, given it's not
technically required, though having the test name before the results
is really useful.
Having tried all three options while writing this email, I think it's
probably best to leave this patch as is (with the Subtest line
followed by the KTAP line), and discuss standardising something
similar as part of the KTAP v2 spec.
> kunit_log(KERN_INFO, suite, KUNIT_SUBTEST_INDENT "1..%zd",
> kunit_suite_num_test_cases(suite));
> }
> @@ -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 : "");
>
> base-commit: 6fe1ad4a156095859721fef85073df3ed43081d4
> --
> 2.38.1.431.g37b22c650d-goog
>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)
Powered by blists - more mailing lists