Date:   Fri, 10 Dec 2021 11:00:51 +0530
From:   Harinder Singh <sharinder@...gle.com>
To:     tim.bird@...y.com
Cc:     davidgow@...gle.com, brendanhiggins@...gle.com, shuah@...nel.org,
        corbet@....net, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/7] Documentation: KUnit: Restyle Test Style and
 Nomenclature page

Hello Tim,

Thanks for the review comments.

Please see my comments below.

On Wed, Dec 8, 2021 at 12:16 AM <Tim.Bird@...y.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Harinder Singh <sharinder@...gle.com>
> >
> > Rewrite page to enhance content consistency.
> >
> > Signed-off-by: Harinder Singh <sharinder@...gle.com>
> > ---
> >  Documentation/dev-tools/kunit/style.rst | 101 ++++++++++++------------
> >  1 file changed, 49 insertions(+), 52 deletions(-)
> >
> > diff --git a/Documentation/dev-tools/kunit/style.rst b/Documentation/dev-tools/kunit/style.rst
> > index 8dbcdc552606..8fae192cae28 100644
> > --- a/Documentation/dev-tools/kunit/style.rst
> > +++ b/Documentation/dev-tools/kunit/style.rst
> > @@ -4,37 +4,36 @@
> >  Test Style and Nomenclature
> >  ===========================
> >
> > -To make finding, writing, and using KUnit tests as simple as possible, it's
> > +To make finding, writing, and using KUnit tests as simple as possible, it is
> >  strongly encouraged that they are named and written according to the guidelines
> > -below. While it's possible to write KUnit tests which do not follow these rules,
> > +below. While it is possible to write KUnit tests which do not follow these rules,
> >  they may break some tooling, may conflict with other tests, and may not be run
> >  automatically by testing systems.
> >
> > -It's recommended that you only deviate from these guidelines when:
> > +It is recommended that you only deviate from these guidelines when:
> >
> > -1. Porting tests to KUnit which are already known with an existing name, or
> > -2. Writing tests which would cause serious problems if automatically run (e.g.,
> > -   non-deterministically producing false positives or negatives, or taking an
> > -   extremely long time to run).
> > +1. Porting tests to KUnit which are already known with an existing name.
> > +2. Writing tests which would cause serious problems if automatically run. For
> > +   example, non-deterministically producing false positives or negatives, or
> > +   taking a long time to run.
> >
> >  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.
> > +To make tests easy to find, they are grouped into suites and subsystems. A test
> > +suite is a group of tests which test a related area of the kernel. A subsystem
> > +is a set of test suites which test different parts of a kernel subsystem
> > +or a 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 subsystem should match a single kernel module. If the code being tested
> > +cannot 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
> > @@ -42,9 +41,8 @@ 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:
> > +unless we are actually testing other tests or the kunit framework itself. For
> > +example, subsystems could be called:
> >
> >  ``ext4``
> >    Matches the module and filesystem name.
> > @@ -56,13 +54,13 @@ Example subsystems could be:
> >    Has several components (``snd``, ``hda``, ``codec``, ``hdmi``) separated by
> >    underscores. Matches the module name.
> >
> > -Avoid names like these:
> > +Avoid names as shown in examples below:
> >
> >  ``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
> > +  This name should not use underscores, not have "kunit-test" as a
>
> This contradicts the preceding sentence.  I believe you have changed the sense
> of the recommendation.
>
> This name should not use underscores, not have ->
>    This name should use underscores, and not have
>

Done

> >    suffix, and ``qos`` is ambiguous as a subsystem name. ``power_qos`` would be a
>
> suffix, and ``qos`` -> suffix.  Also ``qos``
>
> (The way this sentence was originally structured was quite awkward.  I think it's
> better to split into two sentences)
>

Done

> >    better name.
> >  ``pc_parallel_port``
> > @@ -70,34 +68,32 @@ Avoid names like these:
> >    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.
> > +        The KUnit API and tools do not explicitly know about subsystems. They are
> > +        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.
> >
> >  Suites
> >  ------
> >
> >  KUnit tests are grouped into test suites, which cover a specific area of
> >  functionality being tested. Test suites can have shared initialisation and
>
> 'initialization' seems to be preferred to 'initialisation' in most other
> kernel documentation.  (557 instances of 'initialization' to 58 of 'initialisation')
>
> (I know this isn't part of your patch, but since this is a cleanup and consistency
> patch, maybe change this as well?)
>

Done

> > -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).
> > +shutdown code which is run for all tests in the suite. Not all subsystems need
> > +to be split into multiple test suites (for example, 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.
> >
> >  In the event that there are multiple types of test using KUnit within a
> > -subsystem (e.g., both unit tests and integration tests), they should be put into
> > -separate suites, with the type of test as the last element in the suite name.
> > -Unless these tests are actually present, avoid using ``_test``, ``_unittest`` or
> > -similar in the suite name.
> > +subsystem (for example, both unit tests and integration tests), they should be
> > +put into separate suites, with the type of test as the last element in the suite
> > +name. Unless these tests are actually present, avoid using ``_test``, ``_unittest``
> > +or similar in the suite name.
> >
> >  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:
> > +module name. For example, test suites could include:
> >
> >  ``ext4_inode``
> >    Part of the ``ext4`` subsystem, testing the ``inode`` area.
> > @@ -109,26 +105,27 @@ Example test suites could include:
> >    The ``kasan`` subsystem has only one suite, so the suite name is the same as
> >    the subsystem name.
> >
> > -Avoid names like:
> > +Avoid names, for example:
> >
> >  ``ext4_ext4_inode``
> > -  There's no reason to state the subsystem twice.
> > +  There is no reason to state the subsystem twice.
> >  ``property_entry``
> >    The suite name is ambiguous without the subsystem name.
> >  ``kasan_integration_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
> > -  ``integration_test``. Should a separate test suite with, for example, unit
> > -  tests be added, then that suite could be named ``kasan_unittest`` or similar.
> > +  just be called as ``kasan``. Do not redundantly add
> > +  ``integration_test``. It should be a separate test suite. For example, if the
> > +  unit tests are added, then that suite could be named as ``kasan_unittest`` or
> > +  similar.
> >
> >  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.
> > +codepath, property, or function. In the test output, an individual test's
> > +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
> > +Tests should be named after what they are 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.
> > @@ -136,7 +133,7 @@ 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
> > +        naming. It is a good idea to make your test functions `static` to avoid
> >          polluting the global namespace.
> >
> >  Example test names include:
> > @@ -150,7 +147,7 @@ Example test names include:
> >
> >  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 name, separated by a colon (``suite:test``).
>
> Please leave the 'i.e.'
>

Done

> >
> >  Test Kconfig Entries
> >  ====================
> > @@ -162,16 +159,16 @@ 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``
> > +  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
> > +* 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.
> > +If we are not able to meet above conditions (for example, the test is unable to
> > +be built as a module), Kconfig entries for tests should be tristate.
> >
> > -An example Kconfig entry:
> > +For example, a Kconfig entry might look like:
> >
> >  .. code-block:: none
> >
> > @@ -182,8 +179,8 @@ An example Kconfig entry:
> >               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/.
> > +               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.
> >
> > --
> > 2.34.1.400.ga245620fadb-goog
>
> Thanks for the cleanups.
>  -- Tim
>

Regards,
Harinder Singh

Powered by blists - more mailing lists