diff mbox series

[RFC,v2,8/9] kunit: add tests for filtering attributes

Message ID 20230707210947.1208717-9-rmoar@google.com
State New
Headers show
Series kunit: Add test attributes API | expand

Commit Message

Rae Moar July 7, 2023, 9:09 p.m. UTC
Add four tests to executor_test.c to test behavior of filtering attributes.

- parse_filter_attr_test - to test the parsing of inputted filters

- filter_attr_test - to test the filtering procedure on attributes

- filter_attr_empty_test - to test the behavior when all tests are filtered
  out

- filter_attr_skip_test - to test the configurable filter_skip option

Signed-off-by: Rae Moar <rmoar@google.com>
---

Changes since v1:
- This is a new patch

 lib/kunit/executor_test.c | 107 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

Comments

Daniel Latypov July 10, 2023, 6:07 p.m. UTC | #1
On Fri, Jul 7, 2023 at 2:10 PM Rae Moar <rmoar@google.com> wrote:
>
> Add four tests to executor_test.c to test behavior of filtering attributes.
>
> - parse_filter_attr_test - to test the parsing of inputted filters
>
> - filter_attr_test - to test the filtering procedure on attributes
>
> - filter_attr_empty_test - to test the behavior when all tests are filtered
>   out
>
> - filter_attr_skip_test - to test the configurable filter_skip option
>
> Signed-off-by: Rae Moar <rmoar@google.com>

I love that I'm able to read this patch first and get a feel for what
exactly the patch series is doing overall.

Some nits and suggestions below.

> ---
>
> Changes since v1:
> - This is a new patch
>
>  lib/kunit/executor_test.c | 107 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
>
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> index d7ab069324b5..145a78ade33d 100644
> --- a/lib/kunit/executor_test.c
> +++ b/lib/kunit/executor_test.c
> @@ -7,6 +7,7 @@
>   */
>
>  #include <kunit/test.h>
> +#include <kunit/attributes.h>
>
>  static void kfree_at_end(struct kunit *test, const void *to_free);
>  static struct kunit_suite *alloc_fake_suite(struct kunit *test,
> @@ -22,6 +23,14 @@ static struct kunit_case dummy_test_cases[] = {
>         {},
>  };
>
> +static struct kunit_case dummy_attr_test_cases[] = {
> +       /* .run_case is not important, just needs to be non-NULL */
> +       { .name = "test1", .run_case = dummy_test, .module_name = "dummy",
> +         .attr.speed = KUNIT_SPEED_SLOW },
> +       { .name = "test2", .run_case = dummy_test, .module_name = "dummy" },
> +       {},
> +};

1) can we move this array to be just above parse_filter_attr_test so
it's next to where it's used?

2) How about renaming "test1" to "slow" to make the assertions in the
test case a bit easier to follow?
Right now readers need to remember which test case was supposed to be
filtered out.


> +
>  static void parse_filter_test(struct kunit *test)
>  {
>         struct kunit_glob_filter filter = {NULL, NULL};
> @@ -108,11 +117,109 @@ static void filter_suites_to_empty_test(struct kunit *test)
>                                 "should be empty to indicate no match");
>  }
>
> +static void parse_filter_attr_test(struct kunit *test)
> +{
> +       int j, filter_count;
> +       struct kunit_attr_filter *parsed_filters;
> +       char *filters = "speed>slow, module!=example";
> +       int err = 0;
> +
> +       filter_count = kunit_get_filter_count(filters);
> +       KUNIT_EXPECT_EQ(test, filter_count, 2);
> +
> +       parsed_filters = kcalloc(filter_count + 1, sizeof(*parsed_filters), GFP_KERNEL);

nit: kunit_kcalloc() instead?

> +       for (j = 0; j < filter_count; j++)
> +               parsed_filters[j] = kunit_next_attr_filter(&filters, &err);

then here we probably want to check err, i.e.
  KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter '%s'", filters[i]);


> +
> +       KUNIT_EXPECT_STREQ(test, kunit_attr_filter_name(parsed_filters[0]), "speed");
> +       KUNIT_EXPECT_STREQ(test, parsed_filters[0].input, ">slow");
> +
> +       KUNIT_EXPECT_STREQ(test, kunit_attr_filter_name(parsed_filters[1]), "module");
> +       KUNIT_EXPECT_STREQ(test, parsed_filters[1].input, "!=example");
> +
> +       kfree(parsed_filters);
> +}
> +
> +static void filter_attr_test(struct kunit *test)
> +{
> +       struct kunit_suite *subsuite[3] = {NULL, NULL};
> +       struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]};
> +       struct suite_set got;
> +       int err = 0;
> +
> +       subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
> +       subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
> +       subsuite[1]->attr.speed = KUNIT_SPEED_SLOW; // Set suite attribute

Similarly, perhaps we can rename suite2 to "slow_suite"?
That would cause this line to go over 80 characters wide, but since
that's no longer a hard limit, I think this would be a decent place to
go past it.

> +
> +       /* Want: suite1(test1, test2), suite2(test1, test2), NULL -> suite1(test2), NULL */
> +       got = kunit_filter_suites(&suite_set, NULL, "speed>slow", NULL, &err);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> +       KUNIT_ASSERT_EQ(test, err, 0);
> +       kfree_at_end(test, got.start);
> +
> +       /* Validate we just have suite1 */
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
> +       KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->name, "suite1");
> +       KUNIT_ASSERT_EQ(test, got.end - got.start, 1);
> +
> +       /* Now validate we just have test2 */
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
> +       KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test2");
> +       KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
> +}
> +
> +static void filter_attr_empty_test(struct kunit *test)
> +{
> +       struct kunit_suite *subsuite[3] = {NULL, NULL};
> +       struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]};
> +       struct suite_set got;
> +       int err = 0;
> +
> +       subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
> +       subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
> +
> +       got = kunit_filter_suites(&suite_set, NULL, "module!=dummy", NULL, &err);
> +       KUNIT_ASSERT_EQ(test, err, 0);
> +       kfree_at_end(test, got.start); /* just in case */
> +
> +       KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
> +                               "should be empty to indicate no match");
> +}
> +
> +static void filter_attr_skip_test(struct kunit *test)
> +{
> +       struct kunit_suite *subsuite[2] = {NULL};
> +       struct suite_set suite_set = {.start = subsuite, .end = &subsuite[1]};
> +       struct suite_set got;
> +       int err = 0;
> +
> +       subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
> +
> +       /* Want: suite1(test1, test2), NULL -> suite1(test1 with SKIP, test2), NULL */
> +       got = kunit_filter_suites(&suite_set, NULL, "speed>slow", "skip", &err);
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> +       KUNIT_ASSERT_EQ(test, err, 0);
> +       kfree_at_end(test, got.start);
> +
> +       /* Validate we have both test1 and test2 */
> +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);

Should we assert that we have 2 test cases before we dereference the second one?
The other code in this file (that I wrote) is being a bit sloppy and
deref'ing test_cases[0] without checking. It's doing that since I was
relying on the fact that the filtering code drops suites with no test
cases, so we don't necessarily need to check len(test_cases) >= 1.
(In terms of best practices, we should be defensive and checking that, though).

But in this case, we have no such guarantee about the second element.

> +       KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test1");
> +       KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[1].name, "test2");

Trying to remember, I think the cast to `const char *` is no longer
necessary after one of David's changes...
I think we might just never have gotten around to cleaning that up due
to the ordering in which the patches went in...

> +
> +       /* Now ensure test1 is skipped and test2 is not */
> +       KUNIT_EXPECT_EQ(test, got.start[0]->test_cases[0].status, KUNIT_SKIPPED);
> +       KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].status);

Should we check that it's equal to KUNIT_SUCCESS instead?


> +}
> +
>  static struct kunit_case executor_test_cases[] = {
>         KUNIT_CASE(parse_filter_test),
>         KUNIT_CASE(filter_suites_test),
>         KUNIT_CASE(filter_suites_test_glob_test),
>         KUNIT_CASE(filter_suites_to_empty_test),
> +       KUNIT_CASE(parse_filter_attr_test),
> +       KUNIT_CASE(filter_attr_test),
> +       KUNIT_CASE(filter_attr_empty_test),
> +       KUNIT_CASE(filter_attr_skip_test),
>         {}
>  };
>
> --
> 2.41.0.255.g8b1d071c50-goog
>
Rae Moar July 12, 2023, 9:24 p.m. UTC | #2
On Mon, Jul 10, 2023 at 2:07 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> On Fri, Jul 7, 2023 at 2:10 PM Rae Moar <rmoar@google.com> wrote:
> >
> > Add four tests to executor_test.c to test behavior of filtering attributes.
> >
> > - parse_filter_attr_test - to test the parsing of inputted filters
> >
> > - filter_attr_test - to test the filtering procedure on attributes
> >
> > - filter_attr_empty_test - to test the behavior when all tests are filtered
> >   out
> >
> > - filter_attr_skip_test - to test the configurable filter_skip option
> >
> > Signed-off-by: Rae Moar <rmoar@google.com>
>
> I love that I'm able to read this patch first and get a feel for what
> exactly the patch series is doing overall.


Thanks!

>
>
> Some nits and suggestions below.
>
> > ---
> >
> > Changes since v1:
> > - This is a new patch
> >
> >  lib/kunit/executor_test.c | 107 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> >
> > diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> > index d7ab069324b5..145a78ade33d 100644
> > --- a/lib/kunit/executor_test.c
> > +++ b/lib/kunit/executor_test.c
> > @@ -7,6 +7,7 @@
> >   */
> >
> >  #include <kunit/test.h>
> > +#include <kunit/attributes.h>
> >
> >  static void kfree_at_end(struct kunit *test, const void *to_free);
> >  static struct kunit_suite *alloc_fake_suite(struct kunit *test,
> > @@ -22,6 +23,14 @@ static struct kunit_case dummy_test_cases[] = {
> >         {},
> >  };
> >
> > +static struct kunit_case dummy_attr_test_cases[] = {
> > +       /* .run_case is not important, just needs to be non-NULL */
> > +       { .name = "test1", .run_case = dummy_test, .module_name = "dummy",
> > +         .attr.speed = KUNIT_SPEED_SLOW },
> > +       { .name = "test2", .run_case = dummy_test, .module_name = "dummy" },
> > +       {},
> > +};
>
> 1) can we move this array to be just above parse_filter_attr_test so
> it's next to where it's used?

This seems like a great idea. I will move it down.

>
>
> 2) How about renaming "test1" to "slow" to make the assertions in the
> test case a bit easier to follow?
> Right now readers need to remember which test case was supposed to be
> filtered out.

Yes this sounds good. I think including "slow" would be helpful
although I might also consider the name "slow_test".

>
>
> > +
> >  static void parse_filter_test(struct kunit *test)
> >  {
> >         struct kunit_glob_filter filter = {NULL, NULL};
> > @@ -108,11 +117,109 @@ static void filter_suites_to_empty_test(struct kunit *test)
> >                                 "should be empty to indicate no match");
> >  }
> >
> > +static void parse_filter_attr_test(struct kunit *test)
> > +{
> > +       int j, filter_count;
> > +       struct kunit_attr_filter *parsed_filters;
> > +       char *filters = "speed>slow, module!=example";
> > +       int err = 0;
> > +
> > +       filter_count = kunit_get_filter_count(filters);
> > +       KUNIT_EXPECT_EQ(test, filter_count, 2);
> > +
> > +       parsed_filters = kcalloc(filter_count + 1, sizeof(*parsed_filters), GFP_KERNEL);
>
> nit: kunit_kcalloc() instead?

Right, that makes sense. Thanks!

>
> > +       for (j = 0; j < filter_count; j++)
> > +               parsed_filters[j] = kunit_next_attr_filter(&filters, &err);
>
> then here we probably want to check err, i.e.
>   KUNIT_ASSERT_EQ_MSG(test, err, 0, "failed to parse filter '%s'", filters[i]);
>

Sounds good. I will add this.

>
> > +
> > +       KUNIT_EXPECT_STREQ(test, kunit_attr_filter_name(parsed_filters[0]), "speed");
> > +       KUNIT_EXPECT_STREQ(test, parsed_filters[0].input, ">slow");
> > +
> > +       KUNIT_EXPECT_STREQ(test, kunit_attr_filter_name(parsed_filters[1]), "module");
> > +       KUNIT_EXPECT_STREQ(test, parsed_filters[1].input, "!=example");
> > +
> > +       kfree(parsed_filters);
> > +}
> > +
> > +static void filter_attr_test(struct kunit *test)
> > +{
> > +       struct kunit_suite *subsuite[3] = {NULL, NULL};
> > +       struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]};
> > +       struct suite_set got;
> > +       int err = 0;
> > +
> > +       subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
> > +       subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
> > +       subsuite[1]->attr.speed = KUNIT_SPEED_SLOW; // Set suite attribute
>
> Similarly, perhaps we can rename suite2 to "slow_suite"?
> That would cause this line to go over 80 characters wide, but since
> that's no longer a hard limit, I think this would be a decent place to
> go past it.

Like above, I like this idea. I'll change the name. Interesting idea
about the 80 character limit also.

>
>
> > +
> > +       /* Want: suite1(test1, test2), suite2(test1, test2), NULL -> suite1(test2), NULL */
> > +       got = kunit_filter_suites(&suite_set, NULL, "speed>slow", NULL, &err);
> > +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> > +       KUNIT_ASSERT_EQ(test, err, 0);
> > +       kfree_at_end(test, got.start);
> > +
> > +       /* Validate we just have suite1 */
> > +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
> > +       KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->name, "suite1");
> > +       KUNIT_ASSERT_EQ(test, got.end - got.start, 1);
> > +
> > +       /* Now validate we just have test2 */
> > +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
> > +       KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test2");
> > +       KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
> > +}
> > +
> > +static void filter_attr_empty_test(struct kunit *test)
> > +{
> > +       struct kunit_suite *subsuite[3] = {NULL, NULL};
> > +       struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]};
> > +       struct suite_set got;
> > +       int err = 0;
> > +
> > +       subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
> > +       subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
> > +
> > +       got = kunit_filter_suites(&suite_set, NULL, "module!=dummy", NULL, &err);
> > +       KUNIT_ASSERT_EQ(test, err, 0);
> > +       kfree_at_end(test, got.start); /* just in case */
> > +
> > +       KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
> > +                               "should be empty to indicate no match");
> > +}
> > +
> > +static void filter_attr_skip_test(struct kunit *test)
> > +{
> > +       struct kunit_suite *subsuite[2] = {NULL};
> > +       struct suite_set suite_set = {.start = subsuite, .end = &subsuite[1]};
> > +       struct suite_set got;
> > +       int err = 0;
> > +
> > +       subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
> > +
> > +       /* Want: suite1(test1, test2), NULL -> suite1(test1 with SKIP, test2), NULL */
> > +       got = kunit_filter_suites(&suite_set, NULL, "speed>slow", "skip", &err);
> > +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
> > +       KUNIT_ASSERT_EQ(test, err, 0);
> > +       kfree_at_end(test, got.start);
> > +
> > +       /* Validate we have both test1 and test2 */
> > +       KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
>
> Should we assert that we have 2 test cases before we dereference the second one?
> The other code in this file (that I wrote) is being a bit sloppy and
> deref'ing test_cases[0] without checking. It's doing that since I was
> relying on the fact that the filtering code drops suites with no test
> cases, so we don't necessarily need to check len(test_cases) >= 1.
> (In terms of best practices, we should be defensive and checking that, though).
>
> But in this case, we have no such guarantee about the second element.

Good point. I'll add an assert statement here about the length of test_cases.

>
>
> > +       KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test1");
> > +       KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[1].name, "test2");
>
> Trying to remember, I think the cast to `const char *` is no longer
> necessary after one of David's changes...
> I think we might just never have gotten around to cleaning that up due
> to the ordering in which the patches went in...

Ahh got it. That is my bad. I'll double check if these are necessary.

>
>
> > +
> > +       /* Now ensure test1 is skipped and test2 is not */
> > +       KUNIT_EXPECT_EQ(test, got.start[0]->test_cases[0].status, KUNIT_SKIPPED);
> > +       KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].status);
>
> Should we check that it's equal to KUNIT_SUCCESS instead?
>

I wouldn't expect the status to be set in this case. But the status is
returning as 0 so it would pass for both the assert statement above
and if it's equal to KUNIT_SUCCESS. But since it is not supposed to be
set to KUNIT_SUCCESS, I'm inclined to keep it this way.

Thanks for all the comments Daniel!
-Rae

>
>
> > +}
> > +
> >  static struct kunit_case executor_test_cases[] = {
> >         KUNIT_CASE(parse_filter_test),
> >         KUNIT_CASE(filter_suites_test),
> >         KUNIT_CASE(filter_suites_test_glob_test),
> >         KUNIT_CASE(filter_suites_to_empty_test),
> > +       KUNIT_CASE(parse_filter_attr_test),
> > +       KUNIT_CASE(filter_attr_test),
> > +       KUNIT_CASE(filter_attr_empty_test),
> > +       KUNIT_CASE(filter_attr_skip_test),
> >         {}
> >  };
> >
> > --
> > 2.41.0.255.g8b1d071c50-goog
> >
diff mbox series

Patch

diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index d7ab069324b5..145a78ade33d 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -7,6 +7,7 @@ 
  */
 
 #include <kunit/test.h>
+#include <kunit/attributes.h>
 
 static void kfree_at_end(struct kunit *test, const void *to_free);
 static struct kunit_suite *alloc_fake_suite(struct kunit *test,
@@ -22,6 +23,14 @@  static struct kunit_case dummy_test_cases[] = {
 	{},
 };
 
+static struct kunit_case dummy_attr_test_cases[] = {
+	/* .run_case is not important, just needs to be non-NULL */
+	{ .name = "test1", .run_case = dummy_test, .module_name = "dummy",
+	  .attr.speed = KUNIT_SPEED_SLOW },
+	{ .name = "test2", .run_case = dummy_test, .module_name = "dummy" },
+	{},
+};
+
 static void parse_filter_test(struct kunit *test)
 {
 	struct kunit_glob_filter filter = {NULL, NULL};
@@ -108,11 +117,109 @@  static void filter_suites_to_empty_test(struct kunit *test)
 				"should be empty to indicate no match");
 }
 
+static void parse_filter_attr_test(struct kunit *test)
+{
+	int j, filter_count;
+	struct kunit_attr_filter *parsed_filters;
+	char *filters = "speed>slow, module!=example";
+	int err = 0;
+
+	filter_count = kunit_get_filter_count(filters);
+	KUNIT_EXPECT_EQ(test, filter_count, 2);
+
+	parsed_filters = kcalloc(filter_count + 1, sizeof(*parsed_filters), GFP_KERNEL);
+	for (j = 0; j < filter_count; j++)
+		parsed_filters[j] = kunit_next_attr_filter(&filters, &err);
+
+	KUNIT_EXPECT_STREQ(test, kunit_attr_filter_name(parsed_filters[0]), "speed");
+	KUNIT_EXPECT_STREQ(test, parsed_filters[0].input, ">slow");
+
+	KUNIT_EXPECT_STREQ(test, kunit_attr_filter_name(parsed_filters[1]), "module");
+	KUNIT_EXPECT_STREQ(test, parsed_filters[1].input, "!=example");
+
+	kfree(parsed_filters);
+}
+
+static void filter_attr_test(struct kunit *test)
+{
+	struct kunit_suite *subsuite[3] = {NULL, NULL};
+	struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]};
+	struct suite_set got;
+	int err = 0;
+
+	subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
+	subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
+	subsuite[1]->attr.speed = KUNIT_SPEED_SLOW; // Set suite attribute
+
+	/* Want: suite1(test1, test2), suite2(test1, test2), NULL -> suite1(test2), NULL */
+	got = kunit_filter_suites(&suite_set, NULL, "speed>slow", NULL, &err);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
+	KUNIT_ASSERT_EQ(test, err, 0);
+	kfree_at_end(test, got.start);
+
+	/* Validate we just have suite1 */
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
+	KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->name, "suite1");
+	KUNIT_ASSERT_EQ(test, got.end - got.start, 1);
+
+	/* Now validate we just have test2 */
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
+	KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test2");
+	KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
+}
+
+static void filter_attr_empty_test(struct kunit *test)
+{
+	struct kunit_suite *subsuite[3] = {NULL, NULL};
+	struct suite_set suite_set = {.start = subsuite, .end = &subsuite[2]};
+	struct suite_set got;
+	int err = 0;
+
+	subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
+	subsuite[1] = alloc_fake_suite(test, "suite2", dummy_attr_test_cases);
+
+	got = kunit_filter_suites(&suite_set, NULL, "module!=dummy", NULL, &err);
+	KUNIT_ASSERT_EQ(test, err, 0);
+	kfree_at_end(test, got.start); /* just in case */
+
+	KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
+				"should be empty to indicate no match");
+}
+
+static void filter_attr_skip_test(struct kunit *test)
+{
+	struct kunit_suite *subsuite[2] = {NULL};
+	struct suite_set suite_set = {.start = subsuite, .end = &subsuite[1]};
+	struct suite_set got;
+	int err = 0;
+
+	subsuite[0] = alloc_fake_suite(test, "suite1", dummy_attr_test_cases);
+
+	/* Want: suite1(test1, test2), NULL -> suite1(test1 with SKIP, test2), NULL */
+	got = kunit_filter_suites(&suite_set, NULL, "speed>slow", "skip", &err);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
+	KUNIT_ASSERT_EQ(test, err, 0);
+	kfree_at_end(test, got.start);
+
+	/* Validate we have both test1 and test2 */
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
+	KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test1");
+	KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[1].name, "test2");
+
+	/* Now ensure test1 is skipped and test2 is not */
+	KUNIT_EXPECT_EQ(test, got.start[0]->test_cases[0].status, KUNIT_SKIPPED);
+	KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].status);
+}
+
 static struct kunit_case executor_test_cases[] = {
 	KUNIT_CASE(parse_filter_test),
 	KUNIT_CASE(filter_suites_test),
 	KUNIT_CASE(filter_suites_test_glob_test),
 	KUNIT_CASE(filter_suites_to_empty_test),
+	KUNIT_CASE(parse_filter_attr_test),
+	KUNIT_CASE(filter_attr_test),
+	KUNIT_CASE(filter_attr_empty_test),
+	KUNIT_CASE(filter_attr_skip_test),
 	{}
 };