On Thu, 18 Jul 2024 at 00:49, Kees Cook kees@kernel.org wrote:
On Wed, Jul 17, 2024 at 02:28:15PM +0800, David Gow wrote:
On Wed, 17 Jul 2024 at 11:53, Kees Cook kees@kernel.org wrote:
On Tue, Jul 16, 2024 at 01:10:41PM -0700, Linus Torvalds wrote:
On Mon, 15 Jul 2024 at 09:21, Kees Cook kees@kernel.org wrote:
fs/exec.c | 49 ++++++++-- fs/exec_test.c | 141 ++++++++++++++++++++++++++++
I've pulled this, but *PLEASE* don't do this.
This screws up my workflow of just using tab-completion for filenames. As a result, I absolutely abhor anybody who uses the same base-name for different things.
No, this is not the first time it happens, and it won't be the last. And we had that same horrific pattern for fs/binfmt_elf_test.c from before, and I didn't notice because it's not a core file to me, and I seldom actually edit it.
I would suggest that people use the patterns from lib/, which is admittedly a bit schizophrenic in that you can either use "lib/kunit/*.c" (probably preferred) or "lib/test_xyz.c".
(Other subsystems use a "tests" subdirectory, so we do have a lot of different ways to deal with this).
Any of those models will keep the unit testing parts clearly separate, and not mess up basic command line workflows.
But do *not* use this "*_test.c" naming model. It's the worst of all possible worlds.
Please?
Oh, sure, no problem! I have no attachment to this convention at all; I was trying to follow the Kunit docs: https://docs.kernel.org/dev-tools/kunit/style.html#test-file-and-module-name...
If I look at the existing naming, it's pretty scattered:
$ git grep '^static struct kunit_suite\b' | cut -d: -f1 | sort -u
/test/* 7 /tests/* 47 *-test.[ch] 27 *_test.[ch] 27 test-*.c 1 test_*.c 10 *-kunit.c 1 *_kunit.c 17 kunit-*.c 2 kunit_*.c 1
Should we go with "put it all under a 'tests' subdirectory" ?
I think that's probably best overall. I still think it isn't quite as elegant as the suffix, but I'm happy to sacrifice theoretical elegance for a practical reason like this.
Okay, I will send a follow-up patch to rename things.
So for fs/exec_test.c and fs/binfmt_elf_test.c, perhaps fs/tests/exec.c and fs/tests/binfmt_elf.c respectively?
We might want to use both the directory and the suffix, e.g. fs/tests/exec_test.c, because:
- it makes sure the module name contains 'test', so it's obvious that
it's a test and it is less likely to conflict.
- this matches what drm is doing, and they've got the most tests thus far; and
- we won't be renaming the file, just moving it, so it's less likely
to cause friction with workflows, etc.
On the other hand, it has few disadvantages:
- we end up with the same prefix issue with module names (e.g., for
those who have tab completion for modprobe);
- the module name can be changed in the Makefile anyway; and
- it's ugly.
I'm leaning towards tolerating the ugliness and keeping _test suffixes on the files, at least for existing tests, but could be persuaded otherwise. I'd even grow to accept a test_ prefix if I had to, though that'd make my tab completion terribly boring.
I'd been using _test for #included files, and _kunit for kunit modules, but perhaps this isn't a needed distinction?
I went back and checked the original discussion on this, and there were a few proposed uses for the distinction: - _test should be used by default, and _kunit should be used if there's already a non-KUnit _test - _kunit should be used for unit tests (i.e., relatively self-contained, execute quickly), _test should be used for any other form of test, even if it is implemented on top of KUnit - _kunit should be used for _new_ KUnit tests, old tests should not be renamed if it causes friction
In the end, we had the most support for the first option, but I don't think there's a problem reconsidering it. We do have things like test attributes now, which can allow tooling to filter out long-running tests (so using the name for this isn't as important as it once was).
Ultimately, I don't think it really matters much for source files: _test is already used a lot for both KUnit and non-KUnit tests, so it's difficult to get any detailed meaning from it. And while it'd be nice to know for a fact that all KUnit tests were in modules with _kunit in their names, there are enough exceptions that we'll never have this work perfectly. As long as people can find the tests (and, if they're in a tests/ directory, this shouldn't be difficult, regardless of the filenames), I don't think it matters. For the module name, it does a bit more, and #included files don't influence the module name anyway, so we might as well go with _kunit for everything.
And for the lib/*_kunit.c files, use lib/tests/*.c ?
Sounds good to me. I'd rather not put them in lib/kunit unless they're actively testing KUnit itself (which, under this scheme, would want to be in lib/kunit/tests).
Right -- I didn't want to confuse things between kunit itself and kunit tests, so I too prefer the "tests" directory name.
Then we can update the docs, etc.
Even if we don't rename existing tests, we'll probably want to update these just to avoid making the problem worse.
Sounds good.
Excellent. Let's update the docs (I think we'll go with _kunit as the suffix as discussed in the other thread, now we have the tests/ directory), and start renaming things if there's no objection to the docs change.
-- David