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=... [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,

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

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