Re: [RFC v1 3/6] kunit: Add ability to filter attributes

[Rae Moar]

On Sat, Jun 10, 2023 at 4:29 AM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> On Sat, 10 Jun 2023 at 08:52, Rae Moar <rmoar@xxxxxxxxxx> wrote:
> >
> > Add filtering of test attributes. Users can filter tests using a
> > module_param_array called "filter". This functionality will be added to
> > kunit.py in the next patch.
> >
> > The filters will be imputed in the format:
> > "<attribute_name><operation><attribute_value>"
> >
> > Example: "speed>slow"
>
> Maybe give a full command-line example of "kunit.filter=speed>slow"?
>

Hello,

Yes I will add that in the next version.

> >
> > Operations include: >, <, >=, <=, !=, and =. These operations do not need
> > to act the same for every attribute.
>
> I assume here that operations should act the same for attributes of
> the same type, but a string attribute might behave differently from an
> int, an enum, an array, etc.
>
> As a design principle, I think we definitely want the same operation
> to act the same way between different attributes, unless there's an
> extraordinarily good reason.
>

This is a mistake on my end. I should clarify that operations should
act the same for attributes of the same type but then may differ
between the types (like ints and strings).

> >
> > Add method to parse inputted filters.
> >
> > Add the process of filtering tests based on attributes. The process of
> > filtering follows these rules:
> >
> > A test case with a set attribute overrides its parent suite's attribute
> > during filtering.
> >
> > Also, if both the test case attribute and suite attribute are unset the
> > test acts as the default attribute value during filtering.
>
> This behaviour probably needs to be documented more clearly in the
> final version.
>
> As I understand it:
> - Both tests and suites have attributes.
> - Filtering always operates at a per-test level.
> - If a test has an attribute set, then the test's value is filtered on.
> - Otherwise, the value falls back to the suite's value.
> - If neither are set, the attribute has a global "default" value, which is used.
> - If an entire suite is filtered out, it's removed, giving the
> appearance that filtering can operate on a suite level.
>

Yes, this is a great description of how it should behave. I will be
more explicit in my description for the next version.

> I actually quite like these rules, but we do need to document them.
> I'd perhaps argue that the "default attribute" could be done away with
> and we just rely on the filter function choosing whether or not
> "unset" matches a filter or not, but on the other hand, it does make
> reusing filter functions potentially easier.
>

This is true I could do away with the default. However, I do think it
helps to document how an unset attribute acts.

It may seem more unclear why an unset attribute is filtered out for
speed<=slow but not speed>slow. But this could then be documented
separate from the code.

I'm leaning toward keeping it but let me know what you think.

> >
> > Finally, add a "filter" method for the speed attribute to parse and compare
> > enum values of kunit_speed.
> >
> > Signed-off-by: Rae Moar <rmoar@xxxxxxxxxx>
> > ---
>
> One other idea: do we want filtered-out tests to totally disappear (as
> this patch does), to mark them as skipped (potentially useful, too),
> or have configurable behaviour.
>
> I think there are good reasons for each of those: having them totally
> disappear is much cleaner, but it's also useful to see what tests
> you're actually, well, skipping.

I like this idea a lot. I also really like the idea of this being a
configurable behavior.

>
> >  include/kunit/attributes.h |  22 +++++
> >  lib/kunit/attributes.c     | 172 +++++++++++++++++++++++++++++++++++++
> >  lib/kunit/executor.c       |  79 +++++++++++++----
> >  lib/kunit/executor_test.c  |   8 +-
> >  4 files changed, 258 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/kunit/attributes.h b/include/kunit/attributes.h
> > index 9fcd184cce36..bca60d1181bb 100644
> > --- a/include/kunit/attributes.h
> > +++ b/include/kunit/attributes.h
> > @@ -9,6 +9,15 @@
> >  #ifndef _KUNIT_ATTRIBUTES_H
> >  #define _KUNIT_ATTRIBUTES_H
> >
> > +/*
> > + * struct kunit_attr_filter - representation of attributes filter with the
> > + * attribute object and string input
> > + */
> > +struct kunit_attr_filter {
> > +       struct kunit_attr *attr;
> > +       char *input;
> > +};
> > +
> >  /*
> >   * Print all test attributes for a test case or suite.
> >   * Output format for test cases: "# <test_name>.<attribute>: <value>"
> > @@ -16,4 +25,17 @@
> >   */
> >  void kunit_print_attr(void *test_or_suite, bool is_test, unsigned int test_level);
> >
> > +/*
> > + * Parse attributes filter input and return an object containing the attribute
> > + * object and the string input.
> > + */
> > +struct kunit_attr_filter kunit_parse_filter_attr(char *input, int *err);
>
> Should we rename this kunit_parse_attr_filter, as it returns a
> kunit_attr_filter?
>

I will change this. I think that is cleaner.

> > +
> > +
> > +/*
> > + * Returns a copy of the suite containing only tests that pass the filter.
> > + */
> > +struct kunit_suite *kunit_filter_attr_tests(const struct kunit_suite *const suite,
> > +               struct kunit_attr_filter filter, int *err);
> > +
> >  #endif /* _KUNIT_ATTRIBUTES_H */
> > diff --git a/lib/kunit/attributes.c b/lib/kunit/attributes.c
> > index e17889f94693..4f753a28e4ee 100644
> > --- a/lib/kunit/attributes.c
> > +++ b/lib/kunit/attributes.c
> > @@ -49,6 +49,66 @@ static const char *attr_speed_to_string(void *attr, bool *to_free)
> >         return attr_enum_to_string(attr, speed_str_list, to_free);
> >  }
> >
> > +/* Filter Methods */
> > +
> > +static int int_filter(long val, const char *op, int input, int *err)
> > +{
> > +       if (!strncmp(op, "<=", 2))
> > +               return (val <= input);
> > +       else if (!strncmp(op, ">=", 2))
> > +               return (val >= input);
> > +       else if (!strncmp(op, "!=", 2))
> > +               return (val != input);
> > +       else if (!strncmp(op, ">", 1))
> > +               return (val > input);
> > +       else if (!strncmp(op, "<", 1))
> > +               return (val < input);
> > +       else if (!strncmp(op, "=", 1))
> > +               return (val == input);
> > +       *err = -EINVAL;
> > +       pr_err("kunit executor: invalid filter operation: %s\n", op);
> > +       return false;
> > +}
> > +
> > +static int attr_enum_filter(void *attr, const char *input, int *err,
> > +               const char * const str_list[], int max)
>
> As this is a generic helper function to be used by multiple types of
> attributes, let's document it a bit.

Sounds great. I will add documentation here.

>
> > +{
> > +       int i, j, input_int;
> > +       long test_val = (long)attr;
> > +       const char *input_val;
> > +
> > +       for (i = 0; input[i]; i++) {
> > +               if (!strchr("<!=>", input[i])) {
>
> Can we yoink this string of "operation characters" into a global or
> #define or something, as it recurs a few times here, and it'd be best
> to only have it in one place.

Right, that sounds good. I will change this.

>
> > +                       input_val = input + i;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       if (!input_val) {
> > +               *err = -EINVAL;
> > +               pr_err("kunit executor: filter operation not found: %s\n", input);
> > +               return false;
> > +       }
> > +
> > +       for (j = 0; j <= max; j++) {
> > +               if (!strcmp(input_val, str_list[j]))
> > +                       input_int = j;
> > +       }
> > +
> > +       if (!input_int) {
> > +               *err = -EINVAL;
> > +               pr_err("kunit executor: invalid filter input: %s\n", input);
> > +               return false;
> > +       }
> > +
> > +       return int_filter(test_val, input, input_int, err);
> > +}
> > +
> > +static int attr_speed_filter(void *attr, const char *input, int *err)
> > +{
> > +       return attr_enum_filter(attr, input, err, speed_str_list, KUNIT_SPEED_MAX);
> > +}
> > +
> >  /* Get Attribute Methods */
> >
> >  static void *attr_speed_get(void *test_or_suite, bool is_test)
> > @@ -68,6 +128,7 @@ static const struct kunit_attr speed_attr = {
> >         .name = "speed",
> >         .get_attr = attr_speed_get,
> >         .to_string = attr_speed_to_string,
> > +       .filter = attr_speed_filter,
> >         .attr_default = (void *)KUNIT_SPEED_NORMAL,
> >  };
> >
> > @@ -106,3 +167,114 @@ void kunit_print_attr(void *test_or_suite, bool is_test, unsigned int test_level
> >                 }
> >         }
> >  }
> > +
> > +/* Helper Functions to Filter Attributes */
> > +
> > +struct kunit_attr_filter kunit_parse_filter_attr(char *input, int *err)
> > +{
> > +       struct kunit_attr_filter filter;
> > +       int i, j, op_index = 0;
> > +       int attr_index = -1;
> > +       char op;
> > +
> > +       /* Parse input until operation */
> > +       for (i = 0; input[i]; i++) {
> > +               if (strchr("<>!=", input[i])) {
> > +                       op_index = i;
> > +                       break;
> > +               }
> > +               if (input[i] == ' ')
> > +                       break;
> > +       }
> > +
> > +       if (!op_index) {
> > +               *err = -EINVAL;
> > +               pr_err("kunit executor: filter operation not found: %s\n", input);
> > +               return filter;
> > +       }
> > +
> > +       op = input[op_index];
> > +       input[op_index] = '\0';
> > +
> > +       /* Find associated kunit_attr object */
> > +       for (j = 0; j < ARRAY_SIZE(kunit_attr_list); j++) {
> > +               if (!strcmp(input, kunit_attr_list[j].name)) {
> > +                       attr_index = j;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       input[op_index] = op;
> > +       filter.input = input + op_index;
> > +
> > +       if (attr_index < 0) {
> > +               *err = -EINVAL;
> > +               pr_err("kunit executor: attribute not found: %s\n", input);
> > +       } else {
> > +               filter.attr = &kunit_attr_list[attr_index];
> > +       }
> > +
> > +       return filter;
> > +}
> > +
> > +struct kunit_suite *kunit_filter_attr_tests(const struct kunit_suite *const suite,
> > +               struct kunit_attr_filter filter, int *err)
> > +{
> > +       int n = 0;
> > +       struct kunit_case *filtered, *test_case;
> > +       struct kunit_suite *copy;
> > +       void *suite_val, *test_val;
> > +       bool suite_result, test_result, default_result;
> > +
> > +       /* Allocate memory for new copy of suite and list of test cases */
> > +       copy = kmemdup(suite, sizeof(*copy), GFP_KERNEL);
> > +       if (!copy)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       kunit_suite_for_each_test_case(suite, test_case) { n++; }
> > +
> > +       filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL);
> > +       if (!filtered) {
> > +               kfree(copy);
> > +               return ERR_PTR(-ENOMEM);
> > +       }
> > +
> > +       n = 0;
> > +
> > +       /* Save filtering result on default value */
> > +       default_result = filter.attr->filter(filter.attr->attr_default, filter.input, err);
> > +
> > +       /* Save suite attribute value and filtering result on that value */
> > +       suite_val = filter.attr->get_attr((void *)suite, false);
> > +       suite_result = filter.attr->filter(suite_val, filter.input, err);
> > +
> > +       /* For each test case, save test case if passes filtering. */
> > +       kunit_suite_for_each_test_case(suite, test_case) {
> > +               test_val = filter.attr->get_attr((void *) test_case, true);
> > +               test_result = filter.attr->filter(filter.attr->get_attr(test_case, true),
> > +                               filter.input, err);
> > +               /*
> > +                * If attribute value of test case is set, filter on that value.
> > +                * If not, filter on suite value if set. If not, filter on
> > +                * default value.
> > +                */
> > +               if (test_val) {
> > +                       if (test_result)
> > +                               filtered[n++] = *test_case;
> > +               } else if (suite_val) {
> > +                       if (suite_result)
> > +                               filtered[n++] = *test_case;
> > +               } else if (default_result) {
> > +                       filtered[n++] = *test_case;
> > +               }
> > +       }
> > +
> > +       if (n == 0) {
> > +               kfree(copy);
> > +               kfree(filtered);
> > +               return NULL;
> > +       }
> > +
> > +       copy->test_cases = filtered;
> > +       return copy;
> > +}
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 767a84e32f06..c67657821eec 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -15,8 +15,12 @@ extern struct kunit_suite * const __kunit_suites_end[];
> >
> >  #if IS_BUILTIN(CONFIG_KUNIT)
> >
> > +#define MAX_FILTERS 10 // Limit of number of attribute filters
> >  static char *filter_glob_param;
> >  static char *action_param;
> > +static int filter_count;
> > +static char *filter_param[MAX_FILTERS];
> > +
> >
> >  module_param_named(filter_glob, filter_glob_param, charp, 0);
> >  MODULE_PARM_DESC(filter_glob,
> > @@ -26,15 +30,16 @@ MODULE_PARM_DESC(action,
> >                  "Changes KUnit executor behavior, valid values are:\n"
> >                  "<none>: run the tests like normal\n"
> >                  "'list' to list test names instead of running them.\n");
> > +module_param_array_named(filter, filter_param, charp, &filter_count, 0);
> >
> >  /* glob_match() needs NULL terminated strings, so we need a copy of filter_glob_param. */
> > -struct kunit_test_filter {
> > +struct kunit_glob_filter {
> >         char *suite_glob;
> >         char *test_glob;
> >  };
> >
> >  /* Split "suite_glob.test_glob" into two. Assumes filter_glob is not empty. */
> > -static void kunit_parse_filter_glob(struct kunit_test_filter *parsed,
> > +static void kunit_parse_filter_glob(struct kunit_glob_filter *parsed,
> >                                     const char *filter_glob)
> >  {
> >         const int len = strlen(filter_glob);
> > @@ -56,7 +61,7 @@ static void kunit_parse_filter_glob(struct kunit_test_filter *parsed,
> >
> >  /* Create a copy of suite with only tests that match test_glob. */
> >  static struct kunit_suite *
> > -kunit_filter_tests(const struct kunit_suite *const suite, const char *test_glob)
> > +kunit_filter_glob_tests(const struct kunit_suite *const suite, const char *test_glob)
> >  {
> >         int n = 0;
> >         struct kunit_case *filtered, *test_case;
> > @@ -110,12 +115,15 @@ static void kunit_free_suite_set(struct suite_set suite_set)
> >
> >  static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
> >                                             const char *filter_glob,
> > +                                               char **filters,
> > +                                               int filter_count,
> >                                             int *err)
> >  {
> > -       int i;
> > -       struct kunit_suite **copy, *filtered_suite;
> > +       int i, j, k;
> > +       struct kunit_suite **copy, *filtered_suite, *new_filtered_suite;
> >         struct suite_set filtered;
> > -       struct kunit_test_filter filter;
> > +       struct kunit_glob_filter parsed_glob;
> > +       struct kunit_attr_filter parsed_filters[MAX_FILTERS];
> >
> >         const size_t max = suite_set->end - suite_set->start;
> >
> > @@ -126,17 +134,49 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
> >                 return filtered;
> >         }
> >
> > -       kunit_parse_filter_glob(&filter, filter_glob);
> > +       if (filter_glob)
> > +               kunit_parse_filter_glob(&parsed_glob, filter_glob);
> >
> > -       for (i = 0; &suite_set->start[i] != suite_set->end; i++) {
> > -               if (!glob_match(filter.suite_glob, suite_set->start[i]->name))
> > -                       continue;
> > +       /* Parse attribute filters */
> > +       if (filter_count) {
> > +               for (j = 0; j < filter_count; j++) {
> > +                       parsed_filters[j] = kunit_parse_filter_attr(filters[j], err);
> > +                       if (*err)
> > +                               return filtered;
> > +               }
> > +       }
> >
> > -               filtered_suite = kunit_filter_tests(suite_set->start[i], filter.test_glob);
> > -               if (IS_ERR(filtered_suite)) {
> > -                       *err = PTR_ERR(filtered_suite);
> > -                       return filtered;
> > +       for (i = 0; &suite_set->start[i] != suite_set->end; i++) {
> > +               filtered_suite = suite_set->start[i];
> > +               if (filter_glob) {
> > +                       if (!glob_match(parsed_glob.suite_glob, filtered_suite->name))
> > +                               continue;
> > +                       filtered_suite = kunit_filter_glob_tests(filtered_suite,
> > +                                       parsed_glob.test_glob);
> > +                       if (IS_ERR(filtered_suite)) {
> > +                               *err = PTR_ERR(filtered_suite);
> > +                               return filtered;
> > +                       }
> >                 }
> > +               if (filter_count) {
> > +                       for (k = 0; k < filter_count; k++) {
> > +                               new_filtered_suite = kunit_filter_attr_tests(filtered_suite,
> > +                                               parsed_filters[k], err);
> > +
> > +                               /* Free previous copy of suite */
> > +                               if (k > 0 || filter_glob)
> > +                                       kfree(filtered_suite);
> > +                               filtered_suite = new_filtered_suite;
> > +
> > +                               if (*err)
> > +                                       return filtered;
> > +                               if (IS_ERR(filtered_suite)) {
> > +                                       *err = PTR_ERR(filtered_suite);
> > +                                       return filtered;
> > +                               }
> > +                       }
> > +               }
> > +
> >                 if (!filtered_suite)
> >                         continue;
> >
> > @@ -144,8 +184,8 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
> >         }
> >         filtered.end = copy;
> >
> > -       kfree(filter.suite_glob);
> > -       kfree(filter.test_glob);
> > +       kfree(parsed_glob.suite_glob);
> > +       kfree(parsed_glob.test_glob);
> >         return filtered;
> >  }
> >
> > @@ -203,8 +243,9 @@ int kunit_run_all_tests(void)
> >                 goto out;
> >         }
> >
> > -       if (filter_glob_param) {
> > -               suite_set = kunit_filter_suites(&suite_set, filter_glob_param, &err);
> > +       if (filter_glob_param || filter_count) {
> > +               suite_set = kunit_filter_suites(&suite_set, filter_glob_param,
> > +                               filter_param, filter_count, &err);
> >                 if (err) {
> >                         pr_err("kunit executor: error filtering suites: %d\n", err);
> >                         goto out;
> > @@ -218,7 +259,7 @@ int kunit_run_all_tests(void)
> >         else
> >                 pr_err("kunit executor: unknown action '%s'\n", action_param);
> >
> > -       if (filter_glob_param) { /* a copy was made of each suite */
> > +       if (filter_glob_param || filter_count) { /* a copy was made of each suite */
> >                 kunit_free_suite_set(suite_set);
> >         }
> >
> > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> > index ce6749af374d..4c8cb46857b2 100644
> > --- a/lib/kunit/executor_test.c
> > +++ b/lib/kunit/executor_test.c
> > @@ -24,7 +24,7 @@ static struct kunit_case dummy_test_cases[] = {
> >
> >  static void parse_filter_test(struct kunit *test)
> >  {
> > -       struct kunit_test_filter filter = {NULL, NULL};
> > +       struct kunit_glob_filter filter = {NULL, NULL};
> >
> >         kunit_parse_filter_glob(&filter, "suite");
> >         KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite");
> > @@ -50,7 +50,7 @@ static void filter_suites_test(struct kunit *test)
> >         subsuite[1] = alloc_fake_suite(test, "suite2", dummy_test_cases);
> >
> >         /* Want: suite1, suite2, NULL -> suite2, NULL */
> > -       got = kunit_filter_suites(&suite_set, "suite2", &err);
> > +       got = kunit_filter_suites(&suite_set, "suite2", NULL, 0, &err);
> >         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> >         KUNIT_ASSERT_EQ(test, err, 0);
> >         kfree_at_end(test, got.start);
> > @@ -74,7 +74,7 @@ static void filter_suites_test_glob_test(struct kunit *test)
> >         subsuite[1] = alloc_fake_suite(test, "suite2", dummy_test_cases);
> >
> >         /* Want: suite1, suite2, NULL -> suite2 (just test1), NULL */
> > -       got = kunit_filter_suites(&suite_set, "suite2.test2", &err);
> > +       got = kunit_filter_suites(&suite_set, "suite2.test2", NULL, 0, &err);
> >         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> >         KUNIT_ASSERT_EQ(test, err, 0);
> >         kfree_at_end(test, got.start);
> > @@ -100,7 +100,7 @@ static void filter_suites_to_empty_test(struct kunit *test)
> >         subsuite[0] = alloc_fake_suite(test, "suite1", dummy_test_cases);
> >         subsuite[1] = alloc_fake_suite(test, "suite2", dummy_test_cases);
> >
> > -       got = kunit_filter_suites(&suite_set, "not_found", &err);
> > +       got = kunit_filter_suites(&suite_set, "not_found", NULL, 0, &err);
> >         KUNIT_ASSERT_EQ(test, err, 0);
> >         kfree_at_end(test, got.start); /* just in case */
> >
>
> It'd be nice to add some more tests for attribute filtering specifically here.

Yeah, I agree. I will go ahead and add some here.

Thanks for all the comments!
-Rae

>
>
>
> > --
> > 2.41.0.162.gfafddb0af9-goog
> >