lkml.org 
[lkml]   [2023]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC v1 3/6] kunit: Add ability to filter attributes
    On Sat, Jun 10, 2023 at 4:29 AM David Gow <davidgow@google.com> wrote:
    >
    > On Sat, 10 Jun 2023 at 08:52, Rae Moar <rmoar@google.com> 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@google.com>
    > > ---
    >
    > 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
    > >

    \
     
     \ /
      Last update: 2023-06-13 22:43    [W:7.780 / U:0.096 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site