CONFIG_PM_QOS_KUNIT_TESTOn Mon, Jun 15, 2020 at 1:48 AM Kees Cook keescook@chromium.org wrote:
On Sat, Jun 13, 2020 at 02:51:17PM +0800, David Gow wrote:
Yeah, _KUNIT_TEST was what we've sort-of implicitly decided on for config names, but the documentation does need to happen.
That works for me. It still feels redundant, but all I really want is a standard name. :)
We haven't put as much thought into standardising the filenames much, though.
I actually find this to be much more important because it is more end-user-facing (i.e. in module naming, in build logs, in scripts, on filesystem, etc -- CONFIG is basically only present during kernel build). Trying to do any sorting or greping really needs a way to find all the kunit pieces.
Certainly this is more of an issue now we support building KUnit tests as modules, rather than having them always be built-in.
Having some halfway consistent config-name <-> filename <-> test suite name could be useful down the line, too. Unfortunately, not necessarily a 1:1 mapping, e.g.: - CONFIG_KUNIT_TEST compiles both kunit-test.c and string-stream-test.c - kunit-test.c has several test suites within it: kunit-try-catch-test, kunit-resource-test & kunit-log-test. - CONFIG_EXT4_KUNIT_TESTS currently only builds ext4-inode-test.c, but as the plural name suggests, might build others later. - CONFIG_SECURITY_APPARMOR_KUNIT_TEST doesn't actually have its own source file: the test is built into policy_unpack.c - &cetera
Indeed, this made me quickly look up the names of suites, and there are a few inconsistencies there: - most have "-test" as a suffix - some have "_test" as a suffix - some have no suffix
(I'm inclined to say that these don't need a suffix at all.)
Within test suites, we're also largely prefixing all of the tests with a suite name (even if it's not actually the specified suite name). For example, CONFIG_PM_QOS_KUNIT_TEST builds drivers/base/power/qos-test.c which contains a suite called "qos-kunit-test", with tests prefixed "freq_qos_test_". Some of this clearly comes down to wanting to namespace things a bit more ("qos-test" as a name could refer to a few things, I imagine), but specifying how to do so consistently could help.
Both of these are slightly complicated by cases like this where tests are being ported from a non-KUnit test to KUnit. There's a small argument there for trying to keep the name the same, though personally I suspect consistency is more important.
Understood. I think consistency is preferred too, especially since the driving reason to make this conversions is to gain consistency with the actual tests themselves.
We'll go with that until someone objects: from what I recall, this is largely what people have been doing anyway.
Alas, the plans to document test coding style / conventions kept getting pre-empted: I'll drag it back up to the top of the to-do list, and see if we can't prioritise it. I think we'd hoped to be able to catch these in review, but between a bit of forgetfulness and a few tests going upstream without our seeing them has made it obvious that doesn't work.
Once something's documented (and the suitable bikeshedding has subsided), we can consider renaming existing tests if that seems worthwhile.
Yes please! :)
I'll see if I can find time to draft something this week, then. No promises, but hopefully there'll at least be something to build on.
My feeling is we'll go for:
- Kconfig name: ~_KUNIT_TEST
As mentioned, I'm fine with this, but prefer ~_KUNIT
- filename: ~-test.c
I really don't like this. Several reasons reasons:
it does not distinguish it from other tests -- there is no way to identify kunit tests from non-kunit tests from directory listings, build log greps, etc.
the current "common" naming has been with a leading "test", ignoring kunit, tools/, and samples/:
53 test_*.c 27 *_test.c 19 *[a-z0-9]test.c 19 selftest*.c 16 test-*.c 11 *-test.c 11 test[a-z0-9]*.c 8 *-tests.c 5 *-selftest.c 4 *_test_*.c 1 *_selftest_*.c 1 *_selftests.c(these counts might be a bit off -- my eyes started to cross while constructing regexes)
- dashes are converted to _ in module names, leading to some confusion between .c file and .ko file.
I'd strongly prefer ~_kunit.c, but could live with _kunit_test.c (even though it's redundant).
I suggested -test.c largely because it's seemed to be most popular out of existing KUnit tests (and certainly out of the ones that already had-KUNIT_TEST config suffixes), but I definitely see your points. I think that trying to stick to a "common" test naming is a bit contradictory with trying to distinguish KUnit tests from other tests, and I'm leaning to the side of distinguishing them, so I definitely could be converted to ~_kunit.c.
Brendan, any thoughts?
At least for the initial draft documentation, as those seem to be most common, but I think a thread on that would probably be the best place to add it.
I'm ready! :) (Subject updated)
This would also be a good opportunity to document the "standard" KUnit boilerplate help text in the Kconfig options.
Ah yeah, good idea.
-- Kees Cook
Cheers, -- David