Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200505102700.7912-1-anders.roxell@linaro.org>
In-Reply-To: <20200505102700.7912-1-anders.roxell@linaro.org>
From:   David Gow <davidgow@google.com>
Date:   Wed, 6 May 2020 13:08:30 +0800
Message-ID: <CABVgOS=awiwi7APWr5HgU6Eht-VAygWPeQyNsCnAF09OLpR46A@mail.gmail.com>
Subject: Re: [PATCH v2 1/6] kunit: Kconfig: enable a KUNIT_RUN_ALL fragment
To:     Anders Roxell <anders.roxell@linaro.org>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        John Johansen <john.johansen@canonical.com>, jmorris@namei.org,
        serge@hallyn.com, "Theodore Ts'o" <tytso@mit.edu>,
        adilger.kernel@dilger.ca,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-ext4@vger.kernel.org, linux-security-module@vger.kernel.org,
        Marco Elver <elver@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk

On Tue, May 5, 2020 at 6:27 PM Anders Roxell <anders.roxell@linaro.org> wrote:
>
> Make it easier to enable all KUnit fragments.  This is needed for kernel
> test-systems, so its easy to get all KUnit tests enabled and if new gets
> added they will be enabled as well.  Fragments that has to be builtin
> will be missed if CONFIG_KUNIT_RUN_ALL is set as a module.
>
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
>  lib/kunit/Kconfig | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 95d12e3d6d95..537f37bc8400 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -41,4 +41,10 @@ config KUNIT_EXAMPLE_TEST
>           is intended for curious hackers who would like to understand how to
>           use KUnit for kernel development.
>
> +config KUNIT_RUN_ALL
> +       tristate "KUnit run all test"

I'm not 100% sure about this name and description. It only actually
"runs" the tests if they're builtin (as modules, they'll only run when
loaded).

Would KUNIT_BUILD_ALL or KUNIT_ALL_TESTS or similar be better?

It also, as mentioned, only really runs all available (i.e., with
dependencies met in the current .config) tests (as distinct from the
--alltests flag to kunit.py, which builds UML with allyesconfig), it
might be good to add this to the description or help.

Something like "Enable all KUnit tests" or "Enable all available KUnit
tests" (or even something about "all KUnit tests with satisfied
dependencies") might be clearer.

> +       help
> +         Enables all KUnit tests, if they can be enabled.
> +         That depends on if KUnit is enabled as a module or builtin.
> +
I like the first line here, but the second one could use a bit more
explanation. Maybe copy some of the boilerplate text from other tests,
e.g.:

          KUnit tests run during boot and output the results to the debug log
         in TAP format (http://testanything.org/). Only useful for kernel devs
         running the KUnit test harness, and not intended for inclusion into a
         production build.

         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.

>  endif # KUNIT
> --
> 2.20.1
>

Otherwise, this is looking good. I've done some quick testing, and it
all seems to work for me. As long as it's clear what the difference
between this and other ways of running "all tests" (like the
--alltests kunit.py option), I'm all for including this in.

Cheers,
-- David