LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Harinder Singh <sharinder@google.com>
To: tim.bird@sony.com
Cc: davidgow@google.com, brendanhiggins@google.com, shuah@kernel.org,
	corbet@lwn.net, linux-kselftest@vger.kernel.org,
	kunit-dev@googlegroups.com, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 6/7] Documentation: KUnit: Restyle Test Style and Nomenclature page
Date: Fri, 10 Dec 2021 11:00:51 +0530	[thread overview]
Message-ID: <CAHLZCaFbzuGoiFjY3jgaYORTtNdhNZh0ho=8EEfYOzEbQRX-Pg@mail.gmail.com> (raw)
In-Reply-To: <BYAPR13MB2503578CE24525BEABEEB325FD6E9@BYAPR13MB2503.namprd13.prod.outlook.com>

Hello Tim,

Thanks for the review comments.

Please see my comments below.

On Wed, Dec 8, 2021 at 12:16 AM <Tim.Bird@sony.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Harinder Singh <sharinder@google.com>
> >
> > Rewrite page to enhance content consistency.
> >
> > Signed-off-by: Harinder Singh <sharinder@google.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

  reply	other threads:[~2021-12-10  5:31 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-07  5:40 [PATCH v2 0/7] Documentation: KUnit: Rework KUnit documentation Harinder Singh
2021-12-07  5:40 ` [PATCH v2 1/7] Documentation: KUnit: Rewrite main page Harinder Singh
2021-12-07 17:11   ` Tim.Bird
2021-12-10  5:30     ` Harinder Singh
2021-12-07  5:40 ` [PATCH v2 2/7] Documentation: KUnit: Rewrite getting started Harinder Singh
2021-12-07  5:40 ` [PATCH v2 3/7] Documentation: KUnit: Added KUnit Architecture Harinder Singh
2021-12-07 17:24   ` Tim.Bird
2021-12-10  5:31     ` Harinder Singh
2021-12-10 23:08   ` Marco Elver
2021-12-16  6:12     ` Harinder Singh
2021-12-07  5:40 ` [PATCH v2 4/7] Documentation: kunit: Reorganize documentation related to running tests Harinder Singh
2021-12-07 17:33   ` Tim.Bird
2021-12-10  5:31     ` Harinder Singh
2021-12-07  5:40 ` [PATCH v2 5/7] Documentation: KUnit: Rework writing page to focus on writing tests Harinder Singh
2021-12-07 18:28   ` Tim.Bird
2021-12-10  5:31     ` Harinder Singh
2021-12-10 17:16       ` Tim.Bird
2021-12-16  5:43         ` Harinder Singh
2021-12-07  5:40 ` [PATCH v2 6/7] Documentation: KUnit: Restyle Test Style and Nomenclature page Harinder Singh
2021-12-07 18:46   ` Tim.Bird
2021-12-10  5:30     ` Harinder Singh [this message]
2021-12-07  5:40 ` [PATCH v2 7/7] Documentation: KUnit: Restyled Frequently Asked Questions Harinder Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHLZCaFbzuGoiFjY3jgaYORTtNdhNZh0ho=8EEfYOzEbQRX-Pg@mail.gmail.com' \
    --to=sharinder@google.com \
    --cc=brendanhiggins@google.com \
    --cc=corbet@lwn.net \
    --cc=davidgow@google.com \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=shuah@kernel.org \
    --cc=tim.bird@sony.com \
    --subject='Re: [PATCH v2 6/7] Documentation: KUnit: Restyle Test Style and Nomenclature page' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).