As discussed in [1], KUnit tests have hitherto not had a particularly consistent naming scheme. This adds documentation outlining how tests and test suites should be named, including how those names should be used in Kconfig entries and filenames.
[1]: https://lore.kernel.org/linux-kselftest/202006141005.BA19A9D3@keescook/t/#u
Signed-off-by: David Gow davidgow@google.com Reviewed-by: Kees Cook keescook@chromium.org --- This is a follow-up v1 to the RFC patch here: https://lore.kernel.org/linux-kselftest/20200620054944.167330-1-davidgow@goo...
There weren't any fundamental objections to the naming guidelines themselves, so nothing's changed on that front.
Otherwise, changes since the RFC: - Fixed a bit of space/tab confusion in the index (Thanks, Randy) - Added some more examples (and some test case examples). - Added some examples of what not to call subsystems and suites. - No longer explicitly require "If unsure, put N" in Kconfig entries. - Minor formatting changes.
Cheers, -- David
Documentation/dev-tools/kunit/index.rst | 1 + Documentation/dev-tools/kunit/style.rst | 181 ++++++++++++++++++++++++ 2 files changed, 182 insertions(+) create mode 100644 Documentation/dev-tools/kunit/style.rst
diff --git a/Documentation/dev-tools/kunit/index.rst b/Documentation/dev-tools/kunit/index.rst index e93606ecfb01..c234a3ab3c34 100644 --- a/Documentation/dev-tools/kunit/index.rst +++ b/Documentation/dev-tools/kunit/index.rst @@ -11,6 +11,7 @@ KUnit - Unit Testing for the Linux Kernel usage kunit-tool api/index + style faq
What is KUnit? diff --git a/Documentation/dev-tools/kunit/style.rst b/Documentation/dev-tools/kunit/style.rst new file mode 100644 index 000000000000..8cad2627924c --- /dev/null +++ b/Documentation/dev-tools/kunit/style.rst @@ -0,0 +1,181 @@ +.. SPDX-License-Identifier: GPL-2.0 + +=========================== +Test Style and Nomenclature +=========================== + +Subsystems, Suites, and Tests +============================= + +In order to make tests as easy to find as possible, they're grouped into suites +and subsystems. A test suite is a group of tests which test a related area of +the kernel, and a subsystem is a set of test suites which test different parts +of the same kernel subsystem or driver. + +Subsystems +---------- + +Every test suite must belong to a subsystem. A subsystem is a collection of one +or more KUnit test suites which test the same driver or part of the kernel. A +rule of thumb is that a test subsystem should match a single kernel module. If +the code being tested can't be compiled as a module, in many cases the subsystem +should correspond to a directory in the source tree or an entry in the +MAINTAINERS file. If unsure, follow the conventions set by tests in similar +areas. + +Test subsystems should be named after the code being tested, either after the +module (wherever possible), or after the directory or files being tested. Test +subsystems should be named to avoid ambiguity where necessary. + +If a test subsystem name has multiple components, they should be separated by +underscores. *Do not* include "test" or "kunit" directly in the subsystem name +unless you are actually testing other tests or the kunit framework itself. + +Example subsystems could be: + +``ext4`` + Matches the module and filesystem name. +``apparmor`` + Matches the module name and LSM name. +``kasan`` + Common name for the tool, prominent part of the path ``mm/kasan`` +``snd_hda_codec_hdmi`` + Has several components (``snd``, ``hda``, ``codec``, ``hdmi``) separated by + underscores. Matches the module name. + +Avoid names like these: + +``linear-ranges`` + Names should use underscores, not dashes, to separate words. Prefer + ``linear_ranges``. +``qos-kunit-test`` + As well as using underscores, this name should not have "kunit-test" as a + suffix, and ``qos`` is ambiguous as a subsystem name. ``power_qos`` would be a + better name. +``pc_parallel_port`` + The corresponding module name is ``parport_pc``, so this subsystem should also + be named ``parport_pc``. + +.. note:: + The KUnit API and tools do not explicitly know about subsystems. They're + simply a way of categorising test suites and naming modules which + provides a simple, consistent way for humans to find and run tests. This + may change in the future, though. + +Suites +------ + +KUnit tests are grouped into test suites, which cover a specific area of +functionality being tested. Test suites can have shared initialisation and +shutdown code which is run for all tests in the suite. +Not all subsystems will need to be split into multiple test suites (e.g. simple drivers). + +Test suites are named after the subsystem they are part of. If a subsystem +contains several suites, the specific area under test should be appended to the +subsystem name, separated by an underscore. + +The full test suite name (including the subsystem name) should be specified as +the ``.name`` member of the ``kunit_suite`` struct, and forms the base for the +module name (see below). + +Example test suites could include: + +``ext4_inode`` + Part of the ``ext4`` subsystem, testing the ``inode`` area. +``kunit_try_catch`` + Part of the ``kunit`` implementation itself, testing the ``try_catch`` area. +``apparmor_property_entry`` + Part of the ``apparmor`` subsystem, testing the ``property_entry`` area. +``kasan`` + The ``kasan`` subsystem has only one suite, so the suite name is the same as + the subsystem name. + +Avoid names like: + +``ext4_ext4_inode`` + There's no reason to state the subsystem twice. +``property_entry`` + The suite name is ambiguous without the subsystem name. +``kasan_unit_test`` + Because there is only one suite in the ``kasan`` subsystem, the suite should + just be called ``kasan``. There's no need to redundantly add ``unit_test``. + +Test Cases +---------- + +Individual tests consist of a single function which tests a constrained +codepath, property, or function. In the test output, individual tests' results +will show up as subtests of the suite's results. + +Tests should be named after what they're testing. This is often the name of the +function being tested, with a description of the input or codepath being tested. +As tests are C functions, they should be named and written in accordance with +the kernel coding style. + +.. note:: + As tests are themselves functions, their names cannot conflict with + other C identifiers in the kernel. This may require some creative + naming. It's a good idea to make your test functions `static` to avoid + polluting the global namespace. + +Example test names include: + +``unpack_u32_with_null_name`` + Tests the ``unpack_u32`` function when a NULL name is passed in. +``test_list_splice`` + Tests the ``list_splice`` macro. It has the prefix ``test_`` to avoid a + name conflict with the macro itself. + + +Should it be necessary to refer to a test outside the context of its test suite, +the *fully-qualified* name of a test should be the suite name followed by the +test name, separated by a colon (i.e. ``suite:test``). + +Test Kconfig Entries +==================== + +Every test suite should be tied to a Kconfig entry. + +This Kconfig entry must: + +* be named ``CONFIG_<name>_KUNIT_TEST``: where <name> is the name of the test + suite. +* be listed either alongside the config entries for the driver/subsystem being + tested, or be under [Kernel Hacking]→[Kernel Testing and Coverage] +* depend on ``CONFIG_KUNIT`` +* be visible only if ``CONFIG_KUNIT_ALL_TESTS`` is not enabled. +* have a default value of ``CONFIG_KUNIT_ALL_TESTS``. +* have a brief description of KUnit in the help text + +Unless there's a specific reason not to (e.g. the test is unable to be built as +a module), Kconfig entries for tests should be tristate. + +An example Kconfig entry: + +.. code-block:: none + + config FOO_KUNIT_TEST + tristate "KUnit test for foo" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + help + This builds unit tests for foo. + + For more information on KUnit and unit tests in general, please refer + to the KUnit documentation in Documentation/dev-tools/kunit + + If unsure, say N + + +Test Filenames +============== + +Where possible, test suites should be placed in a separate source file in the +same directory as the code being tested. + +This file should be named ``<suite>_kunit.c``. It may make sense to strip +excessive namespacing from the source filename (e.g., ``firmware_kunit.c`` instead of +``<drivername>_firmware.c``), but please ensure the module name does contain the +full suite name. + +
On Thu, Jul 2, 2020 at 12:14 AM David Gow davidgow@google.com wrote:
As discussed in [1], KUnit tests have hitherto not had a particularly consistent naming scheme. This adds documentation outlining how tests and test suites should be named, including how those names should be used in Kconfig entries and filenames.
Signed-off-by: David Gow davidgow@google.com Reviewed-by: Kees Cook keescook@chromium.org
Sorry for taking so long on this; nevertheless, looks great!
Reviewed-by: Brendan Higgins brendanhiggins@google.com
On Thu, Jul 02, 2020 at 12:14AM -0700, David Gow wrote:
As discussed in [1], KUnit tests have hitherto not had a particularly consistent naming scheme. This adds documentation outlining how tests and test suites should be named, including how those names should be used in Kconfig entries and filenames.
Signed-off-by: David Gow davidgow@google.com Reviewed-by: Kees Cook keescook@chromium.org
...
+An example Kconfig entry:
+.. code-block:: none
config FOO_KUNIT_TESTtristate "KUnit test for foo" if !KUNIT_ALL_TESTSdepends on KUNITdefault KUNIT_ALL_TESTShelpThis builds unit tests for foo.For more information on KUnit and unit tests in general, please referto the KUnit documentation in Documentation/dev-tools/kunitIf unsure, say N+Test Filenames +==============
+Where possible, test suites should be placed in a separate source file in the +same directory as the code being tested.
+This file should be named ``<suite>_kunit.c``. It may make sense to strip +excessive namespacing from the source filename (e.g., ``firmware_kunit.c`` instead of +``<drivername>_firmware.c``), but please ensure the module name does contain the +full suite name.
First of all, thanks for the talk yesterday! I only looked at this because somebody pasted the LKML link. :-)
The example about excessive namespacing seems confusing. Was it supposed to be
[...] firmware_kunit.c`` instead of ``<drivername>_firmware_kunit.c [...]
?
While I guess this ship has sailed, and *_kunit.c is the naming convention now, I hope this is still just a recommendation and names of the form *-test.c are not banned!
$> git grep 'KUNIT.*-test.o' drivers/base/power/Makefile:obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o drivers/base/test/Makefile:obj-$(CONFIG_KUNIT_DRIVER_PE_TEST) += property-entry-test.o fs/ext4/Makefile:obj-$(CONFIG_EXT4_KUNIT_TESTS) += ext4-inode-test.o kernel/Makefile:obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o lib/Makefile:obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_TEST) += kunit-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_TEST) += string-stream-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_EXAMPLE_TEST) += kunit-example-test.o
$> git grep 'KUNIT.*_kunit.o' # Returns nothing
Just an idea: Maybe the names are also an opportunity to distinguish real _unit_ style tests and then the rarer integration-style tests. I personally prefer using the more generic *-test.c, at least for the integration-style tests I've been working on (KUnit is still incredibly valuable for integration-style tests, because otherwise I'd have to roll my own poor-man's version of KUnit, so thank you!). Using *_kunit.c for such tests is unintuitive, because the word "unit" hints at "unit tests" -- and having descriptive (and not misleading) filenames is still important. So I hope you won't mind if *-test.c are still used where appropriate.
Thanks, -- Marco
On Thu, Aug 27, 2020 at 9:14 PM Marco Elver elver@google.com wrote:
On Thu, Jul 02, 2020 at 12:14AM -0700, David Gow wrote:
As discussed in [1], KUnit tests have hitherto not had a particularly consistent naming scheme. This adds documentation outlining how tests and test suites should be named, including how those names should be used in Kconfig entries and filenames.
Signed-off-by: David Gow davidgow@google.com Reviewed-by: Kees Cook keescook@chromium.org
...
+An example Kconfig entry:
+.. code-block:: none
config FOO_KUNIT_TESTtristate "KUnit test for foo" if !KUNIT_ALL_TESTSdepends on KUNITdefault KUNIT_ALL_TESTShelpThis builds unit tests for foo.For more information on KUnit and unit tests in general, please referto the KUnit documentation in Documentation/dev-tools/kunitIf unsure, say N+Test Filenames +==============
+Where possible, test suites should be placed in a separate source file in the +same directory as the code being tested.
+This file should be named ``<suite>_kunit.c``. It may make sense to strip +excessive namespacing from the source filename (e.g., ``firmware_kunit.c`` instead of +``<drivername>_firmware.c``), but please ensure the module name does contain the +full suite name.
First of all, thanks for the talk yesterday! I only looked at this because somebody pasted the LKML link. :-)
No worries! Clearly this document needed linking -- even I was starting to suspect the reason no-one was complaining about this was that no-one had read it. :-)
The example about excessive namespacing seems confusing. Was it supposed to be
[...] firmware_kunit.c`` instead of ``<drivername>_firmware_kunit.c [...]?
Whoops, yes. I'll fix that.
While I guess this ship has sailed, and *_kunit.c is the naming convention now, I hope this is still just a recommendation and names of the form *-test.c are not banned!
The ship hasn't technically sailed until this patch is actually accepted. Thus far, we hadn't had any real complaints about the _kunit.c idea, though that may have been due to this email not reaching enough people. If you haven't read the discussion in https://lore.kernel.org/linux-kselftest/202006141005.BA19A9D3@keescook/t/#u it's worthwhile: the _kunit.c name is discussed, and Kees lays out some more detailed rationale for it as well.
$> git grep 'KUNIT.*-test.o' drivers/base/power/Makefile:obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o drivers/base/test/Makefile:obj-$(CONFIG_KUNIT_DRIVER_PE_TEST) += property-entry-test.o fs/ext4/Makefile:obj-$(CONFIG_EXT4_KUNIT_TESTS) += ext4-inode-test.o kernel/Makefile:obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o lib/Makefile:obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_TEST) += kunit-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_TEST) += string-stream-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_EXAMPLE_TEST) += kunit-example-test.o
$> git grep 'KUNIT.*_kunit.o' # Returns nothing
This was definitely something we noted. Part of the goal of having _kunit.c as a filename suffix (and, perhaps more importantly, the _kunit module name suffix) was to have a way of both distinguishing KUnit tests from non-KUnit ones (of which there are quite a few already with -test names), and to have a way of quickly determining what modules contain KUnit tests. That really only works if everyone is using it, so the plan was to push this as much as possible. This'd probably include renaming most of the existing test files to match, which is a bit of a pain (particularly when converting non-KUnit tests to KUnit and similar), but is a one-time thing.
Just an idea: Maybe the names are also an opportunity to distinguish real _unit_ style tests and then the rarer integration-style tests. I personally prefer using the more generic *-test.c, at least for the integration-style tests I've been working on (KUnit is still incredibly valuable for integration-style tests, because otherwise I'd have to roll my own poor-man's version of KUnit, so thank you!). Using *_kunit.c for such tests is unintuitive, because the word "unit" hints at "unit tests" -- and having descriptive (and not misleading) filenames is still important. So I hope you won't mind if *-test.c are still used where appropriate.
As Brendan alluded to in the talk, the popularity of KUnit for these integration-style tests came as something of a surprise (more due to our lack of imagination than anything else, I suspect). It's great that it's working, though: I don't think anyone wants the world filled with more single-use test "frameworks" than is necessary!
I guess the interesting thing to note is that we've to date not really made a distinction between KUnit the framework and the suite of all KUnit tests. Maybe having a separate file/module naming scheme could be a way of making that distinction, though it'd really only appear when loading tests as modules -- there'd be no indication in e.g., suite names or test results. The more obvious solution to me (at least, based on the current proposal) would be to have "integration" or similar be part of the suite name (and hence the filename, so _integration_kunit.c or similar), though even I admit that that's much uglier. Maybe the idea of having the subsystem/suite distinction be represented in the code could pave the way to having different suites support different suffixes like that.
Do you know of any cases where something has/would have both unit-style tests and integration-style tests built with KUnit where the distinction needs to be clear?
Brendan, Kees: do you have any thoughts?
Thanks, -- Marco
Cheers, -- David
On Thu, 27 Aug 2020 at 18:17, David Gow davidgow@google.com wrote: [...]
First of all, thanks for the talk yesterday! I only looked at this because somebody pasted the LKML link. :-)
No worries! Clearly this document needed linking -- even I was starting to suspect the reason no-one was complaining about this was that no-one had read it. :-)
[...]
While I guess this ship has sailed, and *_kunit.c is the naming convention now, I hope this is still just a recommendation and names of the form *-test.c are not banned!
The ship hasn't technically sailed until this patch is actually accepted. Thus far, we hadn't had any real complaints about the _kunit.c idea, though that may have been due to this email not reaching enough people. If you haven't read the discussion in https://lore.kernel.org/linux-kselftest/202006141005.BA19A9D3@keescook/t/#u it's worthwhile: the _kunit.c name is discussed, and Kees lays out some more detailed rationale for it as well.
Thanks, I can see the rationale. AFAIK the main concern was "it does not distinguish it from other tests".
$> git grep 'KUNIT.*-test.o' drivers/base/power/Makefile:obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o drivers/base/test/Makefile:obj-$(CONFIG_KUNIT_DRIVER_PE_TEST) += property-entry-test.o fs/ext4/Makefile:obj-$(CONFIG_EXT4_KUNIT_TESTS) += ext4-inode-test.o kernel/Makefile:obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o lib/Makefile:obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_TEST) += kunit-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_TEST) += string-stream-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_EXAMPLE_TEST) += kunit-example-test.o
$> git grep 'KUNIT.*_kunit.o' # Returns nothing
This was definitely something we noted. Part of the goal of having _kunit.c as a filename suffix (and, perhaps more importantly, the _kunit module name suffix) was to have a way of both distinguishing KUnit tests from non-KUnit ones (of which there are quite a few already with -test names), and to have a way of quickly determining what modules contain KUnit tests. That really only works if everyone is using it, so the plan was to push this as much as possible. This'd probably include renaming most of the existing test files to match, which is a bit of a pain (particularly when converting non-KUnit tests to KUnit and similar), but is a one-time thing.
Feel free to ignore the below, but here might be one concern:
I think the problem of distinguishing KUnit tests from non-KUnit tests is a technical problem (in fact, the Kconfig entries have all the info we need), but a solution that sacrifices readability might cause unnecessary friction.
The main issue I foresee is that the _kunit.c name is opaque as to what the file does, but merely indicates one of its dependencies. Most of us clearly know that KUnit is a test framework, but it's a level of indirection nevertheless. (But _kunit_test.c is also bad, because it's unnecessarily long.) For a dozen tests, that's probably still fine. But now imagine 100s of tests, people will quickly wonder "what's this _kunit thing?". And if KUnit tests are eventually the dominant tests, does it still matter?
I worry that because of the difficulty of enforcing the name, choosing something unintuitive will also achieve the opposite result: proliferation of even more diverse names. A generic convention like "*-test.c" will be close enough to what's intuitive for most people, and we might actually have a chance of getting everyone to stick to it.
The problem of identifying all KUnit tests can be solved with a script.
Just an idea: Maybe the names are also an opportunity to distinguish real _unit_ style tests and then the rarer integration-style tests. I personally prefer using the more generic *-test.c, at least for the integration-style tests I've been working on (KUnit is still incredibly valuable for integration-style tests, because otherwise I'd have to roll my own poor-man's version of KUnit, so thank you!). Using *_kunit.c for such tests is unintuitive, because the word "unit" hints at "unit tests" -- and having descriptive (and not misleading) filenames is still important. So I hope you won't mind if *-test.c are still used where appropriate.
As Brendan alluded to in the talk, the popularity of KUnit for these integration-style tests came as something of a surprise (more due to our lack of imagination than anything else, I suspect). It's great that it's working, though: I don't think anyone wants the world filled with more single-use test "frameworks" than is necessary!
I guess the interesting thing to note is that we've to date not really made a distinction between KUnit the framework and the suite of all KUnit tests. Maybe having a separate file/module naming scheme could be a way of making that distinction, though it'd really only appear when loading tests as modules -- there'd be no indication in e.g., suite names or test results. The more obvious solution to me (at least, based on the current proposal) would be to have "integration" or similar be part of the suite name (and hence the filename, so _integration_kunit.c or similar), though even I admit that that's much uglier.
Yeah, that's not great either. Again, in the end it's probably entirely up to you, but it'd be good if the filenames are descriptive and readable (vs. a puzzle).
Maybe the idea of having the subsystem/suite distinction be represented in the code could pave the way to having different suites support different suffixes like that.
Do you know of any cases where something has/would have both unit-style tests and integration-style tests built with KUnit where the distinction needs to be clear?
None I know of, so probably not a big deal.
Brendan, Kees: do you have any thoughts?
Cheers, -- David
Thanks, -- Marco
On Thu, Aug 27, 2020 at 11:28 AM Marco Elver elver@google.com wrote:
On Thu, 27 Aug 2020 at 18:17, David Gow davidgow@google.com wrote: [...]
First of all, thanks for the talk yesterday! I only looked at this because somebody pasted the LKML link. :-)
No worries! Clearly this document needed linking -- even I was starting to suspect the reason no-one was complaining about this was that no-one had read it. :-)
[...]
While I guess this ship has sailed, and *_kunit.c is the naming convention now, I hope this is still just a recommendation and names of the form *-test.c are not banned!
The ship hasn't technically sailed until this patch is actually accepted. Thus far, we hadn't had any real complaints about the _kunit.c idea, though that may have been due to this email not reaching enough people. If you haven't read the discussion in https://lore.kernel.org/linux-kselftest/202006141005.BA19A9D3@keescook/t/#u it's worthwhile: the _kunit.c name is discussed, and Kees lays out some more detailed rationale for it as well.
Thanks, I can see the rationale. AFAIK the main concern was "it does not distinguish it from other tests".
That was my understanding as well.
$> git grep 'KUNIT.*-test.o' drivers/base/power/Makefile:obj-$(CONFIG_PM_QOS_KUNIT_TEST) += qos-test.o drivers/base/test/Makefile:obj-$(CONFIG_KUNIT_DRIVER_PE_TEST) += property-entry-test.o fs/ext4/Makefile:obj-$(CONFIG_EXT4_KUNIT_TESTS) += ext4-inode-test.o kernel/Makefile:obj-$(CONFIG_SYSCTL_KUNIT_TEST) += sysctl-test.o lib/Makefile:obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_TEST) += kunit-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_TEST) += string-stream-test.o lib/kunit/Makefile:obj-$(CONFIG_KUNIT_EXAMPLE_TEST) += kunit-example-test.o
$> git grep 'KUNIT.*_kunit.o' # Returns nothing
This was definitely something we noted. Part of the goal of having _kunit.c as a filename suffix (and, perhaps more importantly, the _kunit module name suffix) was to have a way of both distinguishing KUnit tests from non-KUnit ones (of which there are quite a few already with -test names), and to have a way of quickly determining what modules contain KUnit tests. That really only works if everyone is using it, so the plan was to push this as much as possible. This'd probably include renaming most of the existing test files to match, which is a bit of a pain (particularly when converting non-KUnit tests to KUnit and similar), but is a one-time thing.
Feel free to ignore the below, but here might be one concern:
I think the problem of distinguishing KUnit tests from non-KUnit tests is a technical problem (in fact, the Kconfig entries have all the info we need), but a solution that sacrifices readability might cause unnecessary friction.
The main issue I foresee is that the _kunit.c name is opaque as to what the file does, but merely indicates one of its dependencies. Most of us clearly know that KUnit is a test framework, but it's a level of indirection nevertheless. (But _kunit_test.c is also bad, because it's unnecessarily long.) For a dozen tests, that's probably still fine. But now imagine 100s of tests, people will quickly wonder "what's this _kunit thing?". And if KUnit tests are eventually the dominant tests, does it still matter?
I worry that because of the difficulty of enforcing the name, choosing something unintuitive will also achieve the opposite result: proliferation of even more diverse names. A generic convention like "*-test.c" will be close enough to what's intuitive for most people, and we might actually have a chance of getting everyone to stick to it.
The problem of identifying all KUnit tests can be solved with a script.
Fair point. However, converting all non-kselftests to kselftest or KUnit will likely take time, and kselftest will likely be with us forever. In short, other tests are likely to co-exist with KUnit for a long time if not in perpetuity; thus, I think Kees' point stands in this case.
Just an idea: Maybe the names are also an opportunity to distinguish real _unit_ style tests and then the rarer integration-style tests. I personally prefer using the more generic *-test.c, at least for the integration-style tests I've been working on (KUnit is still incredibly valuable for integration-style tests, because otherwise I'd have to roll my own poor-man's version of KUnit, so thank you!). Using *_kunit.c for such tests is unintuitive, because the word "unit" hints at "unit tests" -- and having descriptive (and not misleading) filenames is still important. So I hope you won't mind if *-test.c are still used where appropriate.
As Brendan alluded to in the talk, the popularity of KUnit for these integration-style tests came as something of a surprise (more due to our lack of imagination than anything else, I suspect). It's great that it's working, though: I don't think anyone wants the world filled with more single-use test "frameworks" than is necessary!
I guess the interesting thing to note is that we've to date not really made a distinction between KUnit the framework and the suite of all KUnit tests. Maybe having a separate file/module naming scheme could be a way of making that distinction, though it'd really only appear when loading tests as modules -- there'd be no indication in e.g., suite names or test results. The more obvious solution to me (at least, based on the current proposal) would be to have "integration" or similar be part of the suite name (and hence the filename, so _integration_kunit.c or similar), though even I admit that that's much uglier.
Very good point.
Yeah, that's not great either. Again, in the end it's probably entirely up to you, but it'd be good if the filenames are descriptive and readable (vs. a puzzle).
Yeah, I have no good answers either.
Dumb idea: name integration tests *_kint.c. Probably worse and more confusing than either just *_kunit.c or *_integration_{test|kunit}.c.
As misleading as it is to have the label "unit" on an integration test, I tend to think that asking devs to label their tests properly as either unit tests or integration tests is unrealistic.
Then again, does it really matter for anyone other than the maintainer and devs who contribute to the test?
Maybe the idea of having the subsystem/suite distinction be represented in the code could pave the way to having different suites support different suffixes like that.
I don't think I follow.
Do you know of any cases where something has/would have both unit-style tests and integration-style tests built with KUnit where the distinction needs to be clear?
None I know of, so probably not a big deal.
Brendan, Kees: do you have any thoughts?
Marco and Kees both make good points. I don't think there is a right or wrong decision here; I think we just need to pick.
Personally, I like *-test.c, but that's probably just because that was my original intention: I am not really sure that the costs/benefits really make it superior to *-kunit.c.
David, it's your patch, so it's up to you; however, if you want to pick *-test.c, and blame any consequences on me, feel free.
Cheers
On Fri, Aug 28, 2020 at 12:17:05AM +0800, David Gow wrote:
On Thu, Aug 27, 2020 at 9:14 PM Marco Elver elver@google.com wrote:
Just an idea: Maybe the names are also an opportunity to distinguish real _unit_ style tests and then the rarer integration-style tests. I personally prefer using the more generic *-test.c, at least for the integration-style tests I've been working on (KUnit is still incredibly valuable for integration-style tests, because otherwise I'd have to roll my own poor-man's version of KUnit, so thank you!). Using *_kunit.c for such tests is unintuitive, because the word "unit" hints at "unit tests" -- and having descriptive (and not misleading) filenames is still important. So I hope you won't mind if *-test.c are still used where appropriate.
This is a good point, and I guess not one that has really been examined. I'm not sure what to think of some of the lib/ tests. test_user_copy seems to be a "unit" test -- it's validating the function family vs all kinds of arguments and conditions. But test_overflow is less unit and more integration, which includes "do all of these allocators end up acting the same way?" etc
I'm not really sure what to make of that -- I don't really have a formal testing background.
As Brendan alluded to in the talk, the popularity of KUnit for these integration-style tests came as something of a surprise (more due to our lack of imagination than anything else, I suspect). It's great that it's working, though: I don't think anyone wants the world filled with more single-use test "frameworks" than is necessary!
I guess the interesting thing to note is that we've to date not really made a distinction between KUnit the framework and the suite of all KUnit tests. Maybe having a separate file/module naming scheme could be a way of making that distinction, though it'd really only appear when loading tests as modules -- there'd be no indication in e.g., suite names or test results. The more obvious solution to me (at least, based on the current proposal) would be to have "integration" or similar be part of the suite name (and hence the filename, so _integration_kunit.c or similar), though even I admit that that's much uglier. Maybe the idea of having the subsystem/suite distinction be represented in the code could pave the way to having different suites support different suffixes like that.
Heh, yeah, let's not call them "_integration_kunit.c" ;) _behavior.c? _integration.c?
Do you know of any cases where something has/would have both unit-style tests and integration-style tests built with KUnit where the distinction needs to be clear?
This is probably the right question. :)
On Tue, Sep 1, 2020 at 7:47 AM Kees Cook keescook@chromium.org wrote:
On Fri, Aug 28, 2020 at 12:17:05AM +0800, David Gow wrote:
On Thu, Aug 27, 2020 at 9:14 PM Marco Elver elver@google.com wrote:
Just an idea: Maybe the names are also an opportunity to distinguish real _unit_ style tests and then the rarer integration-style tests. I personally prefer using the more generic *-test.c, at least for the integration-style tests I've been working on (KUnit is still incredibly valuable for integration-style tests, because otherwise I'd have to roll my own poor-man's version of KUnit, so thank you!). Using *_kunit.c for such tests is unintuitive, because the word "unit" hints at "unit tests" -- and having descriptive (and not misleading) filenames is still important. So I hope you won't mind if *-test.c are still used where appropriate.
This is a good point, and I guess not one that has really been examined. I'm not sure what to think of some of the lib/ tests. test_user_copy seems to be a "unit" test -- it's validating the function family vs all kinds of arguments and conditions. But test_overflow is less unit and more integration, which includes "do all of these allocators end up acting the same way?" etc
I'm not really sure what to make of that -- I don't really have a formal testing background.
I'm not sure we need a super-precise definition here (maybe just because I don't have one), and really just need to work out what distinctions are actually useful. For example, I'm not sure there's any real advantage to treating test_user_copy and test_overflow differently. The KCSAN tests which spawn lots of threads and are both slower and less deterministic seem more obviously different, though.
I guess there are two audiences to cater for: 1. Test authors, who may wish to have both unit-style and integration-style tests, and want to distinguish them. They probably have the best idea of where the line should be drawn for their subsystems, and may have some existing style/nomenclature. 2. People running "all tests", who want to broadly understand how the whole suite of tests will behave (e.g., how long they'll take to run, are they possibly nondeterministic, are there weird hardware/software dependencies). This is where some more standardisation probably makes sense.
I'm not 100% the file/module name is the best place to make these distinctions (given that it's the Kconfig entries that are at least our current way of finding and running tests). An off-the-wall idea would be to have a flags field in the test suite structure to note things like "large/long-running test" or "nondeterministic", and have either a KUnit option to bypass them, note them in the output, or even something terrifying like parsing it out of a compiled module.
As Brendan alluded to in the talk, the popularity of KUnit for these integration-style tests came as something of a surprise (more due to our lack of imagination than anything else, I suspect). It's great that it's working, though: I don't think anyone wants the world filled with more single-use test "frameworks" than is necessary!
I guess the interesting thing to note is that we've to date not really made a distinction between KUnit the framework and the suite of all KUnit tests. Maybe having a separate file/module naming scheme could be a way of making that distinction, though it'd really only appear when loading tests as modules -- there'd be no indication in e.g., suite names or test results. The more obvious solution to me (at least, based on the current proposal) would be to have "integration" or similar be part of the suite name (and hence the filename, so _integration_kunit.c or similar), though even I admit that that's much uglier. Maybe the idea of having the subsystem/suite distinction be represented in the code could pave the way to having different suites support different suffixes like that.
Heh, yeah, let's not call them "_integration_kunit.c" ;) _behavior.c? _integration.c?
I think we'd really like something that says more strongly that this is a test (which is I suspect one of the reasons why _kunit.c has its detractors: it doesn't have the word "test" in it). The other thing to consider is that if there are multiple tests for the same thing (e.g., a unit test suite and an integration test suite), they'd still need separate, non-conflicting suite names if we wanted them to be able to be present in the same kernel.
Maybe the right thing to do is to say that, for now, the _kunit.c naming guideline only applies to "unit-style" tests.
Do you know of any cases where something has/would have both unit-style tests and integration-style tests built with KUnit where the distinction needs to be clear?
This is probably the right question. :)
I had a quick look, and we don't appear to have anything quite like this yet, at least with KUnit.
So, putting together the various bits of feedback, how about something like this: Test filenames/modules should end in _kunit.c, unless they are either a) not unit-style tests -- i.e, test something other than correctness (e.g., performance), are non-deterministic, take a long time to run (> ~1--2 seconds), or are testing across multiple subsystems -- OR b) are ports of existing tests, which may keep their existing filename (at least for now), so as not to break existing workflows.
This is a bit weaker than the existing guidelines, and will probably need tightening up once we have a better idea of what non-unit tests should be and/or the existing, inconsistently named tests are sufficiently outnumbered by the _kunit ones that people are used to it and the perceived ugliness fades away. What (if any) tooling we need around enumerating tests may end up influencing/being influenced by this a bit, too.
Thoughts? -- David
On Tue, 1 Sep 2020 at 07:31, David Gow davidgow@google.com wrote:
On Tue, Sep 1, 2020 at 7:47 AM Kees Cook keescook@chromium.org wrote:
On Fri, Aug 28, 2020 at 12:17:05AM +0800, David Gow wrote:
On Thu, Aug 27, 2020 at 9:14 PM Marco Elver elver@google.com wrote:
[...]
I guess there are two audiences to cater for:
- Test authors, who may wish to have both unit-style and
integration-style tests, and want to distinguish them. They probably have the best idea of where the line should be drawn for their subsystems, and may have some existing style/nomenclature. 2. People running "all tests", who want to broadly understand how the whole suite of tests will behave (e.g., how long they'll take to run, are they possibly nondeterministic, are there weird hardware/software dependencies). This is where some more standardisation probably makes sense.
I'm not 100% the file/module name is the best place to make these distinctions (given that it's the Kconfig entries that are at least our current way of finding and running tests).
I agree -- as you note, it's very hard to make this distinction. Since we're still discussing the best convention to use, one point I want to make is that encoding a dependency ("kunit") or type of test (unit, integration, etc.) in the name hurts scalability of our workflows. Because as soon as the dependency changes, or the type, any rename/move is very destructive to our workflow, because it immediately causes conflict with any in-flight patches. Whereas encoding this either in a comment, or via Kconfig would be less destructive.
An off-the-wall idea would be to have a flags field in the test suite structure to note things like "large/long-running test" or "nondeterministic", and have either a KUnit option to bypass them, note them in the output, or even something terrifying like parsing it out of a compiled module.
As a side-node, in the other very large codebase I have worked on, we have such markers ("size = ..."): https://docs.bazel.build/versions/master/be/common-definitions.html#common-a... However, there is also incentive to get this distinction right, because the test will be killed by the CI system if it exceeds the specified size (ran too long, OOM). Not sure we have this incentive yet.
[...]
I guess the interesting thing to note is that we've to date not really made a distinction between KUnit the framework and the suite of all KUnit tests. Maybe having a separate file/module naming scheme could be a way of making that distinction, though it'd really only appear when loading tests as modules -- there'd be no indication in e.g., suite names or test results. The more obvious solution to me (at least, based on the current proposal) would be to have "integration" or similar be part of the suite name (and hence the filename, so _integration_kunit.c or similar), though even I admit that that's much uglier. Maybe the idea of having the subsystem/suite distinction be represented in the code could pave the way to having different suites support different suffixes like that.
Heh, yeah, let's not call them "_integration_kunit.c" ;) _behavior.c? _integration.c?
If possible, I'd still prefer generic filenames, because it's easy to get wrong as we noted. Changes will cause conflicts.
I think we'd really like something that says more strongly that this is a test (which is I suspect one of the reasons why _kunit.c has its detractors: it doesn't have the word "test" in it).
^ Agreed.
The other thing to consider is that if there are multiple tests for the same thing (e.g., a unit test suite and an integration test suite), they'd still need separate, non-conflicting suite names if we wanted them to be able to be present in the same kernel.
Maybe the right thing to do is to say that, for now, the _kunit.c naming guideline only applies to "unit-style" tests.
[...]
So, putting together the various bits of feedback, how about something like this: Test filenames/modules should end in _kunit.c, unless they are either a) not unit-style tests -- i.e, test something other than correctness (e.g., performance), are non-deterministic, take a long time to run (> ~1--2 seconds), or are testing across multiple subsystems -- OR b) are ports of existing tests, which may keep their existing filename (at least for now), so as not to break existing workflows.
This is a bit weaker than the existing guidelines, and will probably need tightening up once we have a better idea of what non-unit tests should be and/or the existing, inconsistently named tests are sufficiently outnumbered by the _kunit ones that people are used to it and the perceived ugliness fades away. What (if any) tooling we need around enumerating tests may end up influencing/being influenced by this a bit, too.
Thoughts?
That could work, but it all still feels a little unsatisfying. Here's what I think the requirements for all this are:
1. Clear, intuitive, descriptive filenames ("[...] something that says more strongly that this is a test [...]").
2. Avoid renames if any of the following changes: test framework, test type or scope. I worry the most about this point, because it affects our workflows. We need to avoid unnecessary patch conflicts, keep cherry-picks simple, etc.
3. Strive for consistently named tests, regardless of type (because it's hard to get right).
4. Want to distinguish KUnit tests from non-KUnit tests. (Also consider that tooling can assist with this.)
These are the 2 options under closer consideration:
A. Original choice of "*-test.c": Satisfies 1,2,3. It seems to fail 4, per Kees's original concern.
B. "*_kunit.c": Satisfies 4, maybe 3. - Fails 1, because !strstr("_kunit.c", "test") and the resulting indirection. It hints at "unit test", but this may be a problem for (2). - Fails 2, because if the test for some reason decides to stop using KUnit (or a unit test morphs into an integration test), the file needs to be renamed.
And based on all this, why not:
C. "*-ktest.c" (or "*_ktest.c"): - Satisfies 1, because it's descriptive and clearly says it's a test; the 'k' can suggest it's an "[in-]kernel test" vs. some other hybrid test that requires a userspace component. - Satisfies 2, because neither test framework or test type need to be encoded in the filename. - Satisfies 3, because every test (that wants to use KUnit) can just use this without thinking too much about it. - Satisfies 4, because "git grep -- '[-_]ktest.[co]'" returns nothing.
Thanks, -- Marco
On Tue, Sep 1, 2020 at 8:23 PM Marco Elver elver@google.com wrote:
On Tue, 1 Sep 2020 at 07:31, David Gow davidgow@google.com wrote:
On Tue, Sep 1, 2020 at 7:47 AM Kees Cook keescook@chromium.org wrote:
On Fri, Aug 28, 2020 at 12:17:05AM +0800, David Gow wrote:
On Thu, Aug 27, 2020 at 9:14 PM Marco Elver elver@google.com wrote:
[...]
I guess there are two audiences to cater for:
- Test authors, who may wish to have both unit-style and
integration-style tests, and want to distinguish them. They probably have the best idea of where the line should be drawn for their subsystems, and may have some existing style/nomenclature. 2. People running "all tests", who want to broadly understand how the whole suite of tests will behave (e.g., how long they'll take to run, are they possibly nondeterministic, are there weird hardware/software dependencies). This is where some more standardisation probably makes sense.
I'm not 100% the file/module name is the best place to make these distinctions (given that it's the Kconfig entries that are at least our current way of finding and running tests).
I agree -- as you note, it's very hard to make this distinction. Since we're still discussing the best convention to use, one point I want to make is that encoding a dependency ("kunit") or type of test (unit, integration, etc.) in the name hurts scalability of our workflows. Because as soon as the dependency changes, or the type, any rename/move is very destructive to our workflow, because it immediately causes conflict with any in-flight patches. Whereas encoding this either in a comment, or via Kconfig would be less destructive.
This is a good point -- renaming files is definitely a pain. It's obviously my hope that KUnit sticks around long enough that it's not being added/removed as a dependency too often, particularly for the unit tests, so "_kunit" as a name doesn't worry me that much otherwise.
An off-the-wall idea would be to have a flags field in the test suite structure to note things like "large/long-running test" or "nondeterministic", and have either a KUnit option to bypass them, note them in the output, or even something terrifying like parsing it out of a compiled module.
As a side-node, in the other very large codebase I have worked on, we have such markers ("size = ..."): https://docs.bazel.build/versions/master/be/common-definitions.html#common-a... However, there is also incentive to get this distinction right, because the test will be killed by the CI system if it exceeds the specified size (ran too long, OOM). Not sure we have this incentive yet.
KUnit does have test timeouts, but they're generous (30 seconds), and not configurable per-test or per-suite. Fixing that's probably a separate feature request which, as you point out, probably isn't worth doing until we're actually seeing a problem. Maybe it's something that'll become more apparent as KUnit is integrated into KernelCI and the like, so we see more (and more varied) test runs/configs.
[...]
I guess the interesting thing to note is that we've to date not really made a distinction between KUnit the framework and the suite of all KUnit tests. Maybe having a separate file/module naming scheme could be a way of making that distinction, though it'd really only appear when loading tests as modules -- there'd be no indication in e.g., suite names or test results. The more obvious solution to me (at least, based on the current proposal) would be to have "integration" or similar be part of the suite name (and hence the filename, so _integration_kunit.c or similar), though even I admit that that's much uglier. Maybe the idea of having the subsystem/suite distinction be represented in the code could pave the way to having different suites support different suffixes like that.
Heh, yeah, let's not call them "_integration_kunit.c" ;) _behavior.c? _integration.c?
If possible, I'd still prefer generic filenames, because it's easy to get wrong as we noted. Changes will cause conflicts.
I think we'd really like something that says more strongly that this is a test (which is I suspect one of the reasons why _kunit.c has its detractors: it doesn't have the word "test" in it).
^ Agreed.
I'm not personally convinced that "kunit" isn't something people could associate with tests, particularly as it becomes more popular, but if people really dislike it, we could have"_unittest.c" or similar. There's a balancing act between being generic (and not distinguishing between unit/integration/etc tests) and being consistent or avoiding renames. Take the case where there's a set of unit tests in a "-test.c" file, and an integration test is written as well: it probably should go in a speparate file, so now you'd either have a "-test.c" and a separate "-integration-test.c" (or the other way around if the integration test was written first), or the "-test.c" file would be renamed.
The other thing to consider is that if there are multiple tests for the same thing (e.g., a unit test suite and an integration test suite), they'd still need separate, non-conflicting suite names if we wanted them to be able to be present in the same kernel.
Maybe the right thing to do is to say that, for now, the _kunit.c naming guideline only applies to "unit-style" tests.
[...]
So, putting together the various bits of feedback, how about something like this: Test filenames/modules should end in _kunit.c, unless they are either a) not unit-style tests -- i.e, test something other than correctness (e.g., performance), are non-deterministic, take a long time to run (> ~1--2 seconds), or are testing across multiple subsystems -- OR b) are ports of existing tests, which may keep their existing filename (at least for now), so as not to break existing workflows.
This is a bit weaker than the existing guidelines, and will probably need tightening up once we have a better idea of what non-unit tests should be and/or the existing, inconsistently named tests are sufficiently outnumbered by the _kunit ones that people are used to it and the perceived ugliness fades away. What (if any) tooling we need around enumerating tests may end up influencing/being influenced by this a bit, too.
Thoughts?
That could work, but it all still feels a little unsatisfying. Here's what I think the requirements for all this are:
- Clear, intuitive, descriptive filenames ("[...] something that says
more strongly that this is a test [...]").
- Avoid renames if any of the following changes: test framework, test
type or scope. I worry the most about this point, because it affects our workflows. We need to avoid unnecessary patch conflicts, keep cherry-picks simple, etc.
- Strive for consistently named tests, regardless of type (because
it's hard to get right).
- Want to distinguish KUnit tests from non-KUnit tests. (Also
consider that tooling can assist with this.)
I think that these are somewhat in conflict with each other, which is what makes this complicated. Particularly, it's going to be difficult to both avoid renames if the test framework changes and to distinguish between KUnit and non-KUnit tests by filename.
I personally think that of these requirements, 2 is probably the one that would cause people the most real-world pain. I'm not sure how often test type or scope changes enough to be worth the rename, and I hope KUnit survives long enough and is useful enough that test framework changes are kept to a minimum, but this has already irritated enough people porting tests to KUnit to be a noticeable issue. One possibility is to focus on module names, which are probably more important and can be renamed without changing the filename, though that's pretty ugly.
I actually think "_kunit.c" probably is descriptive/intuitive enough to meet (1) -- or at least will be once KUnit is more widely used -- but it does conflict a bit with 2.
It'd be nice to have consistently named tests, but we're not there at the moment, so fixing it will require a lot of renaming things. It's looking increasingly unlikely that we'll be able to do that for everything, so making this a recommendation for new test suites is probably the best we're likely to get.
These are the 2 options under closer consideration:
A. Original choice of "*-test.c": Satisfies 1,2,3. It seems to fail 4, per Kees's original concern.
Kees also brings up that using hyphens instead of underscores causes some inconsistency with module names, which is a bit of a pain.
B. "*_kunit.c": Satisfies 4, maybe 3.
- Fails 1, because !strstr("_kunit.c", "test") and the resulting
indirection. It hints at "unit test", but this may be a problem for (2).
- Fails 2, because if the test for some reason decides to stop using
KUnit (or a unit test morphs into an integration test), the file needs to be renamed.
And based on all this, why not:
C. "*-ktest.c" (or "*_ktest.c"):
- Satisfies 1, because it's descriptive and clearly says it's a
test; the 'k' can suggest it's an "[in-]kernel test" vs. some other hybrid test that requires a userspace component.
- Satisfies 2, because neither test framework or test type need to
be encoded in the filename.
- Satisfies 3, because every test (that wants to use KUnit) can just
use this without thinking too much about it.
- Satisfies 4, because "git grep -- '[-_]ktest.[co]'" returns nothing.
My concern with this is that we're introducing new jargon either way: does having "test" in the name outweigh the potential confusion from having "ktest" be in the filename only for "KUnit tests". So my feeling is that this would've been really useful if we'd named KUnit KTest (which, ironically, I think Brendan had considered) instead, but as-is is probably more confusing.
Thanks, -- Marco
At the risk of just chickening out at calling this "too hard", I'm leaning towards a variant of (A) here, and going for _test, but making it a weaker recommendation: - Specifying that the module name should end in _test, rather than the source filename. Module names are easier to change without causing merge conflicts (though they're a pain to change for the user). - Only applies to new test suites, and another suffix may be used if it conflicts with an existing non-kunit test (if it conflicts with a kunit test, they should be disambiguated in the suite name). - Test types (unit, integration, some subsystem-specific thing, etc) may be disambiguated in the suite name, at the discretion of the test author. (e.g., "driver_integration" as a suite name, with "driver_integration_test" as the module name, and either "driver_integration_test.c" or "integration_test.c" as recommended test filenames, depending on if "driver" is in its own directory.)
This should satisfy 1 & 2, and go some way towards satisfying 3. We can try to come up with some other technical solution to 4 if we need to.
Unless the objections are particularly earth-shattering, I'll do a new version of the patch that matches this next week. The other option is to drop the filename stuff from the document altogether, and sort it out in another patch, so we at least get some of the consistency in suite and Kconfig names.
Cheers, -- David
On Fri, Sep 04, 2020 at 12:22PM +0800, David Gow wrote: [...]
This is a good point -- renaming files is definitely a pain. It's obviously my hope that KUnit sticks around long enough that it's not being added/removed as a dependency too often, particularly for the unit tests, so "_kunit" as a name doesn't worry me that much otherwise.
Make sense. And I do also hope that once a test is a KUnit test, there is no need to change it. :-)
[...]
I'm not personally convinced that "kunit" isn't something people could associate with tests, particularly as it becomes more popular, but if people really dislike it, we could have"_unittest.c" or similar. There's a balancing act between being generic (and not distinguishing between unit/integration/etc tests) and being consistent or avoiding renames. Take the case where there's a set of unit tests in a "-test.c" file, and an integration test is written as well: it probably should go in a speparate file, so now you'd either have a "-test.c" and a separate "-integration-test.c" (or the other way around if the integration test was written first), or the "-test.c" file would be renamed.
Makes sense, too. Yeah, if we have both we'd need to distinguish one way or another. What might be particularly annoying is the case if an integration test exists first, and it had been named "_kunit.c", followed by addition of a unit test. I think the only sane thing at that point would be to do a rename of the integration test; whereas if it had been named "_test.c", I could live with there simply being a "_unit_test.c" (or similar) file for the new unit test.
[...]
- Clear, intuitive, descriptive filenames ("[...] something that says
more strongly that this is a test [...]").
- Avoid renames if any of the following changes: test framework, test
type or scope. I worry the most about this point, because it affects our workflows. We need to avoid unnecessary patch conflicts, keep cherry-picks simple, etc.
- Strive for consistently named tests, regardless of type (because
it's hard to get right).
- Want to distinguish KUnit tests from non-KUnit tests. (Also
consider that tooling can assist with this.)
I think that these are somewhat in conflict with each other, which is what makes this complicated. Particularly, it's going to be difficult to both avoid renames if the test framework changes and to distinguish between KUnit and non-KUnit tests by filename.
I personally think that of these requirements, 2 is probably the one that would cause people the most real-world pain. I'm not sure how often test type or scope changes enough to be worth the rename, and I hope KUnit survives long enough and is useful enough that test framework changes are kept to a minimum, but this has already irritated enough people porting tests to KUnit to be a noticeable issue. One possibility is to focus on module names, which are probably more important and can be renamed without changing the filename, though that's pretty ugly.
I actually think "_kunit.c" probably is descriptive/intuitive enough to meet (1) -- or at least will be once KUnit is more widely used -- but it does conflict a bit with 2.
It'd be nice to have consistently named tests, but we're not there at the moment, so fixing it will require a lot of renaming things. It's looking increasingly unlikely that we'll be able to do that for everything, so making this a recommendation for new test suites is probably the best we're likely to get.
These are the 2 options under closer consideration:
A. Original choice of "*-test.c": Satisfies 1,2,3. It seems to fail 4, per Kees's original concern.
Kees also brings up that using hyphens instead of underscores causes some inconsistency with module names, which is a bit of a pain.
B. "*_kunit.c": Satisfies 4, maybe 3.
- Fails 1, because !strstr("_kunit.c", "test") and the resulting
indirection. It hints at "unit test", but this may be a problem for (2).
- Fails 2, because if the test for some reason decides to stop using
KUnit (or a unit test morphs into an integration test), the file needs to be renamed.
And based on all this, why not:
C. "*-ktest.c" (or "*_ktest.c"):
- Satisfies 1, because it's descriptive and clearly says it's a
test; the 'k' can suggest it's an "[in-]kernel test" vs. some other hybrid test that requires a userspace component.
- Satisfies 2, because neither test framework or test type need to
be encoded in the filename.
- Satisfies 3, because every test (that wants to use KUnit) can just
use this without thinking too much about it.
- Satisfies 4, because "git grep -- '[-_]ktest.[co]'" returns nothing.
My concern with this is that we're introducing new jargon either way: does having "test" in the name outweigh the potential confusion from having "ktest" be in the filename only for "KUnit tests". So my feeling is that this would've been really useful if we'd named KUnit KTest (which, ironically, I think Brendan had considered) instead, but as-is is probably more confusing.
Make sense, too.
At the risk of just chickening out at calling this "too hard", I'm leaning towards a variant of (A) here, and going for _test, but making it a weaker recommendation:
- Specifying that the module name should end in _test, rather than the
source filename. Module names are easier to change without causing merge conflicts (though they're a pain to change for the user).
- Only applies to new test suites, and another suffix may be used if
it conflicts with an existing non-kunit test (if it conflicts with a kunit test, they should be disambiguated in the suite name).
- Test types (unit, integration, some subsystem-specific thing, etc)
may be disambiguated in the suite name, at the discretion of the test author. (e.g., "driver_integration" as a suite name, with "driver_integration_test" as the module name, and either "driver_integration_test.c" or "integration_test.c" as recommended test filenames, depending on if "driver" is in its own directory.)
This should satisfy 1 & 2, and go some way towards satisfying 3. We can try to come up with some other technical solution to 4 if we need to.
Unless the objections are particularly earth-shattering, I'll do a new version of the patch that matches this next week. The other option is to drop the filename stuff from the document altogether, and sort it out in another patch, so we at least get some of the consistency in suite and Kconfig names.
Thanks for the detailed answer. Your plan sounds good to me. I'm fine either way, as long as requirement (2) is somehow addressed, and we do not end up with unnecessary renames.
Thanks, -- Marco
linux-kselftest-mirror@lists.linaro.org
-
Brendan Higgins -
David Gow -
Kees Cook -
Marco Elver