lkml.org 
[lkml]   [2020]   [Nov]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v6 1/2] kunit: Support for Parameterized Testing
    Date


    > -----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

    \
     
     \ /
      Last update: 2020-11-10 19:44    [W:2.319 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site