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: <20201019224556.3536790-1-dlatypov@google.com> <20201020080943.GY4077@smile.fi.intel.com>
In-Reply-To: <20201020080943.GY4077@smile.fi.intel.com>
From:   Daniel Latypov <dlatypov@google.com>
Date:   Tue, 20 Oct 2020 09:13:21 -0700
Message-ID: <CAGS_qxpPJJXa_fzeaapyAwjWKFdyzHgN7ZZgLE5V=6YR5VD1Sg@mail.gmail.com>
Subject: Re: [PATCH] lib: add basic KUnit test for lib/math
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Brendan Higgins <brendanhiggins@google.com>,
        David Gow <davidgow@google.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 20, 2020 at 1:08 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Oct 19, 2020 at 03:45:56PM -0700, Daniel Latypov wrote:
> > Add basic test coverage for files that don't require any config options:
> > * gcd.c
> > * lcm.c
> > * int_sqrt.c
> > * reciprocal_div.c
> > (Ignored int_pow.c since it's a simple textbook algorithm.)
> >
> > These tests aren't particularly interesting, but
> > * they're chosen as easy to understand examples of how to write tests
> > * provides a place to add tests for any new files in this dir
> > * written so adding new test cases to cover edge cases should be easy
>
> Is documentation not enough?
>
> I have recently wrote my first KUnit test and I found documentation pretty good
> for the start. I think we actually need more complex examples in the code (and
> useful).

I should have been more clear.
The documentation clearly covers the mechanics of how to set up a test
suite with some test cases and KUNIT_EXPECT_* calls.

But it doesn't provide much guidance in the way of _how_ to structure tests.
Or how to use more advanced features, e.g. there are only two files
tree-wide using the _MSG variants of macros:
$ ag KUNIT_.*MSG -l --ignore include/kunit/test.h
fs/ext4/inode-test.c
lib/bitfield_kunit.c
(And both happen to be written by people working on/with KUnit).

While the _MSG macros are not perfect, they add some context when
calling KUNIT_* in a loop.
I already see some tests merged that probably would benefit from using it.
Considering the perspective of an outsider whose change broke one of
those tests, they'd need to first edit the test to add more debug info
to even understand what failed.

But I can see a case for mentioning the _MSG variants in the example
test or Documentation/kunit instead of this patch.
There doesn't seem to be any mention of them at present in the docs.

To put it in kunit_example_test.c (and have the value be clear), we'd
need something like
@@ -27,6 +28,9 @@ static void example_simple_test(struct kunit *test)
         * behavior matched what was expected.
         */
        KUNIT_EXPECT_EQ(test, 1 + 1, 2);
+
+       for (x = 0; x < 2; ++x)
+               KUNIT_EXPECT_EQ_MSG(test, 42, myfunc(x), "myfunc(%d)", x);
 }

Using and testing an actual function like gcd() et al. felt a bit
better than adding a trivial function there.

>
> So, I'm in doubt these test are a good point to start with.

If the above still seems too contrived, I can take a look at where to
update kunit/usage.rst instead.


>
> --
> With Best Regards,
> Andy Shevchenko
>
>