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

[Lucas De Marchi]

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?

>
>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 {
>>
>
>
>
>