Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
From: David Gow <davidgow@google.com>
To: Matthieu Baerts <matthieu.baerts@tessares.net>,
	Nico Pache <npache@redhat.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Brendan Higgins <brendanhiggins@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-ext4@vger.kernel.org, Networking <netdev@vger.kernel.org>,
	rafael@kernel.org, linux-m68k@lists.linux-m68k.org,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	"Theodore Ts'o" <tytso@mit.edu>,
	mathew.j.martineau@linux.intel.com, davem@davemloft.net,
	Mark Brown <broonie@kernel.org>,
	Shuah Khan <skhan@linuxfoundation.org>,
	mptcp@lists.linux.dev
Subject: Re: [PATCH v2 5/6] kunit: mptcp: adhear to KUNIT formatting standard
Date: Thu, 15 Apr 2021 14:01:10 +0800
Message-ID: <CABVgOSm9Lfcu--iiFo=PNLCWCj4vkxqAqO0aZT9B2r3Kw5Fhaw@mail.gmail.com> (raw)
In-Reply-To: <e26fbcc8-ba3e-573a-523d-9c5d5f84bc46@tessares.net>

Hi Nico, Matthieu,

Thanks for going to the trouble of making these conform to the KUnit
style guidelines.

On Wed, Apr 14, 2021 at 5:25 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Nico,
>
> On 14/04/2021 10:58, Nico Pache wrote:
> > Drop 'S' from end of CONFIG_MPTCP_KUNIT_TESTS inorder to adhear to the
> > KUNIT *_KUNIT_TEST config name format.

[Super nitpicky comment for this series: It's 'adhere', not 'adhear'.
It's not worth resending this out just to fix the spelling here, but
if you do another version, it might be worth fiing.]

>
> For MPTCP, we have multiple KUnit tests: crypto and token. That's why we
> wrote TESTS with a S.

So (as this patch series attests), there are a few different config
options which cover (or intend one day to cover) multiple suites, and
hence end in KUNIT_TESTS. Personally, I'd still slightly prefer TEST
here, just to have a common suffix for KUnit test options, and that's
what I was going for when writing the style guide.

That being said, it's also worth noting that there is an explicit
exemption for existing tests, so if you (for example) have a bunch of
automation which relies on this name and can't easily be changed,
that's probably more important than a lone 'S'.

> I'm fine without S if we need to stick with KUnit' standard. But maybe
> the standard wants us to split the two tests and create
> MPTCP_TOKEN_KUNIT_TEST and MPTCP_TOKEN_KUNIT_TEST config?
>
> In this case, we could eventually keep MPTCP_KUNIT_TESTS which will in
> charge of selecting the two new ones.

This is certainly an option if you want to do it, but personally I
wouldn't bother unless it gives you some real advantage. One thing I
would note, however, is that it's possible to have a per-subsystem
'.kunitconfig' file, so if you want to run a particular group of tests
(i.e., have a particular set of config options for testing), it'd be
possible to have that rather than a meta-Kconfig entry.

While there are some advantages to trying to have a  1:1 suite:config
mapping, there's isn't actually anything that depends on it at the
moment (or indeed, anything actively planned). So, in my view, there's
no need for you to split the config entry unless you think there's a
good reason you'd want to be able to build one set of tests but not
the other.

> Up to the KUnit maintainers to decide ;-)

To summarise my view: personally, I'd prefer things the way this patch
works: have everything end in _KUNIT_TEST, even if that enables a
couple of suites. The extra 'S' on the end isn't a huge problem if you
have a good reason to particularly want to keep it, though: as long as
you don't have something like _K_UNIT_VERIFICATION or something
equally silly that'd break grepping for '_KUNIT_TEST', it's fine be
me.

So, since it matches my prejudices, this patch is:

Reviewed-by: David Gow <davidgow@google.com>

Thanks,
-- David

  reply index

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-14  8:58 [PATCH v2 0/6] kunit: Fix formatting of KUNIT tests to meet the standard Nico Pache
2021-04-14  8:58 ` [PATCH v2 1/6] kunit: ASoC: topology: adhear to KUNIT formatting standard Nico Pache
2021-04-14 14:20   ` Mark Brown
2021-04-14  8:58 ` [PATCH v2 2/6] kunit: software node: " Nico Pache
2021-04-14  8:58 ` [PATCH v2 3/6] kunit: ext4: " Nico Pache
2021-04-18 19:41   ` Theodore Ts'o
2021-04-14  8:58 ` [PATCH v2 4/6] kunit: lib: " Nico Pache
2021-04-14  8:58 ` [PATCH v2 5/6] kunit: mptcp: " Nico Pache
2021-04-14  9:25   ` Matthieu Baerts
2021-04-15  6:01     ` David Gow [this message]
2021-04-15  7:10       ` Matthieu Baerts
2021-04-17  4:24         ` David Gow
2021-04-17  8:02           ` Matthieu Baerts
2021-04-19  7:40           ` Geert Uytterhoeven
2021-04-14  8:58 ` [PATCH v2 6/6] m68k: update configs to match the proper KUNIT syntax Nico Pache
2021-04-14 16:06 ` (subset) [PATCH v2 0/6] kunit: Fix formatting of KUNIT tests to meet the standard Mark Brown
2021-04-18 19:39 ` Theodore Ts'o
2021-04-22 20:39   ` Nico Pache
2021-04-23  6:26     ` David Gow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABVgOSm9Lfcu--iiFo=PNLCWCj4vkxqAqO0aZT9B2r3Kw5Fhaw@mail.gmail.com' \
    --to=davidgow@google.com \
    --cc=brendanhiggins@google.com \
    --cc=broonie@kernel.org \
    --cc=davem@davemloft.net \
    --cc=geert@linux-m68k.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-m68k@lists.linux-m68k.org \
    --cc=mathew.j.martineau@linux.intel.com \
    --cc=matthieu.baerts@tessares.net \
    --cc=mptcp@lists.linux.dev \
    --cc=netdev@vger.kernel.org \
    --cc=npache@redhat.com \
    --cc=rafael@kernel.org \
    --cc=skhan@linuxfoundation.org \
    --cc=tytso@mit.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git