Message-ID: <CABVgOSkhT2dVOuzzNYuYWp_s4VLPjHST++xQqeq7j0nKKfjh8g@mail.gmail.com>
Date:   Fri, 26 Aug 2022 12:55:16 +0800
From:   David Gow <davidgow@...gle.com>
To:     Sadiya Kazi <sadiyakazi@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Documentation: KUnit: Reword kunit_tool guide

On Tue, Aug 23, 2022 at 2:10 AM Sadiya Kazi <sadiyakazi@...gle.com> wrote:
>
> Updated the kunit_tool.rst guide to streamline it. The following changes
> were made:
> 1. Updated headings
> 2. Reworded content across sections
> 3. Added a cross reference to full list of command-line args
>
> Signed-off-by: Sadiya Kazi <sadiyakazi@...gle.com>
> ---

This looks okay overall, but there are definitely some parts I'm more
comfortable with than others.

The "kunit_tool How-To" name was fine by my book, but I also like
"Understanding kunit_tool".

My biggest concern here is actually that there's another patch which
removes this page entirely. It makes very little sense to tidy this up
only to delete it:
https://lore.kernel.org/all/20220822022646.98581-2-tales.aparecida@gmail.com/

While there is an argument for keeping this page (its purpose as a
"reference" differs slightly from the more "tutorialised"
run_wrapper.rst, as well as wanting to avoid breaking any external
links if we can), I think that overall, we'll probably lose this page
entirely.

Regardless, I've commented on the exact changes below.

Cheers,
-- David

>  Documentation/dev-tools/kunit/kunit-tool.rst | 82 ++++++++++----------
>  1 file changed, 42 insertions(+), 40 deletions(-)
>
> diff --git a/Documentation/dev-tools/kunit/kunit-tool.rst b/Documentation/dev-tools/kunit/kunit-tool.rst
> index ae52e0f489f9..33186679f5de 100644
> --- a/Documentation/dev-tools/kunit/kunit-tool.rst
> +++ b/Documentation/dev-tools/kunit/kunit-tool.rst
> @@ -1,8 +1,10 @@
>  .. SPDX-License-Identifier: GPL-2.0
>
> -=================
> -kunit_tool How-To
> -=================
> +========================
> +Understanding kunit_tool
> +========================
> +
> +This page introduces the kunit_tool and covers the concepts and working of this tool.

Nit: just "kunit_tool", not "the kunit_tool". Also, is this sentence
really telling us anything?

>
>  What is kunit_tool?
>  ===================
> @@ -10,39 +12,37 @@ What is kunit_tool?
>  kunit_tool is a script (``tools/testing/kunit/kunit.py``) that aids in building
>  the Linux kernel as UML (`User Mode Linux
>  <http://user-mode-linux.sourceforge.net/>`_), running KUnit tests, parsing
> -the test results and displaying them in a user friendly manner.
> +the test results and displaying them in a user-friendly manner.
>
>  kunit_tool addresses the problem of being able to run tests without needing a
>  virtual machine or actual hardware with User Mode Linux. User Mode Linux is a
>  Linux architecture, like ARM or x86; however, unlike other architectures it
> -compiles the kernel as a standalone Linux executable that can be run like any
> +compiles the kernel as a standalone Linux executable. This executable can be run like any
>  other program directly inside of a host operating system. To be clear, it does
>  not require any virtualization support: it is just a regular program.
>
> -What is a .kunitconfig?
> -=======================
> +What is .kunitconfig?
> +=====================

There can be multiple .kunitconfig files, so this may make sense to
leave as "a .kunitconfig"

>
> -It's just a defconfig that kunit_tool looks for in the build directory
> -(``.kunit`` by default).  kunit_tool uses it to generate a .config as you might
> -expect. In addition, it verifies that the generated .config contains the CONFIG
> -options in the .kunitconfig; the reason it does this is so that it is easy to
> -be sure that a CONFIG that enables a test actually ends up in the .config.
> +.kunitconfig is a default configuration file (defconfig) that kunit_tool looks
> +for in the build directory (``.kunit``). The kunit_tool uses this file to
> +generate a .config. Additionally, it also verifies that the generated .config contains the CONFIG options in the .kunitconfig file. This is done to make sure that a CONFIG that enables a test is  actually part of the .config file.

Word wrapping, "the kunit_tool" -> "kunit_tool", the build directory
may not be ``.kunit``, so re-introduce "by default".

>
> -It's also possible to pass a separate .kunitconfig fragment to kunit_tool,
> +It is also possible to pass a separate .kunitconfig fragment to kunit_tool,
>  which is useful if you have several different groups of tests you wish
> -to run independently, or if you want to use pre-defined test configs for
> +to run independently, or if you want to use pre-defined test configurations for
>  certain subsystems.
>
> -Getting Started with kunit_tool
> +Getting started with kunit_tool

The capitalisation of headings in the KUnit documentation (and,
indeed, the whole kernel documentation) is pretty inconsistent. I
wouldn't mind it if these changes to improve consistency, but let's
not just change one page without committing to doing so on all of
them. Particularly since, at the moment, I think we're using (an
approximation of) title-case in more pages.

>  ===============================
>
> -If a kunitconfig is present at the root directory, all you have to do is:
> +If a kunitconfig is present at the root directory, run the following command:

Do we want to standardise on .kunitconfig or kunitconfig? I think we'd
decided that we should use ".kunitconfig" everywhere.

Also, this is outdated: the .kunitconfig needs to be in the build
directory, not the root directory.



>  .. code-block:: bash
>
>         ./tools/testing/kunit/kunit.py run
>
> -However, you most likely want to use it with the following options:
> +However, most likely you may want to use it with the following options:

Personally, I find the earlier version clearer here, but could live
with the change if there's a good reason behind it I'm missing...

>
>  .. code-block:: bash
>
> @@ -68,20 +68,20 @@ For a list of all the flags supported by kunit_tool, you can run:
>
>         ./tools/testing/kunit/kunit.py run --help
>
> -Configuring, Building, and Running Tests
> +Configuring, building, and running tests
>  ========================================
>
> -It's also possible to run just parts of the KUnit build process independently,
> -which is useful if you want to make manual changes to part of the process.
> +It is also possible to run specific parts of the KUnit build process independently.
> +This is useful if you want to make manual changes to part of the process.
>
> -A .config can be generated from a .kunitconfig by using the ``config`` argument
> +If you want to generate a .config from a .kunitconfig, you can use the ``config`` argument
>  when running kunit_tool:
>
>  .. code-block:: bash
>
>         ./tools/testing/kunit/kunit.py config
>
> -Similarly, if you just want to build a KUnit kernel from the current .config,
> +Similarly, if you want to build a KUnit kernel from the current .config,

I think the "just" here is valuable: "kunit.py run" is the default
that most people will use, and it's very easy to assume "kunit.py
build" will do everything required to build the kernel (including
config), but it doesn't.

>  you can use the ``build`` argument:
>
>  .. code-block:: bash
> @@ -95,33 +95,31 @@ run the kernel and display the test results with the ``exec`` argument:
>
>         ./tools/testing/kunit/kunit.py exec
>
> -The ``run`` command which is discussed above is equivalent to running all three
> +The ``run`` command, discussed above is equivalent to running all three
>  of these in sequence.
>
>  All of these commands accept a number of optional command-line arguments. The
>  ``--help`` flag will give a complete list of these, or keep reading this page
>  for a guide to some of the more useful ones.
>
> -Parsing Test Results
> +Parsing test results
>  ====================
>
> -KUnit tests output their results in TAP (Test Anything Protocol) format.
> -kunit_tool will, when running tests, parse this output and print a summary
> -which is much more pleasant to read. If you wish to look at the raw test
> -results in TAP format, you can pass the ``--raw_output`` argument.
> +The output of the KUnit test results are displayed in TAP (Test Anything Protocol) format.
> +When running tests, the kunit_tool parses this output and prints a plaintext, human-readable summary. To view the raw test results in TAP format, you can use the ``--raw_output`` argument.

Word wrapping.

>
>  .. code-block:: bash
>
>         ./tools/testing/kunit/kunit.py run --raw_output
>
>  The raw output from test runs may contain other, non-KUnit kernel log
> -lines. You can see just KUnit output with ``--raw_output=kunit``:
> +lines. To view only the KUnit output, you can use ``--raw_output=kunit``:
>
>  .. code-block:: bash
>
>         ./tools/testing/kunit/kunit.py run --raw_output=kunit
>
> -If you have KUnit results in their raw TAP format, you can parse them and print
> +If you have KUnit results in the raw TAP format, you can parse them and print
>  the human-readable summary with the ``parse`` command for kunit_tool. This
>  accepts a filename for an argument, or will read from standard input.
>
> @@ -135,11 +133,11 @@ accepts a filename for an argument, or will read from standard input.
>  This is very useful if you wish to run tests in a configuration not supported
>  by kunit_tool (such as on real hardware, or an unsupported architecture).
>
> -Filtering Tests
> +Filtering tests
>  ===============
>
> -It's possible to run only a subset of the tests built into a kernel by passing
> -a filter to the ``exec`` or ``run`` commands. For example, if you only wanted
> +It is possible to run only a subset of the tests built into a kernel by passing
> +a filter to the ``exec`` or ``run`` commands. For example, if you want
>  to run KUnit resource tests, you could use:

Again, I'm quite partial to the "only" here, as test filtering can
only _remove_ tests/suites from the set to be executed, not add them.


>
>  .. code-block:: bash
> @@ -148,15 +146,14 @@ to run KUnit resource tests, you could use:
>
>  This uses the standard glob format for wildcards.
>
> -Running Tests on QEMU
> +Running tests on QEMU
>  =====================
>
> -kunit_tool supports running tests on QEMU as well as via UML (as mentioned
> -elsewhere). The default way of running tests on QEMU requires two flags:
> +kunit_tool supports running tests on QEMU as well as via UML. The default way of running tests on QEMU requires two flags:

Word Wrapping

>
>  ``--arch``
>         Selects a collection of configs (Kconfig as well as QEMU configs
> -       options, etc) that allow KUnit tests to be run on the specified
> +       options and so on) that allow KUnit tests to be run on the specified

Do we really need to remove "etc"? "and so on" is less visually distinct, IMO.

>         architecture in a minimal way; this is usually not much slower than
>         using UML. The architecture argument is the same as the name of the
>         option passed to the ``ARCH`` variable used by Kbuild. Not all
> @@ -196,8 +193,8 @@ look something like this:
>                 --jobs=12 \
>                 --qemu_config=./tools/testing/kunit/qemu_configs/x86_64.py
>
> -Other Useful Options
> -====================
> +Other useful options
> +======================
>
>  kunit_tool has a number of other command-line arguments which can be useful
>  when adapting it to fit your environment or needs.
> @@ -228,5 +225,10 @@ Some of the more useful ones are:
>          dependencies by adding ``CONFIG_KUNIT_ALL_TESTS=1`` to your
>          .kunitconfig is preferable.
>
> -There are several other options (and new ones are often added), so do check
> +There are several other options (and new ones are often added), so do run
>  ``--help`` if you're looking for something not mentioned here.
> +For more information on these options, see `Command-line-arguments
> +<https://www.kernel.org/doc/html/latest/dev-tools/kunit/run_wrapper.html#command-line-arguments>`__
> +
> +
> +.

Newlines, full stop at the end of this doc should be removed.

> --
> 2.37.1.595.g718a3a8f04-goog
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4003 bytes)

Powered by blists - more mailing lists