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

[Lucas De Marchi]

On Thu, Oct 31, 2024 at 03:58:11PM +0100, Janusz Krzysztofik wrote:
>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.


oh, I see. I still don't agree with the approach of using skip here, but
I don't care enough to postpone this. Let me fix it and resubmit.
I went ahead and merged the second patch as it doesn't depend on this.

thanks
Lucas De Marchi

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