[PATCH i-g-t 1/2] lib/igt_kmod: Drop legacy kunit fallback

[Janusz Krzysztofik]

On Thursday, 31 October 2024 14:18:25 CET Lucas De Marchi wrote:
> On Thu, Oct 31, 2024 at 11:42:58AM +0100, Janusz Krzysztofik wrote:
> >Hi Lucas,
> >
> >On Thursday, 31 October 2024 01:31:41 CET Lucas De Marchi wrote:
> >> Kunit has support for listing tests with debugfs for quite some time,
> >> there's no need for the fallback to be happening: let's just start
> >> failing the tests if we can't get the list of tests from debugfs.
> >
> >We must not fail, we may skip, see below.
> >
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> >> ---
> >>  lib/igt_kmod.c | 278 ++-----------------------------------------------
> >>  1 file changed, 10 insertions(+), 268 deletions(-)
> >>
> >> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> >> index 75a0d057c..4cb9e886d 100644
> >> --- a/lib/igt_kmod.c
> >> +++ b/lib/igt_kmod.c
> >...
> >> @@ -1523,25 +1268,22 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
> >>
> >>  	/*
> >>  	 * We need to use igt_subtest here, as otherwise it may crash with:
> >> -	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
> >> +	 * "skipping is allowed only in fixtures, subtests or igt_simple_main"
> >>  	 * if used on igt_main. This is also needed in order to provide
> >>  	 * proper namespace for dynamic subtests, with is required for CI
> >>  	 * and for documentation.
> >>  	 */
> >>  	igt_subtest_with_dynamic(subtest) {
> >> -		/*
> >> -		 * TODO: As soon as no longer needed by major Linux
> >> -		 *	 distributions, replace the fallback to
> >> -		 *	 __igt_kunit_legacy() processing path, required by
> >> -		 *	 LTS kernels not capable of using KUnit filters for
> >> -		 *	 listing test cases in KTAP format, with igt_require.
> >> -		 */
> >> +		bool has_tests;
> >> +
> >>  		kunit_debugfs_path(debugfs_path);
> >> -		if (igt_debug_on(!*debugfs_path) ||
> >> -		    !kunit_get_tests(&tests, &tst, suite, opts, debugfs_path, &debugfs_dir, &ktap))
> >> -			__igt_kunit_legacy(&tst, subtest, opts);
> >> -		else
> >> -			__igt_kunit(&tst, subtest, opts, debugfs_path, &tests, &ktap);
> >> +		igt_assert(*debugfs_path);
> >
> >According to a comment followed by respective assert() inside
> >lib/igt_core.c:igt_fail():
> >
> >	/* Dynamic subtest containers must not fail explicitly */
> >
> >that's why I suggested replacing the fallback with igt_require(), not with
> >igt_assert().
> 
> where did you suggest that?

In the above TODO comment you are addressing with this patch.

Janusz

> 
> >
> >But maybe we could go a step forward and replace all
> >
> >	if (igt_debug_on(...))
> >		return;
> >
> >inside kunit_debugfs_path() with
> >
> >	igt_skip_on(...);
> 
> 
> I'd rather not skip and have a big red FAIL since it's not a valid
> setup.
> 
> >
> >then we needed neither igt_require() nor igt_assert() here.
> >
> >> +
> >> +		has_tests = kunit_get_tests(&tests, &tst, suite, opts,
> >> +					    debugfs_path, &debugfs_dir, &ktap);
> >> +		igt_assert(has_tests);
> >
> >Ditto, optionally with all
> >
> >	if (igt_debug_on(...))
> >		return false;
> >
> >inside kunit_get_tests() replaced with
> >
> >	igt_skip_on(...);
> 
> if those writes fail for whatever reason, I don't want the test to skip,
> I want them to fail so we can fix them.
> 
> a skip is a valid thing when there's difference in behavior from
> platform to platform or when it's a valid difference in env setup. Here
> it's not.
> 
> Lucas De Marchi
> 
> >
> >and kunit_get_tests() returning void.
> >
> >Thanks,
> >Janusz
> >
> >> +
> >> +		__igt_kunit(&tst, subtest, opts, debugfs_path, &tests, &ktap);
> >>  	}
> >>
> >>  	igt_fixture {
> >>
> >
> >
> >
> >
>