[PATCH v2] Documentation: KUnit: Update filename best practices

[PATCH v2] Documentation: KUnit: Update filename best practices

[Kees Cook]

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@kernel.org>
---
Cc: David Gow <davidgow@google.com>
Cc: Brendan Higgins <brendan.higgins@linux.dev>
Cc: Rae Moar <rmoar@google.com>
Cc: John Hubbard <jhubbard@nvidia.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-kselftest@vger.kernel.org
Cc: kunit-dev@googlegroups.com
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-hardening@vger.kernel.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.
+
+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.
-- 
2.34.1

Re: [PATCH v2] Documentation: KUnit: Update filename best practices

[John Hubbard]

On 7/20/24 9:54 AM, 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@kernel.org>
> ---
> Cc: David Gow <davidgow@google.com>
> Cc: Brendan Higgins <brendan.higgins@linux.dev>
> Cc: Rae Moar <rmoar@google.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: linux-kselftest@vger.kernel.org
> Cc: kunit-dev@googlegroups.com
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-hardening@vger.kernel.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.

For this paragraph, may I suggest this wording below? It attempts to
explain the _kunit a bit (without leaving anything behind that would need
to be changed later, if/when people rename things from _test.c to _kunit.c),
as well as fixing up the sentence structure slightly:


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. This is to avoid conflicting with regular modules (e.g. if
"foobar" is the core module, then "foobar_kunit" is the KUnit test
module) or with the core kernel source file names (e.g. for
tab-completion). The ``_kunit`` suffix was chosen over the older (and
now deprecated) ``_test`` suffix, because KUnit behavior is sufficiently
distinct that it is worth identifying at file name level.


> +
> +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.

Whether you use that wording or not, this looks good, so:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v2] Documentation: KUnit: Update filename best practices

[Kees Cook]

On Sat, Jul 20, 2024 at 10:59:10AM -0700, John Hubbard wrote:
> On 7/20/24 9:54 AM, 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@kernel.org>
> > ---
> > Cc: David Gow <davidgow@google.com>
> > Cc: Brendan Higgins <brendan.higgins@linux.dev>
> > Cc: Rae Moar <rmoar@google.com>
> > Cc: John Hubbard <jhubbard@nvidia.com>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: linux-kselftest@vger.kernel.org
> > Cc: kunit-dev@googlegroups.com
> > Cc: linux-doc@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-hardening@vger.kernel.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.
> 
> For this paragraph, may I suggest this wording below? It attempts to
> explain the _kunit a bit (without leaving anything behind that would need
> to be changed later, if/when people rename things from _test.c to _kunit.c),
> as well as fixing up the sentence structure slightly:
> 
> 
> 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. This is to avoid conflicting with regular modules (e.g. if
> "foobar" is the core module, then "foobar_kunit" is the KUnit test
> module) or with the core kernel source file names (e.g. for
> tab-completion). The ``_kunit`` suffix was chosen over the older (and
> now deprecated) ``_test`` suffix, because KUnit behavior is sufficiently
> distinct that it is worth identifying at file name level.

Sure! I like that.

> > +
> > +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.
> 
> Whether you use that wording or not, this looks good, so:
> 
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>

Thanks!

-- 
Kees Cook

Re: [PATCH v2] Documentation: KUnit: Update filename best practices

[Marco Elver]

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@kernel.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.

> +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.

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

Thanks,
- Marco

Re: [PATCH v2] Documentation: KUnit: Update filename best practices

[John Hubbard]

On 7/22/24 2:55 AM, Marco Elver wrote:
> On Sat, Jul 20, 2024 at 09:54AM -0700, Kees Cook wrote:
...
> 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.
> 
> While we can apply common sense, all too often someone follows these
> rules blindly and we end up with a mess.
> 

Here, you've actually made a good argument for "blindly" following the
new naming/location conventions: it's easier to find things if a
standard naming and location convention is in place. Especially if
we document it. Now if only someone would post a patch with such
documentation... :)

I would add that the "_kunit" part of the name is especially helpful,
because (as I mentioned earlier) these tests really are different enough
that it's worth calling out. You can run them simply by loading the
kernel module.

So if I want to quickly run kunit tests, searching for "*_kunit.c" does
help with that.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH v2] Documentation: KUnit: Update filename best practices

[Marco Elver]

On Tue, 23 Jul 2024 at 01:49, John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 7/22/24 2:55 AM, Marco Elver wrote:
> > On Sat, Jul 20, 2024 at 09:54AM -0700, Kees Cook wrote:
> ...
> > 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.
> >
> > While we can apply common sense, all too often someone follows these
> > rules blindly and we end up with a mess.
> >
>
> Here, you've actually made a good argument for "blindly" following the
> new naming/location conventions: it's easier to find things if a
> standard naming and location convention is in place. Especially if
> we document it. Now if only someone would post a patch with such
> documentation... :)
>
> I would add that the "_kunit" part of the name is especially helpful,
> because (as I mentioned earlier) these tests really are different enough
> that it's worth calling out. You can run them simply by loading the
> kernel module.
>
> So if I want to quickly run kunit tests, searching for "*_kunit.c" does
> help with that.

That's fair, and I'm not too hung up about _test vs _kunit. But that's
only a tiny change of the new rules, and not the main thing I pointed
out. My main point above was about the suboptimal guidance about
where/when to introduce the "tests" subdirectory.

Thanks,
-- Marco

Re: [PATCH v2] Documentation: KUnit: Update filename best practices

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 6986 bytes --]

On Mon, 22 Jul 2024 at 17:56, Marco Elver <elver@google.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@kernel.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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]

Re: [PATCH v2] Documentation: KUnit: Update filename best practices

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 4421 bytes --]

On Sun, 21 Jul 2024 at 00:54, Kees Cook <kees@kernel.org> 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@kernel.org>
> ---
> Cc: David Gow <davidgow@google.com>
> Cc: Brendan Higgins <brendan.higgins@linux.dev>
> Cc: Rae Moar <rmoar@google.com>
> Cc: John Hubbard <jhubbard@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: linux-kselftest@vger.kernel.org
> Cc: kunit-dev@googlegroups.com
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-hardening@vger.kernel.org
> ---

Thanks again for dealing with this, Kees.

>  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.

I think John's updated version here is better. Personally, I'd rather
the bit about module names lead here, as I think that's the part most
likely to cause actual issues, and the source file name bit is more of
a "here are problems to avoid, and sensible defaults which avoid them"
than "here's an utterly inviolable rule".

Maybe:
```
KUnit tests are often compiled as a separate module. To avoid
conflicting with regular modules, KUnit modules should be named after
the test suite, followed by ``_kunit`` (e.g. if
"foobar" is the core module, then "foobar_kunit" is the KUnit test module).

Test source files, whether compiled as a separate module or an
``#include`` in another source file, are best kept in a ``tests/
subdirectory to not conflict with other source files (e.g. for
tab-completion).

Note that the ``_test`` suffix has also been used in some existing
tests. The ``_kunit`` suffix is preferred, as it makes the distinction
between KUnit and non-KUnit tests clearer.
```

(But this is all largely bikeshedding at this point. As long as we end
up describing the sensible defaults, and don't paint ourselves (and
subsystem maintainers) into a corner, either should work.

> +
> +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 think that this should be ``foo/tests/firmware_kunit.c``. I'd even
be okay with ``foo/tests/firmware.c``, as the module name needs
manually updating in the makefile anyway, where it should either be
``foo_firmware_kunit``, or included in a larger ``foo_kunit`` module.

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]