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