On Mon, 22 Jul 2024 at 17:56, Marco Elver <elver@xxxxxxxxxx> 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@xxxxxxxxxxxxxx/ [1] > > Signed-off-by: Kees Cook <kees@xxxxxxxxxx> > [...] > > 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:
smime.p7s
Description: S/MIME Cryptographic Signature
