linux-kernel - Re: [PATCH] Documentation: KUnit: Update filename best practices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSmHBUrGzaACggEQhisrPqXGcL7j=SvsAsh0_=SAzCMjQA@mail.gmail.com>
Date: Thu, 18 Jul 2024 13:56:46 +0800
From: David Gow <davidgow@...gle.com>
To: Kees Cook <kees@...nel.org>
Cc: John Hubbard <jhubbard@...dia.com>, Brendan Higgins <brendan.higgins@...ux.dev>, 
	Rae Moar <rmoar@...gle.com>, Jonathan Corbet <corbet@....net>, 
	Linus Torvalds <torvalds@...ux-foundation.org>, linux-kselftest@...r.kernel.org, 
	kunit-dev@...glegroups.com, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] Documentation: KUnit: Update filename best practices

On Thu, 18 Jul 2024 at 06:11, Kees Cook <kees@...nel.org> wrote:
>
> On Wed, Jul 17, 2024 at 02:16:30PM -0700, John Hubbard wrote:
> > On 7/17/24 2:00 PM, Kees Cook wrote:
> > > Based on feedback from Linus[1], change the suggested file naming for
> > > KUnit tests.
> > >
> > > Link: https://lore.kernel.org/lkml/CAHk-=wgim6pNiGTBMhP8Kd3tsB7_JTAuvNJ=XYd3wPvvk=OHog@mail.gmail.com/ [1]
> > > Signed-off-by: Kees Cook <kees@...nel.org>
> > > ---
> > > Cc: David Gow <davidgow@...gle.com>
> > > Cc: Brendan Higgins <brendan.higgins@...ux.dev>
> > > Cc: Rae Moar <rmoar@...gle.com>
> > > Cc: Jonathan Corbet <corbet@....net>
> > > Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> > > Cc: linux-kselftest@...r.kernel.org
> > > Cc: kunit-dev@...glegroups.com
> > > Cc: linux-doc@...r.kernel.org
> > > ---
> > >   Documentation/dev-tools/kunit/style.rst | 21 +++++++++++++--------
> > >   1 file changed, 13 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/Documentation/dev-tools/kunit/style.rst b/Documentation/dev-tools/kunit/style.rst
> > > index b6d0d7359f00..761dee3f89ca 100644
> > > --- a/Documentation/dev-tools/kunit/style.rst
> > > +++ b/Documentation/dev-tools/kunit/style.rst
> > > @@ -188,15 +188,20 @@ For example, a Kconfig entry might look like:
> > >   Test File and Module Names
> > >   ==========================
> > > -KUnit tests can often be compiled as a module. These modules should be named
> > > -after the test suite, followed by ``_test``. If this is likely to conflict with
> > > -non-KUnit tests, the suffix ``_kunit`` can also be used.
> > > -
> > > -The easiest way of achieving this is to name the file containing the test suite
> > > -``<suite>_test.c`` (or, as above, ``<suite>_kunit.c``). This file should be
> > > -placed next to the code under test.
> > > +Whether a KUnit test is compiled as a separate module or via an
> > > +``#include`` in a core kernel source file, the files should be named
> > > +after the test suite, followed by ``_test``, and live in a ``tests``
> >
> > I read the previous discussion in the other thread and thought about it.
> > And ran some kunit tests on baremetal. Delightful! I love this approach.
> >
> > However! It is rather distinct and not just any old test module. Kunit
> > has clear conventions and behavior.

I fear the conventions and behaviour aren't _quite_ as clear as would
be ideal (for both good and bad reasons). In particular:
- Tests which don't use KUnit nevertheless are starting to have some
KUnit-like behaviour (e.g., KTAP-formatted output); and
- There exist some non-unit tests (e.g., slower integration-y tests)
which nevertheless use the KUnit framework

When we last discussed the naming scheme, one proposal was to use
_kunit only for tests which were fully "unit" tests (excluding the
last batch), though now that we have the 'speed' attribute and
similar, I don't think that's as useful as it once was.

> > As such, I have quickly become convinced that distinct naming is
> > required here. So I'd like to suggest going with the the suffix:
> >
> >     _kunit
> >
> > ...unconditionally. After all, sometimes you'll end up with that
> > anyway, so clearly, the _test suffix isn't strictly required.
> >
> > And given that we are putting these in tests/ , a _test suffix is
> > redundant.
> >
> > Yes?
>
> I would agree. David, what do you think? I realize drm already does
> tests/*_test.c, but it does seem like better information density to use
> the tests/*_kunit.c pattern by default?

I personally don't mind either way. Other than the argument that KUnit
is simply an implementation detail (and that renaming things makes
porting tests to KUnit slightly more annoying), I don't think there's
anything wrong with using _kunit, and it would give users an easier
way to know if they can use KUnit tooling with it.

Let's do _kunit by default, then.

-- David