RE: [PATCH v6 1/2] kunit: Support for Parameterized Testing

["Bird, Tim" <Tim.Bird@sony.com>]

> -----Original Message-----
> From: David Gow <davidgow@google.com>
> 
> On Mon, Nov 9, 2020 at 2:49 PM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
> >
> > On 07/11/20 3:36 pm, Marco Elver wrote:
> > > On Sat, 7 Nov 2020 at 05:58, David Gow <davidgow@google.com> wrote:
> > >> On Sat, Nov 7, 2020 at 3:22 AM Arpitha Raghunandan <98.arpi@gmail.com> wrote:
> > >>>
> > >>> Implementation of support for parameterized testing in KUnit.
> > >>> This approach requires the creation of a test case using the
> > >>> KUNIT_CASE_PARAM macro that accepts a generator function as input.
> > >>> This generator function should return the next parameter given the
> > >>> previous parameter in parameterized tests. It also provides
> > >>> a macro to generate common-case generators.
> > >>>
> > >>> Signed-off-by: Arpitha Raghunandan <98.arpi@gmail.com>
> > >>> Co-developed-by: Marco Elver <elver@google.com>
> > >>> Signed-off-by: Marco Elver <elver@google.com>
> > >>> ---
> > >>
> > >> This looks good to me! A couple of minor thoughts about the output
> > >> format below, but I'm quite happy to have this as-is regardless.
> > >>
> > >> Reviewed-by: David Gow <davidgow@google.com>
> > >>
> > >> Cheers,
> > >> -- David
> > >>
> > >>> Changes v5->v6:
> > >>> - Fix alignment to maintain consistency
> > >>> Changes v4->v5:
> > >>> - Update kernel-doc comments.
> > >>> - Use const void* for generator return and prev value types.
> > >>> - Add kernel-doc comment for KUNIT_ARRAY_PARAM.
> > >>> - Rework parameterized test case execution strategy: each parameter is executed
> > >>>   as if it was its own test case, with its own test initialization and cleanup
> > >>>   (init and exit are called, etc.). However, we cannot add new test cases per TAP
> > >>>   protocol once we have already started execution. Instead, log the result of
> > >>>   each parameter run as a diagnostic comment.
> > >>> Changes v3->v4:
> > >>> - Rename kunit variables
> > >>> - Rename generator function helper macro
> > >>> - Add documentation for generator approach
> > >>> - Display test case name in case of failure along with param index
> > >>> Changes v2->v3:
> > >>> - Modifictaion of generator macro and method
> > >>> Changes v1->v2:
> > >>> - Use of a generator method to access test case parameters
> > >>>
> > >>>  include/kunit/test.h | 36 ++++++++++++++++++++++++++++++++++
> > >>>  lib/kunit/test.c     | 46 +++++++++++++++++++++++++++++++-------------
> > >>>  2 files changed, 69 insertions(+), 13 deletions(-)
> > >>>
> > >>> diff --git a/include/kunit/test.h b/include/kunit/test.h
> > >>> index db1b0ae666c4..16616d3974f9 100644
> > >>> --- a/include/kunit/test.h
> > >>> +++ b/include/kunit/test.h
> > >>> @@ -107,6 +107,7 @@ struct kunit;
> > > [...]
> > >>> -       kunit_suite_for_each_test_case(suite, test_case)
> > >>> -               kunit_run_case_catch_errors(suite, test_case);
> > >>> +       kunit_suite_for_each_test_case(suite, test_case) {
> > >>> +               struct kunit test = { .param_value = NULL, .param_index = 0 };
> > >>> +               bool test_success = true;
> > >>> +
> > >>> +               if (test_case->generate_params)
> > >>> +                       test.param_value = test_case->generate_params(NULL);
> > >>> +
> > >>> +               do {
> > >>> +                       kunit_run_case_catch_errors(suite, test_case, &test);
> > >>> +                       test_success &= test_case->success;
> > >>> +
> > >>> +                       if (test_case->generate_params) {
> > >>> +                               kunit_log(KERN_INFO, &test,
> > >>> +                                         KUNIT_SUBTEST_INDENT
> > >>> +                                         "# %s: param-%d %s",
> > >>
> > >> Would it make sense to have this imitate the TAP format a bit more?
> > >> So, have "# [ok|not ok] - [name]" as the format? [name] could be
> > >> something like "[test_case->name]:param-[index]" or similar.
> > >> If we keep it commented out and don't indent it further, it won't
> > >> formally be a nested test (though if we wanted to support those later,
> > >> it'd be easy to add), but I think it would be nicer to be consistent
> > >> here.
> > >
> > > The previous attempt [1] at something similar failed because it seems
> > > we'd need to teach kunit-tool new tricks [2], too.
> > > [1] https://lkml.kernel.org/r/20201105195503.GA2399621@elver.google.com
> > > [2] https://lkml.kernel.org/r/20201106123433.GA3563235@elver.google.com
> > >
> > > So if we go with a different format, we might need a patch before this
> > > one to make kunit-tool compatible with that type of diagnostic.
> > >
> > > Currently I think we have the following proposals for a format:
> > >
> > > 1. The current "# [test_case->name]: param-[index] [ok|not ok]" --
> > > this works well, because no changes to kunit-tool are required, and it
> > > also picks up the diagnostic context for the case and displays that on
> > > test failure.
> > >
> > > 2. Your proposed "# [ok|not ok] - [test_case->name]:param-[index]".
> > > As-is, this needs a patch for kunit-tool as well. I just checked, and
> > > if we change it to "# [ok|not ok] - [test_case->name]: param-[index]"
> > > (note the space after ':') it works without changing kunit-tool. ;-)
> > >
> > > 3. Something like "# [ok|not ok] param-[index] - [test_case->name]",
> > > which I had played with earlier but kunit-tool is definitely not yet
> > > happy with.
> > >
> > > So my current preference is (2) with the extra space (no change to
> > > kunit-tool required). WDYT?
> > >
> 
> Hmm… that failure in kunit_tool is definitely a bug: we shouldn't care
> what comes after the comment character except if it's an explicit
> subtest declaration or a crash. I'll try to put a patch together to
> fix it, but I'd rather not delay this just for that.
> 
> In any thought about this a bit more, It turns out that the proposed
> KTAP spec[1] discourages the use of ':', except as part of a subtest
> declaration, or perhaps an as-yet-unspecified fully-qualified test
> name. The latter is what I was going for, but if it's actively
> breaking kunit_tool, we might want to hold off on it.
> 
> If we were to try to treat these as subtests in accordance with that
> spec, the way we'd want to use one of these options:
> A) "[ok|not ok] [index] - param-[index]" -- This doesn't mention the
> test case name, but otherwise treats things exactly the same way we
> treat existing subtests.
> 
> B) "[ok|not ok] [index] - [test_case->name]" -- This doesn't name the
> "subtest", just gives repeated results with the same name.
> 
> C) "[ok|not ok] [index] - [test_case->name][separator]param-[index]"
> -- This is equivalent to my suggestion with a separator of ":", or 2
> above with a separator of ": ". The in-progress spec doesn't yet
> specify how these fully-qualified names would work, other than that
> they'd use a colon somewhere, and if we comment it out, ": " is
> required.
> 
> >
> > Which format do we finally go with?
> >
> 
> I'm actually going to make another wild suggestion for this, which is
> a combination of (1) and (A):
> "# [test_case->name]: [ok|not ok] [index] - param-[index]"

I strongly object to putting actual testcase results in comments.
I'd rather that we found a way to include the parameter in the
sub-test-case name, rather than require all parsers to know about
specially-formatted comments.  There are tools outside
the kernel that parse these lines.

> 
> This gives us a KTAP-compliant result line, except prepended with "#
> [test_case->name]: ", which makes it formally a diagnostic line,
> rather than an actual subtest. Putting the test name at the start
> matches what kunit_tool is expecting at the moment. If we then want to
> turn it into a proper subtest, we can just get rid of that prefix (and
> add the appropriate counts elsewhere).
> 
> Does that sound good?
No.

I strongly prefer option C above:
"[ok|not ok] [index] - [test_case->name][separator]param-[index]"

Except of course the second index is not the same as the first, so it
would be:
"[ok|not ok] [index] - [test_case->name][separator]param-[param-index]"

If ':' is problematical as a separator, let's choose something else.
What are the allowed and disallowed characters in the testcase name now?
How bad would it be to use something like % or &?

Unless the separator is #, I think most parsers are going to just treat the whole
string from after the '[index] -' to a following '#' as a testcase name, and it
should get parsed (and presented) OK. I'm not sure what kunit_tool's problem is.

 -- Tim