Hello Tim, Thanks for the review comments. Please see my comments below. On Wed, Dec 8, 2021 at 12:16 AM <Tim.Bird@xxxxxxxx> wrote: > > > > > -----Original Message----- > > From: Harinder Singh <sharinder@xxxxxxxxxx> > > > > Rewrite page to enhance content consistency. > > > > Signed-off-by: Harinder Singh <sharinder@xxxxxxxxxx> > > --- > > 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
