Message-ID: <CABVgOSkp0tyQN7NRd=Tpss33xSnBb5vzuvz952rGaA0UAS0UBg@mail.gmail.com>
Date: Wed, 24 Jul 2024 13:04:39 +0800
From: David Gow <davidgow@...gle.com>
To: Marco Elver <elver@...gle.com>
Cc: Kees Cook <kees@...nel.org>, Brendan Higgins <brendan.higgins@...ux.dev>, 
	Rae Moar <rmoar@...gle.com>, John Hubbard <jhubbard@...dia.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 v2] Documentation: KUnit: Update filename best practices

On Mon, 22 Jul 2024 at 17:56, Marco Elver <elver@...gle.com> wrote:
>
> On Sat, Jul 20, 2024 at 09:54AM -0700, Kees Cook wrote:
> > Based on feedback from Linus[1] and follow-up discussions, 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>
> [...]
> >  Documentation/dev-tools/kunit/style.rst | 25 +++++++++++++++----------
> >  1 file changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/Documentation/dev-tools/kunit/style.rst b/Documentation/dev-tools/kunit/style.rst
> > index b6d0d7359f00..1538835cd0e2 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 file should be named
> > +after the test suite, followed by ``_kunit``, and live in a ``tests``
> > +subdirectory to avoid conflicting with regular modules (e.g. if "foobar"
> > +is the core module, then "foobar_kunit" is the KUnit test module) or the
> > +core kernel source file names (e.g. for tab-completion). Many existing
> > +tests use a ``_test`` suffix, but this is considered deprecated.
>
> What's wrong with the `_test` suffix (if inside a "tests" subdir)?
>
> Rules are good, but please can we retain some common sense?
>
> I understand the requirement for adding things to a "tests" subdir, so
> that $foo.c is not right next to a $foo_test.c or $foo_kunit.c.
>
> There are exception, however, if there is no $foo.c. For example:
>
>         - mm/kfence/kfence_test.c
>         - kernel/kcsan/kcsan_test.c
>         - mm/kmsan/kmsan_test.c
>
> In all these cases it'd be very annoying to move things into a "tests"
> subdir, because there's only 1 test, and there isn't even a $foo.c file.
> While there's a $foo.h file, I consider deeper directory nesting with 1
> file in the subdir to be more annoying.
>
> The rules should emphasize some basic guidelines, as they have until
> now, and maybe add some additional suggestions to avoid the problem that
> Linus mentioned. But _overfitting_ the new rules to avoid that single
> problem is just adding more friction elsewhere if followed blindly.
>

I agree in principle here: the purpose of these is very much to be
"guidelines" rather than "rules". Certainly the idea was that
individual maintainers could interpret and/or override these to best
fit their subsystem. (But, obviously, it's best if there's a reason to
do so.)

Ultimately, we have one major new guideline:
- Avoid having multiple files in the same directory with the same
prefix, probably by placing test files in a tests/ subdirectory.

And one revised guideline:
- Test modules should be named with a suffix to distinguish them from
the code being tested. Using the "_kunit" suffix makes it easier to
search for KUnit tests, and clarifies that these are KUnit tests.

I don't think there's much need to quickly find all KUnit test modules
by looking for _kunit.ko, though. While it could be handy, we already
have mechanisms for configuring KUnit tests (CONFIG_KUNIT_ALL_TESTS)
and detecting if a module contains KUnit tests (look for the
'.kunit_test_suites' section). So the distinction between '_test' and
'_kunit' is really only there for humans, and it doesn't matter one
way or the other if all of a subsystem's tests use KUnit. If there are
a mix of KUnit and non-KUnit tests, then making the KUnit ones end in
_kunit was already suggested, so we're really just changing the
default. It's slightly complicated by the existence of
"non-unit-tests" using KUnit, which may not want to get caught up
automatically in lists of KUnit tests. I think that's a case of
common-sense, but since we're not really using filenames as a way of
listing all tests anyway, using CONFIG_KUNIT_ALL_TESTS and the 'slow'
attribute probably makes more sense from a tooling perspective,
anyway.

> > +So for the common case, name the file containing the test suite
> > +``tests/<suite>_kunit.c``. The ``tests`` directory should be placed at
> > +the same level as the code under test. For example, tests for
> > +``lib/string.c`` live in ``lib/tests/string_kunit.c``.
> >
> >  If the suite name contains some or all of the name of the test's parent
> > -directory, it may make sense to modify the source filename to reduce redundancy.
> > -For example, a ``foo_firmware`` suite could be in the ``foo/firmware_test.c``
> > -file.
> > +directory, it may make sense to modify the source filename to reduce
> > +redundancy. For example, a ``foo_firmware`` suite could be in the
> > +``tests/foo/firmware_kunit.c`` file.
>
> I'm more confused now. This is just moving tests further away from what
> they are testing for no good reason. If there's a directory "foo", then
> moving things to "tests/foo" is unclear. It's unclear if "tests" is
> inside parent of "foo" or actually a subdir of "foo". Per the paragraph
> above, I inferred it's "foo/tests/foo/...", which is horrible. If it's
> "../tests/foo/..." it's also bad because it's just moving tests further
> away from what they are testing.
>
> And keeping tests close to the source files under test is generally
> considered good practice, as it avoids the friction required to discover
> where tests live. Moving tests to "../tests" or "../../*/tests" in the
> majority of cases is counterproductive.
>
> It is more important for people to quickly discover tests nearby and
> actually run them, vs. having them stashed away somewhere so they don't
> bother us.

I definitely agree that we should encourage tests to be alongside the
code being tested (whether in a subdirectory or not), and not in an
ancestor or sibling directory (so, no "../tests" or "../../tests").
Though I can see that making sense for some subsystems which already
have established "tests" directories (e.g. DRM), so it's not a
never-break-this rule.

> While we can apply common sense, all too often someone follows these
> rules blindly and we end up with a mess.

Agreed. The goal here is definitely to describe a 'sensible default'.
Once we're hitting unusual cases, though, this will have to be a
matter of common sense and maintainer discretion. Trying to come up
with an exhaustive list of rules seems a fool's errand to me.

Cheers,
-- David

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4014 bytes)

Powered by blists - more mailing lists