Re: [PATCH v3 1/1] lib: Convert UUID runtime test to KUnit

[Daniel Latypov]

On Sun, Jun 13, 2021 at 11:42 PM Christoph Hellwig <hch@xxxxxx> wrote:
>
> > +config UUID_KUNIT_TEST
> > +     tristate "Unit test for UUID" if !KUNIT_ALL_TESTS
> > +     depends on KUNIT
> > +     default KUNIT_ALL_TESTS
> > +     help
> > +       This builds the UUID unit test.
>
> Does this first help line really add any value if we have this second
> line:
>
> > +       Tests parsing functions for UUID/GUID strings.
>
> ?
>
> > +       If unsure, say N.
>
> Not specific to this case, but IMHO we can drop this line for all kunit
> tests as it is completely obvious.
>
> > @@ -354,5 +353,6 @@ obj-$(CONFIG_LIST_KUNIT_TEST) += list-test.o
> >  obj-$(CONFIG_LINEAR_RANGES_TEST) += test_linear_ranges.o
> >  obj-$(CONFIG_BITS_TEST) += test_bits.o
> >  obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o
> > +obj-$(CONFIG_UUID_KUNIT_TEST) += test_uuid.o
>
> Another meta-comment on the kunit tests:  Wouldn't it make more sense
> to name them all as CONFIG_KUNIT_TEST_FOO to allow for easier grepping?

But putting them in a "kunit namespace" by prefixing them as such
would be misleading, IMO.
The tests live adjacent to the code they test and are owned by the
same maintainers, or at least that's the intent.

And if the goal is just to find configs, then I don't see much
difference between "config.*KUNIT_TEST" and "config KUNIT_TEST.*"

>
> > -struct test_uuid_data {
> > +struct test_data {
> >       const char *uuid;
> >       guid_t le;
> >       uuid_t be;
> >  };
> >
> > -static const struct test_uuid_data test_uuid_test_data[] = {
> > +static const struct test_data correct_data[] = {
>
> What is the reason for these renames?  Is this a pattern used for
> other kunit tests?

No, this is not a pattern.
The structs can be renamed back.

>
> > +static void uuid_correct_le(struct kunit *test)
> >  {
> > +     guid_t le;
> > +     const struct test_data *data = (const struct test_data *)(test->param_value);
>
> Overly long line.  But as far as I can tell there is no need for the
> case that causes this mess anyway given that param_value is a
> "const void *".

There is no need for the cast or the brace, yes.
This is my fault.

The documentation has both since I had thought that would make how it
works more clear:
https://www.kernel.org/doc/html/latest/dev-tools/kunit/usage.html#parameterized-testing
I don't really understand my past thought process...

>
> Same for all the other instances of this.
>
> > +static void uuid_wrong_le(struct kunit *test)
> >  {
> >       guid_t le;
> > +     const char **data = (const char **)(test->param_value);
>
> No need for the second pair of braces.  Same for various other instances.