[PATCH i-g-t v4 8/8] lib/kunit: Perform test suite filtering on the kernel side

[Janusz Krzysztofik]

Hi Kamil,

Thanks for your reviews, this one and others of the series.

On Tuesday, 6 February 2024 18:21:06 CET Kamil Konieczny wrote:
> Hi Janusz,
> On 2024-02-06 at 11:00:32 +0100, Janusz Krzysztofik wrote:
> > When getting a list of test cases provided by a KUnit test module, we now
> > set KUnit filters in a way that results in all test cases from all test
> > suites of that module being reported as skipped, then we delete test cases
> > that don't belong to a requested test suite from the list.  Moreover,
> > before executing test cases from that list, we modify KUnit filters in a
> > way that all test cases from all test suites of the test module are
> > executed, but we report only those from the requested test suite.  Since
> > KUnit base module has a capability of filtering test cases by test suite
> > name, which we don't use, our approach to filtering is not optimal.
> > 
> > Limit kernel side processing to test cases that belong to the requested
> > test suite by setting KUnit filter_glob parameter to that test suite name,
> > and drop user side test suite filtering.  With user side post-processing
> > of test case lists limited to SKIP verification, defer cleanup of memory
> > occupied by the list entries to a first igt_fixture section that follows.
> > 
> > Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > ---
> >  lib/igt_kmod.c | 34 +++++++++-------------------------
> >  1 file changed, 9 insertions(+), 25 deletions(-)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 506093c954..9b92696f1f 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -1157,7 +1157,6 @@ static bool kunit_get_tests(struct igt_list_head *tests,
> >  			    const char *suite,
> >  			    const char *opts)
> >  {
> > -	char *suite_name = NULL, *case_name = NULL;
> >  	struct igt_ktap_result *r, *rn;
> >  	struct igt_ktap_results *ktap;
> >  	unsigned long taints;
> > @@ -1179,7 +1178,7 @@ static bool kunit_get_tests(struct igt_list_head *tests,
> >  	 * perfectly -- seems to be more safe than extracting a test case list
> >  	 * of unknown length from /dev/kmsg.
> >  	 */
> > -	if (igt_debug_on(!kunit_set_filtering(NULL, "module=none", "skip")))
> > +	if (igt_debug_on(!kunit_set_filtering(suite, "module=none", "skip")))
> >  		return false;
> >  
> >  	igt_skip_on(modprobe(tst->kmod, opts));
> > @@ -1194,30 +1193,13 @@ static bool kunit_get_tests(struct igt_list_head *tests,
> >  
> >  	igt_ktap_free(ktap);
> >  
> > -	if (!err)
> > -		igt_list_for_each_entry_safe(r, rn, tests, link) {
> > -			if (igt_debug_on(r->code != IGT_EXIT_SKIP)) {
> > -				err = r->code ?: -EPROTO;
> > -				break;
> > -			}
> > -
> > -			if (!suite)
> > -				continue;
> > -
> > -			if (strcmp(r->suite_name, suite))
> > -				kunit_result_free(&r, &case_name, &suite_name);
> > -		}
> > -
> > -	if (err) {
> > -		kunit_results_free(tests, &case_name, &suite_name);
> > -	} else {
> > -		free(suite_name);
> > -		free(case_name);
> > -	}
> > -
> >  	igt_skip_on_f(err,
> >  		      "KTAP parser failed while getting a list of test cases\n");
> >  
> > +	igt_list_for_each_entry_safe(r, rn, tests, link)
> > +		igt_require_f(r->code == IGT_EXIT_SKIP,
> > +			      "Unexpected non-SKIP result while listing test cases\n");
> > +
> >  	igt_skip_on(kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE));
> >  
> >  	return true;
> > @@ -1225,6 +1207,7 @@ static bool kunit_get_tests(struct igt_list_head *tests,
> >  
> >  static void __igt_kunit(struct igt_ktest *tst,
> >  			const char *subtest,
> > +			const char *suite,
> >  			const char *opts,
> >  			struct igt_list_head *tests)
> >  {
> > @@ -1249,7 +1232,8 @@ static void __igt_kunit(struct igt_ktest *tst,
> >  			      t->case_name) {
> >  
> >  			if (!modprobe.thread) {
> > -				igt_require(kunit_set_filtering(NULL, NULL, NULL));
> > +				igt_require(kunit_set_filtering(suite,
> > +								NULL, NULL));
> 
> Can you keep it in one line? 

OK.

Thanks,
Janusz

> With or without it,
> 
> Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> 
> >  
> >  				igt_assert_eq(pthread_mutexattr_init(&attr), 0);
> >  				igt_assert_eq(pthread_mutexattr_setrobust(&attr,
> > @@ -1443,7 +1427,7 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
> >  		if (!kunit_get_tests(&tests, &tst, suite, opts))
> >  			__igt_kunit_legacy(&tst, subtest, opts);
> >  		else
> > -			__igt_kunit(&tst, subtest, opts, &tests);
> > +			__igt_kunit(&tst, subtest, suite, opts, &tests);
> >  	}
> >  
> >  	igt_fixture {
>