Message-ID: <Y00f/OHxTJXH2vYc@debian.me>
Date:   Mon, 17 Oct 2022 16:27:24 +0700
From:   Bagas Sanjaya <bagasdotme@...il.com>
To:     Sadiya Kazi <sadiyakazi@...gle.com>
Cc:     brendanhiggins@...gle.com, davidgow@...gle.com,
        skhan@...uxfoundation.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 v3] Documentation: Kunit: Update architecture.rst for
 minor fixes

On Mon, Oct 17, 2022 at 06:54:53AM +0000, Sadiya Kazi wrote:
> Updated the architecture.rst page with the following changes:
> -Add missing article _the_ across the document.
> -Reword content across for style and standard.
> -Update all occurrences of Command Line to Command-line
>  across the document.
> -Correct grammatical issues, for example,
>  added _it_wherever missing.
> -Update all occurrences of “via" to either use
>  “through” or “using”.
> -Update the text preceding the external links and pushed the full
>  link to a new line for better readability.
> -Reword content under the config command to make it more clear and concise.
> 
> Signed-off-by: Sadiya Kazi <sadiyakazi@...gle.com>
> ---
> 
> Thank you Bagas for your detailed comments. 
> I think the current commit message does convey the right message as it is not a complete rewrite, hence retained it. 
> Also since we talk about the two parts of the architecture, I have retained the it as 'kunit_tool (Command-line Test Harness)' instead of 'Running Tests Options'.
> 
> Changes since v2:
> https://lore.kernel.org/linux-kselftest/20221013080545.1552573-1-sadiyakazi@google.com/
> -Updated the link descriptions as per Bagas’s feedback
> -Reworded content talking about options to run tests and added links as per Bagas’s feedback
> 
> Best Regards,
> Sadiya Kazi
> ---
>  .../dev-tools/kunit/architecture.rst          | 118 +++++++++---------
>  1 file changed, 60 insertions(+), 58 deletions(-)
> 
> diff --git a/Documentation/dev-tools/kunit/architecture.rst b/Documentation/dev-tools/kunit/architecture.rst
> index 8efe792bdcb9..52b1a30c9f89 100644
> --- a/Documentation/dev-tools/kunit/architecture.rst
> +++ b/Documentation/dev-tools/kunit/architecture.rst
> @@ -4,16 +4,17 @@
>  KUnit Architecture
>  ==================
>  
> -The KUnit architecture can be divided into two parts:
> +The KUnit architecture is divided into two parts:
>  
>  - `In-Kernel Testing Framework`_
> -- `kunit_tool (Command Line Test Harness)`_
> +- `kunit_tool (Command-line Test Harness)`_
>  
>  In-Kernel Testing Framework
>  ===========================
>  
>  The kernel testing library supports KUnit tests written in C using
> -KUnit. KUnit tests are kernel code. KUnit does several things:
> +KUnit. These KUnit tests are kernel code. KUnit performs the following
> +tasks:
>  
>  - Organizes tests
>  - Reports test results
> @@ -22,19 +23,17 @@ KUnit. KUnit tests are kernel code. KUnit does several things:
>  Test Cases
>  ----------
>  
> -The fundamental unit in KUnit is the test case. The KUnit test cases are
> -grouped into KUnit suites. A KUnit test case is a function with type
> -signature ``void (*)(struct kunit *test)``.
> -These test case functions are wrapped in a struct called
> -struct kunit_case.
> +The test case is the fundamental unit in KUnit. KUnit test cases are organised
> +into suites. A KUnit test case is a function with type signature
> +``void (*)(struct kunit *test)``. These test case functions are wrapped in a
> +struct called struct kunit_case.
>  
>  .. note:
>  	``generate_params`` is optional for non-parameterized tests.
>  
> -Each KUnit test case gets a ``struct kunit`` context
> -object passed to it that tracks a running test. The KUnit assertion
> -macros and other KUnit utilities use the ``struct kunit`` context
> -object. As an exception, there are two fields:
> +Each KUnit test case receives a ``struct kunit`` context object that tracks a
> +running test. The KUnit assertion macros and other KUnit utilities use the
> +``struct kunit`` context object. As an exception, there are two fields:
>  
>  - ``->priv``: The setup functions can use it to store arbitrary test
>    user data.
> @@ -75,14 +74,15 @@ with the KUnit test framework.
>  Executor
>  --------
>  
> -The KUnit executor can list and run built-in KUnit tests on boot.
> +The KUnit executor can list and run built-in KUnit tests on boot
>  The Test suites are stored in a linker section
> -called ``.kunit_test_suites``. For code, see:
> -https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/asm-generic/vmlinux.lds.h?h=v5.15#n945.
> +called ``.kunit_test_suites``. For the code, see ``KUNIT_TABLE()`` macro
> +definition in
> +`include/asm-generic/vmlinux.lds.h <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/asm-generic/vmlinux.lds.h?h=v6.0#n950>`_.
>  The linker section consists of an array of pointers to
>  ``struct kunit_suite``, and is populated by the ``kunit_test_suites()``
> -macro. To run all tests compiled into the kernel, the KUnit executor
> -iterates over the linker section array.
> +macro. The KUnit executor iterates over the linker section array in order to
> +run all the tests that are compiled into the kernel.
>  
>  .. kernel-figure:: kunit_suitememorydiagram.svg
>  	:alt:	KUnit Suite Memory
> @@ -90,17 +90,18 @@ iterates over the linker section array.
>  	KUnit Suite Memory Diagram
>  
>  On the kernel boot, the KUnit executor uses the start and end addresses
> -of this section to iterate over and run all tests. For code, see:
> -https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/kunit/executor.c
> -
> +of this section to iterate over and run all tests. For the implementation of the
> +executor, see
> +`lib/kunit/executor.c <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/kunit/executor.c>`_.
>  When built as a module, the ``kunit_test_suites()`` macro defines a
>  ``module_init()`` function, which runs all the tests in the compilation
>  unit instead of utilizing the executor.
>  
>  In KUnit tests, some error classes do not affect other tests
>  or parts of the kernel, each KUnit case executes in a separate thread
> -context. For code, see:
> -https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/kunit/try-catch.c?h=v5.15#n58
> +context. For the implememtation details, see ``kunit_try_catch_run()`` function
> +code in
> +`lib/kunit/try-catch.c <https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/kunit/try-catch.c?h=v5.15#n58>`_.
>  
>  Assertion Macros
>  ----------------
> @@ -111,37 +112,36 @@ All expectations/assertions are formatted as:
>  
>  - ``{EXPECT|ASSERT}`` determines whether the check is an assertion or an
>    expectation.
> +  In the event of a failure, the testing flow differs as follows:
>  
> -	- For an expectation, if the check fails, marks the test as failed
> -	  and logs the failure.
> +	- For expectations, the test is marked as failed and the failure is logged.
>  
> -	- An assertion, on failure, causes the test case to terminate
> -	  immediately.
> +	- Failing assertions, on the other hand, result in the test case being
> +	  terminated immediately.
>  
> -		- Assertions call function:
> +		- Assertions call the function:
>  		  ``void __noreturn kunit_abort(struct kunit *)``.
>  
> -		- ``kunit_abort`` calls function:
> +		- ``kunit_abort`` calls the function:
>  		  ``void __noreturn kunit_try_catch_throw(struct kunit_try_catch *try_catch)``.
>  
> -		- ``kunit_try_catch_throw`` calls function:
> +		- ``kunit_try_catch_throw`` calls the function:
>  		  ``void kthread_complete_and_exit(struct completion *, long) __noreturn;``
>  		  and terminates the special thread context.
>  
>  - ``<op>`` denotes a check with options: ``TRUE`` (supplied property
> -  has the boolean value “true”), ``EQ`` (two supplied properties are
> +  has the boolean value "true"), ``EQ`` (two supplied properties are
>    equal), ``NOT_ERR_OR_NULL`` (supplied pointer is not null and does not
> -  contain an “err” value).
> +  contain an "err" value).
>  
>  - ``[_MSG]`` prints a custom message on failure.
>  
>  Test Result Reporting
>  ---------------------
> -KUnit prints test results in KTAP format. KTAP is based on TAP14, see:
> -https://github.com/isaacs/testanything.github.io/blob/tap14/tap-version-14-specification.md.
> -KTAP (yet to be standardized format) works with KUnit and Kselftest.
> -The KUnit executor prints KTAP results to dmesg, and debugfs
> -(if configured).
> +KUnit prints the test results in KTAP format. KTAP is based on TAP14, see
> +Documentation/dev-tools/ktap.rst.
> +KTAP works with KUnit and Kselftest. The KUnit executor prints KTAP results to
> +dmesg, and debugfs (if configured).
>  
>  Parameterized Tests
>  -------------------
> @@ -150,33 +150,35 @@ Each KUnit parameterized test is associated with a collection of
>  parameters. The test is invoked multiple times, once for each parameter
>  value and the parameter is stored in the ``param_value`` field.
>  The test case includes a KUNIT_CASE_PARAM() macro that accepts a
> -generator function.
> -The generator function is passed the previous parameter and returns the next
> -parameter. It also provides a macro to generate common-case generators based on
> -arrays.
> +generator function. The generator function is passed the previous parameter
> +and returns the next parameter. It also includes a macro for generating
> +array-based common-case generators.
>  
> -kunit_tool (Command Line Test Harness)
> +kunit_tool (Command-line Test Harness)
>  ======================================
>  
> -kunit_tool is a Python script ``(tools/testing/kunit/kunit.py)``
> -that can be used to configure, build, exec, parse and run (runs other
> -commands in order) test results. You can either run KUnit tests using
> -kunit_tool or can include KUnit in kernel and parse manually.
> +``kunit_tool`` is a Python script, found in ``tools/testing/kunit/kunit.py``. It
> +is used to configure, build, execute, parse test results and run all of the
> +previous commands in correct order (i.e., configure, build, execute and parse).
> +You have two options for running KUnit tests: either build the kernel with KUnit
> +enabled and manually parse the results (see
> +Documentation/dev-tools/kunit/run_manual.rst) or use ``kunit_tool``
> +(see Documentation/dev-tools/kunit/run_wrapper.rst).
>  
>  - ``configure`` command generates the kernel ``.config`` from a
>    ``.kunitconfig`` file (and any architecture-specific options).
> -  For some architectures, additional config options are specified in the
> -  ``qemu_config`` Python script
> -  (For example: ``tools/testing/kunit/qemu_configs/powerpc.py``).
> +  The Python scripts available in ``qemu_configs`` folder
> +  (for example, ``tools/testing/kunit/qemu configs/powerpc.py``) contains
> +  additional configuration options for specific architectures.
>    It parses both the existing ``.config`` and the ``.kunitconfig`` files
> -  and ensures that ``.config`` is a superset of ``.kunitconfig``.
> -  If this is not the case, it will combine the two and run
> -  ``make olddefconfig`` to regenerate the ``.config`` file. It then
> -  verifies that ``.config`` is now a superset. This checks if all
> -  Kconfig dependencies are correctly specified in ``.kunitconfig``.
> -  ``kunit_config.py`` includes the parsing Kconfigs code. The code which
> -  runs ``make olddefconfig`` is a part of ``kunit_kernel.py``. You can
> -  invoke this command via: ``./tools/testing/kunit/kunit.py config`` and
> +  to ensure that ``.config`` is a superset of ``.kunitconfig``.
> +  If not, it will combine the two and run ``make olddefconfig`` to regenerate
> +  the ``.config`` file. It then checks to see if ``.config`` has become a superset.
> +  This verifies that all the Kconfig dependencies are correctly specified in the
> +  file ``.kunitconfig``. The ``kunit_config.py`` script contains the code for parsing
> +  Kconfigs. The code which runs ``make olddefconfig`` is part of the
> +  ``kunit_kernel.py`` script. You can invoke this command through:
> +  ``./tools/testing/kunit/kunit.py config`` and
>    generate a ``.config`` file.
>  - ``build`` runs ``make`` on the kernel tree with required options
>    (depends on the architecture and some options, for example: build_dir)
> @@ -184,8 +186,8 @@ kunit_tool or can include KUnit in kernel and parse manually.
>    To build a KUnit kernel from the current ``.config``, you can use the
>    ``build`` argument: ``./tools/testing/kunit/kunit.py build``.
>  - ``exec`` command executes kernel results either directly (using
> -  User-mode Linux configuration), or via an emulator such
> -  as QEMU. It reads results from the log via standard
> +  User-mode Linux configuration), or through an emulator such
> +  as QEMU. It reads results from the log using standard
>    output (stdout), and passes them to ``parse`` to be parsed.
>    If you already have built a kernel with built-in KUnit tests,
>    you can run the kernel and display the test results with the ``exec``

Seems like you're ignoring my review suggestions from both v1 and v2
(code locations and redundant kunit_tool summary), hence NAK until you address
them.

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists