linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Latypov <dlatypov@google.com>
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>
Subject: Re: [PATCH] lib: add basic KUnit test for lib/math
Date: Tue, 20 Oct 2020 09:13:21 -0700	[thread overview]
Message-ID: <CAGS_qxpPJJXa_fzeaapyAwjWKFdyzHgN7ZZgLE5V=6YR5VD1Sg@mail.gmail.com> (raw)
In-Reply-To: <20201020080943.GY4077@smile.fi.intel.com>

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
>
>

  reply	other threads:[~2020-10-20 16:13 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-19 22:45 [PATCH] lib: add basic KUnit test for lib/math Daniel Latypov
2020-10-20  0:45 ` kernel test robot
2020-10-20  8:09 ` Andy Shevchenko
2020-10-20 16:13   ` Daniel Latypov [this message]
2020-10-21  3:40 ` David Gow
2020-10-21 17:47   ` Daniel Latypov
2020-10-22 15:06     ` Andy Shevchenko
2020-10-22 16:26       ` Daniel Latypov
2020-10-22 18:51         ` Brendan Higgins
2020-10-22 19:10         ` Andy Shevchenko
2020-10-22 19:12           ` Andy Shevchenko
2020-10-22 18:53       ` Brendan Higgins
2020-10-22 19:05         ` Andy Shevchenko
2020-10-22 21:21           ` Brendan Higgins
2020-10-23  9:02             ` Andy Shevchenko
2020-11-02 14:51 ` kernel test robot
2020-11-03  1:32 ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAGS_qxpPJJXa_fzeaapyAwjWKFdyzHgN7ZZgLE5V=6YR5VD1Sg@mail.gmail.com' \
    --to=dlatypov@google.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=brendanhiggins@google.com \
    --cc=davidgow@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=skhan@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).