[igt-dev] [PATCH i-g-t 10/10] KUnit: replace abort with graceful skip

[Janusz Krzysztofik]

On Tuesday, 13 June 2023 10:41:03 CEST Mauro Carvalho Chehab wrote:
> On Tue, 13 Jun 2023 09:27:26 +0200
> Dominik Karol Piatkowski <dominik.karol.piatkowski at intel.com> wrote:
> 
> > Sample drm_buddy output with missing requirements:
> > 	Starting subtest: drm_buddy_test
> > 	(drm_buddy:32218) igt_kmod-WARNING: Unable to load KUnit
> > 	Subtest drm_buddy_test: SKIP (0.001s)
> > 
> > Signed-off-by: Dominik Karol Piątkowski 
<dominik.karol.piatkowski at intel.com>
> > Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mauro.chehab at linux.intel.com>
> > ---
> >  lib/igt_kmod.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 1511bdef4..04220f404 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -763,27 +763,27 @@ static void __igt_kunit(const char *module_name, 
const char *opts)
> >  	int ret;
> >  	struct ktap_test_results *results;
> >  	struct ktap_test_results_element *temp;
> > +	bool skip = false;
> >  
> >  	/* get normalized module name */
> >  	if (igt_ktest_init(&tst, module_name) != 0) {
> > -		igt_warn("Unable to initialize ktest for %s\n", 
module_name);
> > -		igt_fail(IGT_EXIT_SKIP);
> > +		igt_skip("Unable to initialize ktest for %s\n", 
module_name);
> >  	}
> >  
> >  	if (igt_ktest_begin(&tst) != 0) {
> > -		igt_warn("Unable to begin ktest for %s\n", module_name);
> > -
> >  		igt_ktest_fini(&tst);
> > -		igt_fail(IGT_EXIT_SKIP);
> > +		igt_skip("Unable to begin ktest for %s\n", module_name);
> >  	}
> >  
> >  	if (tst.kmsg < 0) {
> >  		igt_warn("Could not open /dev/kmsg\n");
> > +		skip = true;
> 
> Hmm.... not being able to read from /dev/kmsg doesn't seem a reason for
> skipping the test.
> 
> IMO, the test should be skipped only if:
> 
> 1. The Kernel is not compiled with KUnit support;
> 2. The KUnit test was not compiled.
> 
> Errors for all other conditions should be considered as failures.
> 
> Now, I'm not sure what would be the proper way to test for such
> skip condition. I suspect you could check the error code at
> igt_kmod_load(), calling igt_skip() if it returns -ENOENT,
> e. g. on this part of __igt_kunit():
> 
>         /* The KUnit module is required for running any KUnit tests */
>         if (igt_kmod_load("kunit", NULL) != 0 ||
>             kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 
0) {
>                 igt_warn("Unable to load KUnit\n");
>                 igt_fail(IGT_EXIT_FAILURE);
>         }
> 
>         is_builtin = kmod_module_get_initstate(kunit_kmod) == 
KMOD_MODULE_BUILTIN;
> 
>         results = ktap_parser_start(f, is_builtin);
> 
>         if (igt_kmod_load(module_name, opts) != 0) {
>                 igt_warn("Unable to load %s module\n", module_name);
>                 ret = ktap_parser_stop();
>                 igt_fail(IGT_EXIT_FAILURE);
>         }
> 
> 
> This could be replaced by something similar to:
> 
> 	err = igt_kmod_load("kunit", NULL);
> 	igt_skip_on(err == ENOENT, "KUnit not supported");
> 	igt_fail_on_f(err != 0, "Error %d when trying to load kunit", err);
>         if (kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 
0) {
>                 igt_warn("Unable to load KUnit\n");
>                 igt_fail(IGT_EXIT_FAILURE);
>         }
> 
> 
>         is_builtin = kmod_module_get_initstate(kunit_kmod) == 
KMOD_MODULE_BUILTIN;
> 
>         results = ktap_parser_start(f, is_builtin);
> 
>         err = igt_kmod_load(module_name, opts) 
> 	igt_skip_on(err == ENOENT, "Module %s not found", module_name);
> 	if (err) {
>                 igt_warn("Error %d when trying to load %s module\n", err, 
module_name);
>                 ret = ktap_parser_stop();
>                 igt_fail(IGT_EXIT_FAILURE);
>         }
> 
> 
> In order to explicitly check for (1) and (2).

Why do we reinvent the wheel instead of following the pattern from 
igt_kselftest()?  We used to follow it, before the whole igt_kunit() body was 
put under igt_subtest_with_dynamic() (patch 6/8 of v5, then squashed with 5/5, 
IIRC).  That forced us to introduce a series of changes inside igt_kunit(), 
which we now contest.  Was that really necessary?

Thanks,
Janusz

> 
> Regards,
> Mauro
>