From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D6BFBC388F9 for ; Sat, 7 Nov 2020 10:07:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8D11F20888 for ; Sat, 7 Nov 2020 10:07:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="cFZKFASw" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728170AbgKGKGo (ORCPT ); Sat, 7 Nov 2020 05:06:44 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:57792 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728091AbgKGKGm (ORCPT ); Sat, 7 Nov 2020 05:06:42 -0500 Received: from mail-oi1-x244.google.com (mail-oi1-x244.google.com [IPv6:2607:f8b0:4864:20::244]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DC759C0613D3 for ; Sat, 7 Nov 2020 02:06:40 -0800 (PST) Received: by mail-oi1-x244.google.com with SMTP id o25so3456057oie.5 for ; Sat, 07 Nov 2020 02:06:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=TErsauc9tpAzI6S5cYpbCRvl4twcQzw2HKSJ2d2Z+Qg=; b=cFZKFASwAfugNcZhz/FuAFmwgIJgslvWVTlXGObXW4GVaiNx5l+4SITTnhwIes3oiJ reYnqBcoLt6G3wNTxTLRdKwgOg7Ns7cE5FqN5L4rV7tMFTjEp2540lKCJlUvc52aoVKR 0YjZ0+zMCT0aW++pNadjtwASHdwbGcnxya4d7uBY6bTaj1NGc6B9kOGdHCcbOMUKZvXf bJd1gYeIAayqwjDTVfshWKIZxHJX7MEwCH3td/atGMHmO6tHjcBdloKYwzPy6HyHIlWy +nIGIc8yTT0VDKRsKtvKA7fit+ZKdqNJdSHlD/fEWKCd9D7kRQN5jIMqPlF0YA+MdZGz t+Ug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=TErsauc9tpAzI6S5cYpbCRvl4twcQzw2HKSJ2d2Z+Qg=; b=cLVuwsxkmCjdkxvps36QBvj0mh5AjGH/x2AYC2re5hUaJ7QmBRnGj7fRaECpIU9v7b iPvqKzvQgUYlD5NQPyAtbFic0wF9qDDXU2nS+zO5dhm0ARCPRCC+YQnjOOvV3G8oivw9 h6xx1DVPECykr4ozGPVzYl6h2a2YMILVPgrPhN7UW4C9Sf/ti10H26C3a+OpF7iHRSFJ r6v5AhbgoU8y1/+raTjOdHTt2s9FBFYMeuIfjClwFf9cy1FEgt9BEMvz4vP8MGE97x6G ZFiukw9gSvsMWCqD04DeRO/hwZjspM8JArtn6gUswlB3M+Gmr8Rd/miLZv1vccDE/QgO dnvw== X-Gm-Message-State: AOAM530SdqZ5RZhS6gXasONHQI8rKpEjUnbEOCpf9ndPdSeXSkTqCKz5 NqQuTBv9zGOZqlhGPGHU0tTMn77yE0RkMZe5zBw66Q== X-Google-Smtp-Source: ABdhPJxb+xfOZ9jVhfWoVlbABCI67vL5rewh1ARjZCLWNyu463apeEQlWxjA1SZroi07HqVGkXthuRthzuku33QQCxw= X-Received: by 2002:aca:6206:: with SMTP id w6mr3818129oib.121.1604743599913; Sat, 07 Nov 2020 02:06:39 -0800 (PST) MIME-Version: 1.0 References: <20201106192154.51514-1-98.arpi@gmail.com> In-Reply-To: From: Marco Elver Date: Sat, 7 Nov 2020 11:06:28 +0100 Message-ID: Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing To: David Gow Cc: Arpitha Raghunandan <98.arpi@gmail.com>, Brendan Higgins , Shuah Khan , Iurii Zaikin , "Theodore Ts'o" , Andreas Dilger , "open list:KERNEL SELFTEST FRAMEWORK" , KUnit Development , Linux Kernel Mailing List , linux-kernel-mentees@lists.linuxfoundation.org, linux-ext4@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 7 Nov 2020 at 05:58, David Gow 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 > > Signed-off-by: Marco Elver > > --- > > 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 > > 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