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

[Janusz Krzysztofik]

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().

But maybe we could go a step forward and replace all

	if (igt_debug_on(...))
		return;

inside kunit_debugfs_path() with

	igt_skip_on(...);

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(...);

and kunit_get_tests() returning void.

Thanks,
Janusz

> +
> +		__igt_kunit(&tst, subtest, opts, debugfs_path, &tests, &ktap);
>  	}
>  
>  	igt_fixture {
>