Messages in this thread | | | Date | Mon, 14 Jun 2021 08:42:05 +0200 | From | Christoph Hellwig <> | Subject | Re: [PATCH v3 1/1] lib: Convert UUID runtime test to KUnit |
| |
> +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?
> -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?
> +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 *".
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.
| |