On Tue, Nov 15, 2022 at 12:18 PM Rae Moar <rmoar@xxxxxxxxxx> wrote: > Yes, I agree. I think it does make more sense to provide KTAP > compatibility with the parser before changing the test output. This > would also help to solve the issue that Daniel brought up on this > patch about the "KTAP version 1" line and test plan being stored > in the test.log as random kernel output. I will swap the patches in > the v2 of this patch series. > > > > > 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@xxxxxxxxxx> > > > > 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. > > > > I also struggle to decide how the "Subtest" line should be handled. Since > the current KTAP v1 spec does not provide a way to declare the name of > a test with subtests prior to the results, I think it is important to continue > to include this Subtest line because it provides that functionality. > Additionally, > the line is not expected to be very disruptive for other parsers because it > is read as a diagnostic line. Yeah, since it's going to be ignored as a diagnostic line, I think we're largely free to put it where we want? I'm actually leaning towards making things more uniform e.g. KTAP version 1 # Subtest: optionally set for the top-level test! 1..2 KTAP version 1 # Subtest: suite1 1..1 ok 1 - test1 ok 1 -suite1 // etc. Then we can simplify the parser by not differentiating (as much) between the top-level test and subtests. This also simplifies parsing multiple KTAP documents (e.g. for supporting modules, etc.) We'll probably talk about this offline soon, but I wanted to put this out there. Daniel > > The location of the "Subtest" line before the KTAP version line is potentially > not the most optimal but it seems to be the best choice while ensuring > compatibility with the current KTAP v1 spec. I recommend that we discuss > a standardized replacement for this "Subtest" line in the KTAP v2 spec.
