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=-11.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no 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 CCC8FC388F7 for ; Tue, 10 Nov 2020 16:41:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6E6A620781 for ; Tue, 10 Nov 2020 16:41:17 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="dfqAtJ6i" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730232AbgKJQlQ (ORCPT ); Tue, 10 Nov 2020 11:41:16 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53112 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726827AbgKJQlN (ORCPT ); Tue, 10 Nov 2020 11:41:13 -0500 Received: from mail-oi1-x242.google.com (mail-oi1-x242.google.com [IPv6:2607:f8b0:4864:20::242]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AB748C0613D1 for ; Tue, 10 Nov 2020 08:41:13 -0800 (PST) Received: by mail-oi1-x242.google.com with SMTP id u127so15054768oib.6 for ; Tue, 10 Nov 2020 08:41:13 -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:content-transfer-encoding; bh=izOyzxAEdr+DI01iVm48YK7PFRjR8KW8FC4qJD6+q7g=; b=dfqAtJ6i1g/8zXDewDWAZzZC9ODZMAAauizp0O00z0plxOcWjiq0KhZFTkRW8WUpzD xJtbQbmovoOWkY7KEL8TmYp1Ps7mC/ncBjKhSQkRJuWbdQnVMMW7McBID1pKs2rM71Gb f2KQbU816XD5z+rw9xKdky5heu9LFFcInnynvZZhlddc+VabMKmKOLSe2s1FCMYk6npJ doLkpMOt1lcbPz2ZcvNYnJaGOySktBWJez1fFMmTo03xtQAAoOLV+LHqYcBLTZW6Sv/N qVaKQXRPvttdl4Viygj86GexG/+11Ow/QTyVxIw8mQmtGYRWwjZhMBLXwO7hhG+Ol1ik TKzg== 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:content-transfer-encoding; bh=izOyzxAEdr+DI01iVm48YK7PFRjR8KW8FC4qJD6+q7g=; b=h4oSF5Njg7oUtgmO5XBy4xpnIx8+nTc+kXGAUOUHk63ljcw5Gox27wRbea8CFiCu43 WRMgWjwMkh4XciqUnPBCczfq+/9fTqIghoOYFcV1eBKroEgZKhVNhP1A6OgwvDzg3p0X 68T1mgnByqGXbUST938x5PLTNCC/o3jRRtseDnnmh9ws8g8LbwYh5UfpX8RZmIORTtTR +kbOSo5hmUieG3Ns/G2qQXgew+IofsTCy860kIgqy/wG+EIbjB04wkTl6o0722+Ta8Tg yj4/ZCHAjOHddncg3OMep/4L+5abAYipbjhtQDpTZr0ghEN/I8TH8vJYT2tgiZCi/OW1 bC3g== X-Gm-Message-State: AOAM530h8Rfbnp2bnbxBcaVgbR7m7EMtelB0KBjry3eFaFR8Y/n1VUfF Sz+PcblbqLWuCPk+CgtzF2i/wMrTJNOIRyfSDaJyLw== X-Google-Smtp-Source: ABdhPJx/HhVmD026aUKZNmLBW8yx2d6rLrKnKvoUbe5Vy8uCbDu9yQ+y4hcX1WCBNzuJSZLOuwTc032J2CVqt3vmDBA= X-Received: by 2002:aca:6206:: with SMTP id w6mr3464315oib.121.1605026472655; Tue, 10 Nov 2020 08:41:12 -0800 (PST) MIME-Version: 1.0 References: <20201106192154.51514-1-98.arpi@gmail.com> <47a05c5a-485d-026b-c1c3-476ed1a97856@gmail.com> <06106c1a-7e1b-c317-91f2-7f9c64072794@gmail.com> In-Reply-To: <06106c1a-7e1b-c317-91f2-7f9c64072794@gmail.com> From: Marco Elver Date: Tue, 10 Nov 2020 17:41:00 +0100 Message-ID: Subject: Re: [PATCH v6 1/2] kunit: Support for Parameterized Testing To: Arpitha Raghunandan <98.arpi@gmail.com> Cc: David Gow , 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" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 10 Nov 2020 at 17:32, Arpitha Raghunandan <98.arpi@gmail.com> wrote= : > > On 10/11/20 4:05 pm, Marco Elver wrote: > > On Tue, 10 Nov 2020 at 08:21, David Gow wrote: > > [...] > >>>> > >>>> The previous attempt [1] at something similar failed because it seem= s > >>>> 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 th= is > >>>> 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, an= d > >>>> 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=E2=80=A6 that failure in kunit_tool is definitely a bug: we should= n'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]" > >> > >> 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? > > > > Sounds reasonable to me! The repetition of [index] seems unnecessary > > for now, but I think if we at some point have a way to get a string > > representation of a param, we can substitute param-[index] with a > > string that represents the param. > > > > So, with this the inode-test.c will have the following output, right? > > TAP version 14 > 1..7 > # Subtest: ext4_inode_test > 1..1 > # inode_test_xtimestamp_decoding: ok 0 - param-0 So the params should certainly be 0-indexed, but I think TAP starts counting at 1, so maybe this should be: # inode_test_xtimestamp_decoding: ok 1 - param-0 and so on... ? Right now it doesn't matter, but it will if we make these subsubtests as David suggested. > # inode_test_xtimestamp_decoding: ok 1 - param-1 > # inode_test_xtimestamp_decoding: ok 2 - param-2 > # inode_test_xtimestamp_decoding: ok 3 - param-3 > # inode_test_xtimestamp_decoding: ok 4 - param-4 > # inode_test_xtimestamp_decoding: ok 5 - param-5 > # inode_test_xtimestamp_decoding: ok 6 - param-6 > # inode_test_xtimestamp_decoding: ok 7 - param-7 > # inode_test_xtimestamp_decoding: ok 8 - param-8 > # inode_test_xtimestamp_decoding: ok 9 - param-9 > # inode_test_xtimestamp_decoding: ok 10 - param-10 > # inode_test_xtimestamp_decoding: ok 11 - param-11 > # inode_test_xtimestamp_decoding: ok 12 - param-12 > # inode_test_xtimestamp_decoding: ok 13 - param-13 > # inode_test_xtimestamp_decoding: ok 14 - param-14 > # inode_test_xtimestamp_decoding: ok 15 - param-15 > ok 1 - inode_test_xtimestamp_decoding > ok 1 - ext4_inode_test > > I will send another patch with this change. > Thanks! Yes that looks right, but see the comment above ^ Thanks, -- Marco