Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210727225847.22185-1-jjoseaquiless@gmail.com>
In-Reply-To: <20210727225847.22185-1-jjoseaquiless@gmail.com>
From:   David Gow <davidgow@google.com>
Date:   Wed, 28 Jul 2021 09:37:38 +0800
Message-ID: <CABVgOSkT5_R3jWLnwftGHHWQKaPOMySXWhCx_=SCK6BKX4w_qw@mail.gmail.com>
Subject: Re: [PATCH] lib: use of kunit in test_parman.c
To:     =?UTF-8?Q?Jos=C3=A9_Aquiles_Guedes_de_Rezende?= 
        <jjoseaquiless@gmail.com>
Cc:     Jiri Pirko <jiri@nvidia.com>, Networking <netdev@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Brendan Higgins <brendanhiggins@google.com>,
        Daniel Latypov <dlatypov@google.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>, ~lkcamp/patches@lists.sr.ht,
        Matheus Henrique de Souza Silva 
        <matheushenriquedesouzasilva@protonmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jul 28, 2021 at 7:04 AM Jos=C3=A9 Aquiles Guedes de Rezende
<jjoseaquiless@gmail.com> wrote:
>
> Convert the parman test module to use the KUnit test framework.
> This makes thetest clearer by leveraging KUnit's assertion macros
> and test case definitions,as well as helps standardize on a testing frame=
work.
>
> Co-developed-by: Matheus Henrique de Souza Silva <matheushenriquedesouzas=
ilva@protonmail.com>
> Signed-off-by: Matheus Henrique de Souza Silva <matheushenriquedesouzasil=
va@protonmail.com>
> Signed-off-by: Jos=C3=A9 Aquiles Guedes de Rezende <jjoseaquiless@gmail.c=
om>
> ---

Thanks for porting this! A few more notes from the KUnit side.

>  lib/test_parman.c | 145 +++++++++++++++++++---------------------------
>  1 file changed, 60 insertions(+), 85 deletions(-)

This seems to be missing changes to lib/Kconfig.debug: you'll want to
modify the TEST_PARMAN config item to
- depend on KUNIT
- only appear if !KUNIT_ALL_TESTS
- default KUNIT_ALL_TESTS

It might also be nice to:
- select PARMAN (it's otherwise extremely unlikely a config will
actually pick up the test)
- maybe rename TEST_PARMAN to PARMAN_KUNIT_TEST (this is optional,
since this is a port of an existing test, but does make it clearer, so
it really depends on what existing usage looks like)

>
> diff --git a/lib/test_parman.c b/lib/test_parman.c
> index 35e32243693c..bd5010f0a412 100644
> --- a/lib/test_parman.c
> +++ b/lib/test_parman.c

The rest of this looks okay to me, but you should check out Daniel's
comments in his email, too.

Cheers,
-- David