Re: common KUnit Kconfig and file naming (was: Re: [PATCH] lib: kunit_test_overflow: add KUnit test of check_*_overflow functions)
On Thu, Jun 18, 2020 at 11:39 PM David Gow wrote: [...] > On Fri, Jun 19, 2020 at 4:28 AM Brendan Higgins > wrote: > > > > On Tue, Jun 16, 2020 at 9:21 PM David Gow wrote: > > > > > > On Tue, Jun 16, 2020 at 5:40 PM Alan Maguire > > > wrote: > > > > > > > > On Tue, 16 Jun 2020, David Gow wrote: [...] > > > > > wrote: > > > > > > > > > > > > On Sat, Jun 13, 2020 at 02:51:17PM +0800, David Gow wrote: [...] > > > - Test names: Personally, I'd kind-of like to not prefix these at all, > > > as they're already part of the suite. If we do want to, though, prefix > > > them with and . > > > > Eh, I did that to remain consistent with the kernel naming > > conventions, but I think those have diverged too. If maintainers are > > cool with it, I agree that the prefixes are redundant on tests and > > generally way too long. > > > > Do you have a link to the conventions you're talking about? A link no. This is only of those undocumented rules that most people follow. The rule is something like this: Global identifiers should be named: __...___foo. For example, let's say I am working on Synopsis' DesignWare I2C master driver. The outermost namespace is i2c, and because DesignWare is long, we might prefix each function with i2c_dw_*. It is a practice that is not universally maintained around the kernel, but it seems to be the most common method of namespacing aside from just randomly throwing characters together in a prefix that hasn't been used before. Anyway, standardized or not, that is the convention I was trying to follow.
Re: common KUnit Kconfig and file naming (was: Re: [PATCH] lib: kunit_test_overflow: add KUnit test of check_*_overflow functions)
I'm in the process of writing up some documentation for this now. I hope to post a draft soon, but here's the overview of what's going in it: - Test filenames should be _kunit.c - (If the suite name is too long/namespaced, the source filename may have prefixes removed, so long as the module name doesn't) - Config names should end in _KUNIT_TEST, and follow the other existing conventions (re: help text, default values, etc) - Suite names should be fully-qualified/unambiguous across the whole kernel (i.e., include driver/subsystem names) - Possibly look at the path to the code being tested as inspiration for the suite name. - Use underscores as separators, and don't decorate test/suite names with "test" or "kunit". Still thinking about: - Whether to formalise a "subsystem", or just have it implicitly part of the suite name. - Whether to talk about having multiple suites in one file and/or Kconfig entry. On Fri, Jun 19, 2020 at 4:28 AM Brendan Higgins wrote: > > On Tue, Jun 16, 2020 at 9:21 PM David Gow wrote: > > > > On Tue, Jun 16, 2020 at 5:40 PM Alan Maguire > > wrote: > > > > > > On Tue, 16 Jun 2020, David Gow wrote: > > > > > > > CONFIG_PM_QOS_KUNIT_TESTOn Mon, Jun 15, 2020 at 1:48 AM Kees Cook > > > > 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.) > > > > > > > > > > A good convention for module names - which I _think_ is along the lines > > > of what Kees is suggesting - might be something like > > > > > > [_]_kunit.ko > > > > > > So for example > > > > > > kunit_test -> test_kunit.ko > > > string_stream_test.ko -> test_string_stream_kunit.ko > > > kunit_example_test -> example_kunit.ko > > > ext4_inode_test.ko -> ext4_inode_kunit.ko > > > > > > For the kunit selftests, "selftest_" might be a better name > > > than "test_", as the latter might encourage people to reintroduce > > > a redundant "test" into their module name. > > > > > > > I quite like the idea of having the deeper "subsystem:suite:test" > > hierarchy here. "selftest" possibly would be a bit confusing against > > kselftest for the KUnit tests -- personally I'd probably go with > > "kunit", even if that introduces a redundant-looking kunit into the > > module name. > > Ditto. My first reaction was that it would create too much nesting and > subsystems are a poorly defined notion in the Linux kernel; however, > after thinking about it some, I think we are already doing what you > are proposing with namespacing in identifier names. It makes sense to > reflect that in test organization and reporting. > The real trick for documenting this is, as you say, defining subsystem. I suspect we'll be okay leaving this up to common sense, and perhaps suggesting the MAINTAINERS entry or file path as places to pull subsystem names from. Having the subsystem be the name of the module being tested for drivers, for example, makes some sense, too. > > So, this could look something like: > > - Kconfig name: CONFIG___KUNIT_TEST, or very > > possibly CONFIG__KUNIT_TEST(S?) -- we already have > > something like that
Re: common KUnit Kconfig and file naming (was: Re: [PATCH] lib: kunit_test_overflow: add KUnit test of check_*_overflow functions)
On Thu, Jun 18, 2020 at 01:27:55PM -0700, Brendan Higgins wrote: > I am cool with changing *-test.c to *-kunit.c. The *-test.c was a hold I am fine with basically any decision as long as there's a single naming convention, *except* for this part. Dashes in source files creates confusion for module naming. Separators should be underscores. This is a standing pet-peeve of mine, and while I certainly can't fix it universally in the kernel, we can at least avoid creating an entire subsystem that gets this wrong for all modules. :) To illustrate: $ modinfo dvb-bt8xx filename: .../kernel/drivers/media/pci/bt8xx/dvb-bt8xx.ko ... name: dvb_bt8xx ^ does not match the .ko file, nor source. Primarily my issue is the disconnect between "dmesg" output and finding the source. It's not like, a huge deal, but it bugs me. :) As in: $ strings drivers/media/pci/bt8xx/dvb-bt8xx.o | grep 'Init Error' 4dvb_bt8xx: or51211: Init Error - Can't Reset DVR (%i) All this said, if there really is some good reason to use dashes, I will get over it. :P (And now that I've had to say all this "out loud", I wonder if maybe I could actually fix this all at the root cause: KBUILD_MOD_NAME... it is sometimes used for identifiers, which is why it does the underscore replacement... I wonder if it could be split into "name" and "identifier"...) -- Kees Cook
Re: common KUnit Kconfig and file naming (was: Re: [PATCH] lib: kunit_test_overflow: add KUnit test of check_*_overflow functions)
On Tue, Jun 16, 2020 at 9:21 PM David Gow wrote: > > On Tue, Jun 16, 2020 at 5:40 PM Alan Maguire wrote: > > > > On Tue, 16 Jun 2020, David Gow wrote: > > > > > CONFIG_PM_QOS_KUNIT_TESTOn Mon, Jun 15, 2020 at 1:48 AM Kees Cook > > > 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.) > > > > > > > A good convention for module names - which I _think_ is along the lines > > of what Kees is suggesting - might be something like > > > > [_]_kunit.ko > > > > So for example > > > > kunit_test -> test_kunit.ko > > string_stream_test.ko -> test_string_stream_kunit.ko > > kunit_example_test -> example_kunit.ko > > ext4_inode_test.ko -> ext4_inode_kunit.ko > > > > For the kunit selftests, "selftest_" might be a better name > > than "test_", as the latter might encourage people to reintroduce > > a redundant "test" into their module name. > > > > I quite like the idea of having the deeper "subsystem:suite:test" > hierarchy here. "selftest" possibly would be a bit confusing against > kselftest for the KUnit tests -- personally I'd probably go with > "kunit", even if that introduces a redundant-looking kunit into the > module name. Ditto. My first reaction was that it would create too much nesting and subsystems are a poorly defined notion in the Linux kernel; however, after thinking about it some, I think we are already doing what you are proposing with namespacing in identifier names. It makes sense to reflect that in test organization and reporting. > So, this could look something like: > - Kconfig name: CONFIG___KUNIT_TEST, or very > possibly CONFIG__KUNIT_TEST(S?) -- we already have > something like that for the ext4 tests. I think the biggest question there is whether we go with the every suite gets its own config approach or all suites in a subsystem are turned on by a single config. I don't think there are enough examples to determine what the community would prefer, and I can see advantages and disadvantages to both. > - Source filename: _kunit.c within a subsystem directory. (We > probably don't need to enforce suites being in separate files, but > whatever file does contain the tests should at least end "kunit.c") I am cool with changing *-test.c to *-kunit.c. The *-test.c was a hold over from when everything was prefixed TEST_* instead of KUNIT_* (back in the early days of the RFC). I never liked naming everything KUNIT_* or -kunit- whatever; it felt kind of egotistical to me (there was also always a part of me that hoped I would come up with a better name than KUnit, but that ship sailed a long time ago). However, purely logically speaking, I think naming all KUnit tests *-kunit.c makes more sense than anything else. One question: the AppArmor KUnit tests are #included into another .c file when the tests are selected. Should tests #included in this manner be -kunit.h or should all tests be -kunit.c? > - Module filename: __kunit.ko, or > _kunit.ko if all suites are built into the same module (or > there's just one suite for the whole subsystem) > - Suite name: Either _ or have a separate subsystem > field in kunit_suite. If there's only one suite for the subsystem,
Re: common KUnit Kconfig and file naming (was: Re: [PATCH] lib: kunit_test_overflow: add KUnit test of check_*_overflow functions)
On Tue, Jun 16, 2020 at 5:40 PM Alan Maguire wrote:
>
> On Tue, 16 Jun 2020, David Gow wrote:
>
> > CONFIG_PM_QOS_KUNIT_TESTOn Mon, Jun 15, 2020 at 1:48 AM Kees Cook
> > 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.)
> >
>
> A good convention for module names - which I _think_ is along the lines
> of what Kees is suggesting - might be something like
>
> [_]_kunit.ko
>
> So for example
>
> kunit_test -> test_kunit.ko
> string_stream_test.ko -> test_string_stream_kunit.ko
> kunit_example_test -> example_kunit.ko
> ext4_inode_test.ko -> ext4_inode_kunit.ko
>
> For the kunit selftests, "selftest_" might be a better name
> than "test_", as the latter might encourage people to reintroduce
> a redundant "test" into their module name.
>
I quite like the idea of having the deeper "subsystem:suite:test"
hierarchy here. "selftest" possibly would be a bit confusing against
kselftest for the KUnit tests -- personally I'd probably go with
"kunit", even if that introduces a redundant-looking kunit into the
module name.
So, this could look something like:
- Kconfig name: CONFIG___KUNIT_TEST, or very
possibly CONFIG__KUNIT_TEST(S?) -- we already have
something like that for the ext4 tests.
- Source filename: _kunit.c within a subsystem directory. (We
probably don't need to enforce suites being in separate files, but
whatever file does contain the tests should at least end "kunit.c")
- Module filename: __kunit.ko, or
_kunit.ko if all suites are built into the same module (or
there's just one suite for the whole subsystem)
- Suite name: Either _ or have a separate subsystem
field in kunit_suite. If there's only one suite for the subsystem, set
suite==subsystem
- Test names: Personally, I'd kind-of like to not prefix these at all,
as they're already part of the suite. If we do want to, though, prefix
them with and .
> > 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.
> >
>
> Could we add some definitions to help standardize this?
> For example, adding a "subsystem" field to "struct kunit_suite"?
>
> So for the ext4 tests the "subsystem" would be "ext4" and the
> name "inode" would specify the test area within that subsystem.
> For the KUnit selftests, the subsystem would be "test"/"selftest".
> Logging could utilize the subsystem definition to allow test
> writers to use less redundant test names too. For example
> the suite name logged could be constructed from the
> subsystem + area values associated with the kunit_suite,
> and individual test names could be shown as the
> suite area + test_name.
As above, I quite like this. If we were really brave, we could
actually nest the results into subsystem:area/suite:test using the TAP
subtests stuff. Generating the longer suite name may be easier on
people manua
Re: common KUnit Kconfig and file naming (was: Re: [PATCH] lib: kunit_test_overflow: add KUnit test of check_*_overflow functions)
On Tue, 16 Jun 2020, David Gow wrote:
> CONFIG_PM_QOS_KUNIT_TESTOn Mon, Jun 15, 2020 at 1:48 AM Kees Cook
> 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.)
>
A good convention for module names - which I _think_ is along the lines
of what Kees is suggesting - might be something like
[_]_kunit.ko
So for example
kunit_test -> test_kunit.ko
string_stream_test.ko -> test_string_stream_kunit.ko
kunit_example_test -> example_kunit.ko
ext4_inode_test.ko -> ext4_inode_kunit.ko
For the kunit selftests, "selftest_" might be a better name
than "test_", as the latter might encourage people to reintroduce
a redundant "test" into their module name.
> 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.
>
Could we add some definitions to help standardize this?
For example, adding a "subsystem" field to "struct kunit_suite"?
So for the ext4 tests the "subsystem" would be "ext4" and the
name "inode" would specify the test area within that subsystem.
For the KUnit selftests, the subsystem would be "test"/"selftest".
Logging could utilize the subsystem definition to allow test
writers to use less redundant test names too. For example
the suite name logged could be constructed from the
subsystem + area values associated with the kunit_suite,
and individual test names could be shown as the
suite area + test_name.
Thanks!
Alan
Re: common KUnit Kconfig and file naming (was: Re: [PATCH] lib: kunit_test_overflow: add KUnit test of check_*_overflow functions)
CONFIG_PM_QOS_KUNIT_TESTOn Mon, Jun 15, 2020 at 1:48 AM Kees Cook
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 convert
common KUnit Kconfig and file naming (was: Re: [PATCH] lib: kunit_test_overflow: add KUnit test of check_*_overflow functions)
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. > 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. > 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! :) > 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). > 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
