lkml.org 
[lkml]   [2021]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@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

\
 
 \ /
  Last update: 2021-12-10 06:31    [W:0.045 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site