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