lkml.org 
[lkml]   [2020]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v6 1/2] kunit: Support for Parameterized Testing
    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?

    > My other suggestion -- albeit one outside the scope of this initial
    > version -- would be to allow the "param-%d" name to be overridden
    > somehow by a test. For example, the ext4 inode test has names for all
    > its test cases: it'd be nice to be able to display those instead (even
    > if they're not formatted as identifiers as-is).

    Right, I was thinking about this, but it'd need a way to optionally
    pass another function that converts const void* params to readable
    strings. But as you say, we should do that as a follow-up patch later
    because it might require a few more iterations.

    [...]

    Thanks,
    -- Marco

    \
     
     \ /
      Last update: 2020-11-07 11:07    [W:4.063 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site