Re: [PATCH] kunit: add unit test for filtering suites by names

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 9421 bytes --]

On Tue, Apr 13, 2021 at 8:08 AM Daniel Latypov <dlatypov@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 <dlatypov@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 <davidgow@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 <dlatypov@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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4000 bytes --]