Message-ID: <f0d102a8-6501-88d8-d47d-202fd36e55a5@gmail.com>
Date:   Tue, 18 Feb 2020 19:18:52 -0600
From:   Frank Rowand <frowand.list@...il.com>
To:     "Bird, Tim" <Tim.Bird@...y.com>,
        Brendan Higgins <brendanhiggins@...gle.com>
Cc:     Alan Maguire <alan.maguire@...cle.com>,
        Greg KH <gregkh@...uxfoundation.org>, shuah <shuah@...nel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>,
        David Gow <davidgow@...gle.com>
Subject: Re: [PATCH v3 kunit-next 1/2] kunit: add debugfs
 /sys/kernel/debug/kunit/<suite>/results display

On 2/18/20 2:49 PM, Bird, Tim wrote:
> 
> 
>> -----Original Message-----
>> From:  Brendan Higgins
>>
>> On Wed, Feb 12, 2020 at 7:25 PM Frank Rowand <frowand.list@...il.com> wrote:
>>>
>>> On 2/7/20 10:58 AM, Alan Maguire wrote:
> 
> ...
> 
>>>> diff --git a/lib/kunit/test.c b/lib/kunit/test.c
>>>> index 9242f93..aec607f 100644
>>>> --- a/lib/kunit/test.c
>>>> +++ b/lib/kunit/test.c
>>>> @@ -10,6 +10,7 @@
>>>>  #include <linux/kernel.h>
>>>>  #include <linux/sched/debug.h>
>>>>
>>>> +#include "debugfs.h"
>>>>  #include "string-stream.h"
>>>>  #include "try-catch-impl.h"
>>>>
>>>> @@ -28,73 +29,91 @@ static void kunit_print_tap_version(void)
>>>>       }
>>>>  }
>>>>
>>>> -static size_t kunit_test_cases_len(struct kunit_case *test_cases)
>>>> +size_t kunit_suite_num_test_cases(struct kunit_suite *suite)
>>>>  {
>>>>       struct kunit_case *test_case;
>>>>       size_t len = 0;
>>>>
>>>> -     for (test_case = test_cases; test_case->run_case; test_case++)
>>>> +     kunit_suite_for_each_test_case(suite, test_case)
>>>>               len++;
>>>>
>>>>       return len;
>>>>  }
>>>> +EXPORT_SYMBOL_GPL(kunit_suite_num_test_cases);
>>>>
>>>>  static void kunit_print_subtest_start(struct kunit_suite *suite)
>>>>  {
>>>>       kunit_print_tap_version();
>>>> -     pr_info("\t# Subtest: %s\n", suite->name);
>>>> -     pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
>>>> +     kunit_log(KERN_INFO, suite, "# Subtest: %s", suite->name);
>>>> +     kunit_log(KERN_INFO, suite, "1..%zd",
>>>> +               kunit_suite_num_test_cases(suite));
>>>
>>> The subtest 'is a TAP stream indented 4 spaces'.  (So the old code was
>>> also incorrect since it indented with a tab.)
>>
>> Whoops.
>>
>> I agree that fixing tabs to spaces is probably the easiest thing to do
>> here; nevertheless, I think this might be a good time to talk about
>> other deviations from the spec and what to do about it. This might
>> also be a good time to bring up Tim's comment at LPC last year about
>> forking TAP. Arguably I already have given that TAP14 is still under
>> review and is consequently subject to change.
>>
>> Additionally, the way I report expectation/assertion failures are my
>> own extension to the TAP spec. I did this because at the time I wasn't
>> ready to open the can of worms that was adding a YAML serializer to
>> the Linux kernel; I mentioned adding a YAML serializer at LPC and
>> people didn't seem super thrilled with the idea.
> 
> I'm not sure I follow.  Are you talking about writing YAML or interpreting
> YAML.  You don't need a serializer to write YAML.  It can be done 
> with straight text output.  I guess it depends on the scope of what you
> envision.  Even if you want to do more than trivial structured output,
> I don't think you'll need a full serializer.  (IOW, I think you could sneak
> something in and just call it a test output formatter.  Just don't call it YAML
> and most people won't notice. :-)

A serializer is a red herring.  Just drop the entire label and concept.
What is already in KUnit is the equivalent, printing out (eg through
printk() or however printk() is wrapped) simple text of the form (by
example) of

  label: value
  label: value
  label:
    label: value
    label: value
  label:
     label:
       label: value
       label: value

So basically
  - label/value pairs
  - label without value indicating a node or block (another level)
  - some rules about the format of value
  - indentation indicating a node or block

That is something really simple.  No need for any fancy coding to
encapsulate.

-Frank

> 
>>
>> Further both the TAP implementation here as well as what is in
>> kselftest have arbitrary kernel output mixed in with TAP output, which
>> seems to be a further deviation from the spec.
> Well that's a different kettle of worms, and really argues for staying
> with something that is strictly line-based.
> 
>>
>> In an effort to do this, and so that at the very least I could
>> document what I have done here, I have been looking into getting a
>> copy of TAP into the kernel. Unfortunately, TAP appears to have some
>> licensing issues. TAP says that it can be used/modified "under the
>> same terms as Perl itself" and then provides a dead link. I filed a
>> pull request to update the licence to the Perl Artistic Licence 1.0
>> since I believe that is what they are referencing; however, I have not
>> heard back from them yet.
> 
> When you say "getting a copy of TAP into the kernel", I presume you mean
> an existing implementation to produce TAP output?  Or are you talking about
> a TAP interpreter?  I'm not sure the former needs to use an existing implementation.
> 
> I previously volunteered (in Lisbon) to write up the TAP deviations,
> and never got around to it.   Sorry about that. I can try to work on it now if
> people are still interested.
>  -- Tim
> 
> [rest of patch omitted]
> 
> 

Powered by blists - more mailing lists