On Tue, Apr 13, 2021 at 8:08 AM Daniel Latypov <dlaty...@google.com> wrote: > > This adds unit tests for kunit_filter_subsuite() and > kunit_filter_suites(). > > Note: what the executor means by "subsuite" is the array of suites > corresponding to each test file. > > This patch lightly refactors executor.c to avoid the use of global > variables to make it testable. > It also includes a clever `kfree_at_end()` helper that makes this test > easier to write than it otherwise would have been. > > Tested by running just the new tests using itself > $ ./tools/testing/kunit/kunit.py run '*exec*' > > Signed-off-by: Daniel Latypov <dlaty...@google.com>
I really like this test, thanks.
A few small notes below, including what I think is a missing
kfree_at_end() call.
Assuming that one issue is fixed (or I'm mistaken):
Reviewed-by: David Gow <david...@google.com>
-- David
> ---
> lib/kunit/executor.c | 26 ++++----
> lib/kunit/executor_test.c | 132 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 147 insertions(+), 11 deletions(-)
> create mode 100644 lib/kunit/executor_test.c
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 15832ed44668..96a4ae983786 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -19,7 +19,7 @@ MODULE_PARM_DESC(filter_glob,
> "Filter which KUnit test suites run at boot-time, e.g.
> list*");
>
> static struct kunit_suite * const *
> -kunit_filter_subsuite(struct kunit_suite * const * const subsuite)
> +kunit_filter_subsuite(struct kunit_suite * const * const subsuite, const
> char *filter_glob)
> {
> int i, n = 0;
> struct kunit_suite **filtered;
> @@ -52,19 +52,14 @@ struct suite_set {
> struct kunit_suite * const * const *end;
> };
>
> -static struct suite_set kunit_filter_suites(void)
> +static struct suite_set kunit_filter_suites(const struct suite_set
> *suite_set,
> + const char *filter_glob)
> {
> int i;
> struct kunit_suite * const **copy, * const *filtered_subsuite;
> struct suite_set filtered;
>
> - const size_t max = __kunit_suites_end - __kunit_suites_start;
> -
> - if (!filter_glob) {
> - filtered.start = __kunit_suites_start;
> - filtered.end = __kunit_suites_end;
> - return filtered;
> - }
> + const size_t max = suite_set->end - suite_set->start;
>
> copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL);
> filtered.start = copy;
> @@ -74,7 +69,7 @@ static struct suite_set kunit_filter_suites(void)
> }
>
> for (i = 0; i < max; ++i) {
> - filtered_subsuite =
> kunit_filter_subsuite(__kunit_suites_start[i]);
> + filtered_subsuite =
> kunit_filter_subsuite(suite_set->start[i], filter_glob);
> if (filtered_subsuite)
> *copy++ = filtered_subsuite;
> }
> @@ -98,8 +93,13 @@ static void kunit_print_tap_header(struct suite_set
> *suite_set)
> int kunit_run_all_tests(void)
> {
> struct kunit_suite * const * const *suites;
> + struct suite_set suite_set = {
> + .start = __kunit_suites_start,
> + .end = __kunit_suites_end,
> + };
>
> - struct suite_set suite_set = kunit_filter_suites();
> + if (filter_glob)
> + suite_set = kunit_filter_suites(&suite_set, filter_glob);
>
> kunit_print_tap_header(&suite_set);
>
> @@ -115,4 +115,8 @@ int kunit_run_all_tests(void)
> return 0;
> }
>
> +#if IS_BUILTIN(CONFIG_KUNIT_TEST)
> +#include "executor_test.c"
> +#endif
> +
> #endif /* IS_BUILTIN(CONFIG_KUNIT) */
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> new file mode 100644
> index 000000000000..8e925395beeb
> --- /dev/null
> +++ b/lib/kunit/executor_test.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for the KUnit executor.
> + *
> + * Copyright (C) 2021, Google LLC.
> + * Author: Daniel Latypov <dlaty...@google.com>
> + */
> +
> +#include <kunit/test.h>
> +
> +static void kfree_at_end(struct kunit *test, const void *to_free);
> +static struct kunit_suite *alloc_fake_suite(struct kunit *test,
> + const char *suite_name);
> +
> +static void filter_subsuite_test(struct kunit *test)
> +{
> + struct kunit_suite *subsuite[3] = {NULL, NULL, NULL};
> + struct kunit_suite * const *filtered;
> +
> + subsuite[0] = alloc_fake_suite(test, "suite1");
> + subsuite[1] = alloc_fake_suite(test, "suite2");
> +
> + /* Want: suite1, suite2, NULL -> suite2, NULL */
> + filtered = kunit_filter_subsuite(subsuite, "suite2*");
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered);
> + kfree_at_end(test, filtered);
> +
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered[0]);
> + KUNIT_EXPECT_STREQ(test, (const char *)filtered[0]->name, "suite2");
Is it worth testing that filtered[0] == subsuite[1], not just the
name? (I suspect it doesn't really matter, but that seems to be what's
happening in filter_suites_test() below.)
> +
> + KUNIT_EXPECT_FALSE(test, filtered[1]);
> +}
> +
> +static void filter_subsuite_to_empty_test(struct kunit *test)
> +{
> + struct kunit_suite *subsuite[3] = {NULL, NULL, NULL};
> + struct kunit_suite * const *filtered;
> +
> + subsuite[0] = alloc_fake_suite(test, "suite1");
> + subsuite[1] = alloc_fake_suite(test, "suite2");
> +
> + filtered = kunit_filter_subsuite(subsuite, "not_found");
> + kfree_at_end(test, filtered); /* just in case */
> +
> + KUNIT_EXPECT_FALSE_MSG(test, filtered,
> + "should be NULL to indicate no match");
> +}
> +
> +static void kfree_subsuites_at_end(struct kunit *test, struct suite_set
> *suite_set)
> +{
> + struct kunit_suite * const * const *suites;
> +
> + for (suites = suite_set->start; suites < suite_set->end; suites++)
> + kfree_at_end(test, *suites);
> +}
> +
> +static void filter_suites_test(struct kunit *test)
> +{
> + /* Suites per-file are stored as a NULL terminated array */
> + struct kunit_suite *subsuites[2][2] = {
> + {NULL, NULL},
> + {NULL, NULL},
> + };
> + /* Match the memory layout of suite_set */
> + struct kunit_suite * const * const suites[2] = {
> + subsuites[0], subsuites[1],
> + };
> +
> + const struct suite_set suite_set = {
> + .start = suites,
> + .end = suites + 2,
> + };
> + struct suite_set filtered = {.start = NULL, .end = NULL};
> +
> + /* Emulate two files, each having one suite */
> + subsuites[0][0] = alloc_fake_suite(test, "suite0");
> + subsuites[1][0] = alloc_fake_suite(test, "suite1");
> +
> + /* Filter out suite1 */
> + filtered = kunit_filter_suites(&suite_set, "suite0");
> + kfree_subsuites_at_end(test, &filtered); /* let us use ASSERTs
> without leaking */
Do we also need to kfree_at_end(test, &filtered.start) here?
> + KUNIT_ASSERT_EQ(test, filtered.end - filtered.start, (ptrdiff_t) 1);
> +
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered.start);
> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filtered.start[0]);
> + KUNIT_EXPECT_PTR_EQ(test, filtered.start[0][0], subsuites[0][0]);
> +}
> +
> +static struct kunit_case executor_test_cases[] = {
> + KUNIT_CASE(filter_subsuite_test),
> + KUNIT_CASE(filter_subsuite_to_empty_test),
> + KUNIT_CASE(filter_suites_test),
> + {}
> +};
> +
> +static struct kunit_suite executor_test_suite = {
> + .name = "kunit_executor_test",
> + .test_cases = executor_test_cases,
> +};
> +
> +kunit_test_suites(&executor_test_suite);
> +
> +/* Test helpers */
> +
> +static void kfree_res_free(struct kunit_resource *res)
> +{
> + kfree(res->data);
> +}
> +
> +/* Use the resource API to register a call to kfree(to_free).
> + * Since we never actually use the resource, it's safe to use on const data.
> + */
> +static void kfree_at_end(struct kunit *test, const void *to_free)
> +{
> + /* kfree() handles NULL already, but avoid allocating a no-op
> cleanup. */
> + if (IS_ERR_OR_NULL(to_free))
> + return;
> + kunit_alloc_and_get_resource(test, NULL, kfree_res_free, GFP_KERNEL,
> + (void *)to_free);
> +}
This actually seems useful enough to move out of this test and have as
part of the KUnit framework proper. Admittedly, I normally am very
sceptical about doing this when there's only one user, but this does
seem more obviously useful than most things. As a bonus, it could
reuse the kunit_kmalloc_free() function, rather than having its own
kfree_res_free() helper.
> +
> +static struct kunit_suite *alloc_fake_suite(struct kunit *test,
> + const char *suite_name)
> +{
> + struct kunit_suite *suite;
> +
> + /* We normally never expect to allocate suites, hence the non-const
> cast. */
> + suite = kunit_kzalloc(test, sizeof(*suite), GFP_KERNEL);
> + strncpy((char *)suite->name, suite_name, sizeof(suite->name));
> +
> + return suite;
> +}
>
> base-commit: 1678e493d530e7977cce34e59a86bb86f3c5631e
> --
> 2.31.1.295.g9ea45b61b8-goog
>
smime.p7s
Description: S/MIME Cryptographic Signature
