Received-SPF: pass (google.com: domain of linux-kernel+bounces-155911-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240423090808.242389-1-davidgow@google.com>
In-Reply-To: <20240423090808.242389-1-davidgow@google.com>
From: Rae Moar <rmoar@google.com>
Date: Tue, 23 Apr 2024 17:12:59 -0400
Message-ID: <CA+GJov4SLN-=V0RrqkgwG6WN-DUwcsmtpH-fEzJ4+NyOfaDgow@mail.gmail.com>
Subject: Re: [PATCH] kunit: test: Move fault tests behind KUNIT_FAULT_TEST
 Kconfig option
To: David Gow <davidgow@google.com>
Cc: =?UTF-8?B?TWlja2HDq2wgU2FsYcO8bg==?= <mic@digikod.net>, 
	Shuah Khan <skhan@linuxfoundation.org>, Guenter Roeck <linux@roeck-us.net>, 
	kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Apr 23, 2024 at 5:08=E2=80=AFAM David Gow <davidgow@google.com> wro=
te:
>
> The NULL dereference tests in kunit_fault deliberately trigger a kernel
> BUG(), and therefore print the associated stack trace, even when the
> test passes. This is both annoying (as it bloats the test output), and
> can confuse some test harnesses, which assume any BUG() is a failure.
>
> Allow these tests to be specifically disabled (without disabling all
> of KUnit's other tests), by placing them behind the
> CONFIG_KUNIT_FAULT_TEST Kconfig option. This is enabled by default, but
> can be set to 'n' to disable the test. An empty 'kunit_fault' suite is
> left behind, which will automatically be marked 'skipped'.
>
> As the fault tests already were disabled under UML (as they weren't
> compatible with its fault handling), we can simply adapt those
> conditions, and add a dependency on !UML for our new option.
>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Link: https://lore.kernel.org/all/928249cc-e027-4f7f-b43f-502f99a1ea63@ro=
eck-us.net/
> Fixes: 82b0beff3497 ("kunit: Add tests for fault")
> Signed-off-by: David Gow <davidgow@google.com>

Hello!

This looks good to me. Very useful!

Reviewed-by: Rae Moar <rmoar@google.com>

Thanks!
-Rae

> ---
>  lib/kunit/Kconfig      | 11 +++++++++++
>  lib/kunit/kunit-test.c |  8 ++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 68a6daec0aef..34d7242d526d 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -24,6 +24,17 @@ config KUNIT_DEBUGFS
>           test suite, which allow users to see results of the last test s=
uite
>           run that occurred.
>
> +config KUNIT_FAULT_TEST
> +       bool "Enable KUnit tests which print BUG stacktraces"
> +       depends on KUNIT_TEST
> +       depends on !UML
> +       default y
> +       help
> +         Enables fault handling tests for the KUnit framework. These tes=
ts may
> +         trigger a kernel BUG(), and the associated stack trace, even wh=
en they
> +         pass. If this conflicts with your test infrastrcture (or is con=
fusing
> +         or annoying), they can be disabled by setting this to N.
> +
>  config KUNIT_TEST
>         tristate "KUnit test for KUnit" if !KUNIT_ALL_TESTS
>         default KUNIT_ALL_TESTS
> diff --git a/lib/kunit/kunit-test.c b/lib/kunit/kunit-test.c
> index 0fdca5fffaec..e3412e0ca399 100644
> --- a/lib/kunit/kunit-test.c
> +++ b/lib/kunit/kunit-test.c
> @@ -109,7 +109,7 @@ static struct kunit_suite kunit_try_catch_test_suite =
=3D {
>         .test_cases =3D kunit_try_catch_test_cases,
>  };
>
> -#ifndef CONFIG_UML
> +#if IS_ENABLED(CONFIG_KUNIT_FAULT_TEST)
>
>  static void kunit_test_null_dereference(void *data)
>  {
> @@ -136,12 +136,12 @@ static void kunit_test_fault_null_dereference(struc=
t kunit *test)
>         KUNIT_EXPECT_TRUE(test, ctx->function_called);
>  }
>
> -#endif /* !CONFIG_UML */
> +#endif /* CONFIG_KUNIT_FAULT_TEST */
>
>  static struct kunit_case kunit_fault_test_cases[] =3D {
> -#ifndef CONFIG_UML
> +#if IS_ENABLED(CONFIG_KUNIT_FAULT_TEST)
>         KUNIT_CASE(kunit_test_fault_null_dereference),
> -#endif /* !CONFIG_UML */
> +#endif /* CONFIG_KUNIT_FAULT_TEST */
>         {}
>  };
>
> --
> 2.44.0.769.g3c40516874-goog
>