| Message ID | 20200620054944.167330-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Documentation: kunit: Add naming guidelines | expand |
Hi-- On 6/19/20 10:49 PM, David Gow wrote: > Documentation/dev-tools/kunit/index.rst | 1 + > Documentation/dev-tools/kunit/style.rst | 139 ++++++++++++++++++++++++ > 2 files changed, 140 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..117c88856fb3 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 Use tab for indentation, not spaces. > faq thanks.
I imagine +Theodore Ts'o might have some thoughts on this. +Bird, Timothy - Figured you might be interested since I think this might pertain to the KTAP discussion. On Fri, Jun 19, 2020 at 10:50 PM 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. > > [1]: > https://lore.kernel.org/linux-kselftest/202006141005.BA19A9D3@keescook/t/#u > > Signed-off-by: David Gow <davidgow@google.com> > --- > This is a first draft of some naming guidelines for KUnit tests. Note > that I haven't edited it for spelling/grammar/style yet: I wanted to get > some feedback on the actual naming conventions first. > > The issues which came most to the forefront while writing it were: > - Do we want to make subsystems a more explicit thing (make the KUnit > framework recognise them, make suites KTAP subtests of them, etc) > - I'm leaning towards no, mainly because it doesn't seem necessary, > and it makes the subsystem-with-only-one-suite case ugly. > > - Do we want to support (or encourage) Kconfig options and/or modules at > the subsystem level rather than the suite level? > - This could be nice: it'd avoid the proliferation of a large number > of tiny config options and modules, and would encourage the test for > <module> to be <module>_kunit, without other stuff in-between. > > - As test names are also function names, it may actually make sense to > decorate them with "test" or "kunit" or the like. > - If we're testing a function "foo", "test_foo" seems like as good a > name for the function as any. Sure, many cases may could have better > names like "foo_invalid_context" or something, but that won't make > sense for everything. > - Alternatively, do we split up the test name and the name of the > function implementing the test? > > Thoughts? Overall it looks pretty good. I would like to see some examples fleshed out a bit more or at least say how things like subsystem names are used, but otherwise this looks good to me. > Documentation/dev-tools/kunit/index.rst | 1 + > Documentation/dev-tools/kunit/style.rst | 139 ++++++++++++++++++++++++ > 2 files changed, 140 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..117c88856fb3 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..9363b5607262 > --- /dev/null > +++ b/Documentation/dev-tools/kunit/style.rst > @@ -0,0 +1,139 @@ > +.. 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 nit: Embolden "Do not". > +unless you are actually testing other tests or the kunit framework itself. > + > +Example subsystems could be: > + > +* ``ext4`` > +* ``apparmor`` > +* ``kasan`` Maybe add some examples that exercise the "multiple components ... separated by underscores". Some negative examples might also be good since we currently violate this rule. > +.. 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. I think we should have some way to enshrine this in KUnit, if not via code, I think we should at least say how the convention is used. > +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`` > +* ``kunit_try_catch`` > +* ``apparmor_property_entry`` > +* ``kasan`` > + > +Tests nit: "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. Can you add an example? > +.. 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. > + > +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 > +* include "If unsure, say N" 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. > + > + > -- > 2.27.0.111.gc72c7da667-goog >
On Fri, Jun 19, 2020 at 10:49:44PM -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. Thanks for preparing this! > [...] > +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 > +* include "If unsure, say N" in the help text Is this last one needed? It seems redundant -- I'm not sure any of the normal Kconfig help needs this. *shrug* Everything else looks good! Reviewed-by: Kees Cook <keescook@chromium.org>
diff --git a/Documentation/dev-tools/kunit/index.rst b/Documentation/dev-tools/kunit/index.rst index e93606ecfb01..117c88856fb3 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..9363b5607262 --- /dev/null +++ b/Documentation/dev-tools/kunit/style.rst @@ -0,0 +1,139 @@ +.. 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`` +* ``apparmor`` +* ``kasan`` + +.. 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`` +* ``kunit_try_catch`` +* ``apparmor_property_entry`` +* ``kasan`` + +Tests +----- + +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. + +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 +* include "If unsure, say N" 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. + +
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> --- This is a first draft of some naming guidelines for KUnit tests. Note that I haven't edited it for spelling/grammar/style yet: I wanted to get some feedback on the actual naming conventions first. The issues which came most to the forefront while writing it were: - Do we want to make subsystems a more explicit thing (make the KUnit framework recognise them, make suites KTAP subtests of them, etc) - I'm leaning towards no, mainly because it doesn't seem necessary, and it makes the subsystem-with-only-one-suite case ugly. - Do we want to support (or encourage) Kconfig options and/or modules at the subsystem level rather than the suite level? - This could be nice: it'd avoid the proliferation of a large number of tiny config options and modules, and would encourage the test for <module> to be <module>_kunit, without other stuff in-between. - As test names are also function names, it may actually make sense to decorate them with "test" or "kunit" or the like. - If we're testing a function "foo", "test_foo" seems like as good a name for the function as any. Sure, many cases may could have better names like "foo_invalid_context" or something, but that won't make sense for everything. - Alternatively, do we split up the test name and the name of the function implementing the test? Thoughts? Documentation/dev-tools/kunit/index.rst | 1 + Documentation/dev-tools/kunit/style.rst | 139 ++++++++++++++++++++++++ 2 files changed, 140 insertions(+) create mode 100644 Documentation/dev-tools/kunit/style.rst