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" },{},+};
- 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.
- 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 attributeSimilarly, 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